Run security-review / crypto-audit skill on the codebase after the address-book PR lands — focus on compromised-MCP surface

[szhygulin]

Context

The address-book feature (`claude-work/HIGHEST-plan-address-book-merged.md`) introduces a new attack surface that didn't exist before: a persistent, label-keyed address resolver. The plan's threat model already names the core risk —

The contacts file lives on disk. Without integrity, a compromised MCP / malware / second process can substitute `"Mom" → attacker_address`; the agent dutifully resolves the label, the user clicks through the device screen because they trust the name.

…and proposes a per-chain signed blob + anchor re-derivation + version high-water mark as defenses. Once this lands, the codebase's existing layered-defenses table in `SECURITY.md` needs to be re-evaluated against the new surface, and the design's claims need to be pressure-tested empirically rather than just on paper.

This issue tracks running the `security-review` (and/or `crypto-audit`) Claude skill against the merged address-book code, with focus on the compromised-MCP threat boundary.

Why this is a separate issue, not part of the address-book PR

The implementation PR will include the design's defenses. A clean security audit:

• needs to be run against the merged code, not the in-flight branch (so all integration points with existing tools are visible);
• is independent enough that bundling delays the address-book ship;
• should explicitly red-team the residuals the plan already names — not just verify the happy path the implementer wrote tests for.

Surfaces to audit

Signed-blob integrity (the load-bearing defense)

• BIP-137 / EIP-191 / TIP-191 / Solana off-chain message signing — does each chain's signing helper actually preimage-lock the JCS-canonicalized payload? Is the domain tag (`VaultPilot-contact-v1:`) hardwired inside the signing helper, or is there any path where caller-supplied bytes reach the device?
• Cross-chain replay: can a signature produced for chain A's contact list be re-used to validate chain B's contact list? The plan says `chainId` is in the signed payload — verify this empirically.
• JCS canonicalization (RFC-8785) — is the implementation conformant? Are there cases where two semantically-equal contact lists produce different JCS outputs? Conversely, can two semantically-different lists produce the same JCS output?
• Is the `signature` field actually excluded from its own preimage?

Anchor re-derivation

• `CONTACTS_ANCHOR_MISMATCH` is described as fail-hard. Is there any code path that catches and silently downgrades this to a warning?
• If the pairing cache (`session.ts` / `config.json`) is poisoned to a different paired address, does the in-memory anchor re-derivation from the device still catch the mismatch?
• What happens when the user has multiple paired accounts (e.g. `pair_ledger_btc` was called with multiple `accountIndex` values)? Which is the anchor? Is there ambiguity an attacker can exploit by selecting a different paired account?

Version rollback

• Plan explicitly names this residual: "cold-start rollback before any session has read the file is undetectable." Audit should confirm this is the only residual — i.e., no warm-session rollback path exists.
• Is the version high-water mark per-chain or global? If per-chain, can rolling back BTC's list independently of EVM's introduce a vulnerability?

Metadata sidecar (unsigned)

• What can an attacker do by mutating `metadata.` while leaving the signed entries intact? E.g., adding a `tags: ["verified safe"]` that the agent surfaces unconditionally. Does the agent prompt or any agent-facing string treat metadata as trusted?
• Per memory feedback_metadata_filter_not_surface — quality-flag metadata should be filtered from user view by default. Confirm this rule extends to address-book metadata.
• Can metadata mutation cause label collisions or join-table confusion at read time?

Resolver-chain composition (ENS / `.sol` / Bonfida)

• When name resolution composes with the contact list (e.g., user says "send to alice.eth"), does a compromised resolver attacker have any path to override a saved contact? Or a saved-contact attacker have any path to override a resolver result?
• Is the resolution order documented + audit-friendly (contact list → ENS → `.sol` → Bonfida vs. the reverse)?

Verification-block UX

• When the user sends to a contact (`prepare_*({ to: "Mom" })` resolved via the address book), does the verification block surface the actual hex/base58 address prominently, or does the label dominate? Per memory feedback_verify_block_ux, the verification UX is already a concern.
• On the Ledger device — is the address shown the resolved one, or the label? (The device sees the address. Confirm UX-side parity.)
• Active address-poisoning campaigns (suffixes 4361 / 4075 / 6a14 per the project memory) — does the address-book add-flow detect typosquats against the user's already-known wallets? Or is that a separate guard?

Tool surface

• Confirm `add_contact` / `remove_contact` / `list_contacts` / `verify_contacts` are the only MCP tools touching the contact store. No other tool should read contacts and take a write action based on them implicitly.
• Confirm no tool surface is added that lets a compromised MCP silently add contacts (the plan explicitly names "auto-import prompt is OUT" — verify this stays out and no equivalent silent-add path exists).
• Is the storage file (`~/.vaultpilot-mcp/contacts.json`) created with `0o600` and the symlink-rejection check from `writeUserConfig` reused (per the plan)?

Failure-path UX

• When v1.0 ships without Solana / TRON contact support, the plan says `add_contact({ chain: "solana" })` returns `CONTACTS_CHAIN_NOT_YET_SUPPORTED`. Confirm this is a loud refusal, not a silent no-op or a silent fall-through to "saved without signature".
• What happens when signature verification fails on a chain that v1.0 supports? Plan says fail-hard — confirm the failure mode propagates to the agent without leaking the "unverified" entries.

Suggested process

1. Wait for the address-book PR to land on `main`.
2. Spawn a fresh agent session (separate context, not the implementer's session — independent review).
3. Invoke the `security-review` skill against `main` with the address-book commit range.
4. Optionally invoke the `crypto-audit` skill for the signing-helper code paths (BIP-137 / EIP-191 / TIP-191 / Sol off-chain-message implementations) — these are the highest-stakes integration points.
5. File any findings as separate issues; this issue closes when the audit summary is posted as a comment.

Out of scope

• v1.5 chains (Solana / TRON contacts) — re-audit after they land.
• v2 defense-in-depth (cross-LLM verification, second-device attestation) — deferred to its own audit.
• Taproot BTC contacts — deferred until BIP-322 is in the Ledger BTC app.

Cross-references

• Address-book plan: `claude-work/HIGHEST-plan-address-book-merged.md`
• Existing security model: `SECURITY.md`
• Active address-poisoning campaign (memory `project_address_poisoning_targets`): vanity-lookalike suffixes 4361 / 4075 / 6a14 actively mimicking user wallets

[szhygulin]

Audit summary

Security review run against the merged address-book code on main (post-PR #284), with the focus the issue requested: the compromised-MCP threat boundary.

Result: no HIGH or MEDIUM findings meeting the security-review skill's reporting bar (>80% confidence, real exploit path). One LOW defense-in-depth note worth tracking, plus a documentation gap that landed as a separate PR.

Surfaces audited and verdicts

Signed-blob integrity (load-bearing)

• chainId is in the signing preimage (src/contacts/canonicalize.ts:80,98) → cross-chain replay blocked.
• signature field is excluded from SigningPreimage → signature-exclusion clean.
• entries array sorted by label ascending in buildSigningPreimage → canonical ordering deterministic.
• Domain prefix VaultPilot-contact-v1: is hardwired at the signing-helper layer (src/signers/contacts/{btc,evm}.ts), not the canonicalizer → caller-supplied bytes cannot reach the device under the contacts code path. (Path-C trade-off on EVM personal_sign namespace exposure is documented in SECURITY.md.)
• JCS canonicalizer is intentionally narrow (integers + strings + arrays + objects only); booleans/null/floats fail-loud. Schema doesn't use those shapes, so the simplification is safe.

Anchor re-derivation

• Anchor captured into in-memory state on session start; mismatch → CONTACTS_ANCHOR_MISMATCH hard-fail. No silent-downgrade path found.
• Pairing-cache poisoning does NOT defeat re-derivation because pair_ledger_* re-derives from the device, not from the cache.

Version rollback

• In-memory high-water mark is per-chain (matches the plan).
• Per-chain isolation is correct: rolling back BTC's list independently of EVM's only weakens BTC's protection — there's no cross-chain join that would let an attacker leverage one chain's rollback against another.
• Confirmed the only residual is the named one: cold-start rollback before any session has read the file is undetectable. SECURITY.md already names this honestly.

Resolver scope

• resolveRecipient (src/contacts/resolver.ts) implements: literal → contact → ENS → unknown.
• Abort-on-tamper is correctly scoped to the label-resolution path only. Literal-address sends and ENS-resolved sends proceed with a verification-block warning; only label-keyed sends hard-fail with CONTACTS_TAMPERED. This is the right call: it avoids weaponizing a tamper-warning into a denial-of-service vector for legitimate literal-address sends.
• ENS resolution is independent of the contact list; reverse-decoration silently skipped on tamper rather than fail-hard (SECURITY.md documents this).

Metadata sidecar

• Two-layer storage (signed per-chain blobs + unsigned metadata sidecar) per the plan. Notes/tags are explicitly unsigned and do NOT participate in resolution decisions — they only enrich verification-block decoration after the address is resolved through the signed path.
• No code path treats metadata as authoritative for routing.

Tool surface

• add_contact / remove_contact / list_contacts / verify_contacts are the only contact-store tools. Confirmed via grep against the tool registry.
• No silent-add path. The signing primitives (signContactsBlob*) are not exposed as MCP tools.
• No auto-import prompt was added — the explicit out-of-scope item from the plan stayed out.

Storage hardening

• Atomic write via tmp + rename (src/contacts/storage.ts:82-84).
• Symlink-rejection check on the final target path (src/contacts/storage.ts:73-79) — matches writeUserConfig precedent.
• File mode 0o600, dir 0o700.

Failure-path UX

• add_contact({ chain: "solana" }) returns CONTACTS_CHAIN_NOT_YET_SUPPORTED — loud refusal, not silent fall-through. Verified.
• Signature verification failure on supported chains propagates as CONTACTS_TAMPERED — verifier never returns the unverified entries to callers.

LOW finding (defense-in-depth, not blocking)

writeContactsFile symlink-check covers the final path but not the .vaultpilot.tmp staging path. An attacker with write access to ~/.vaultpilot-mcp/ who pre-creates contacts.json.vaultpilot.tmp as a symlink to a privileged file would have writeFileSync follow the symlink before the rename. Severity is LOW because the attacker needs prior write access to the user's config dir (a stronger compromise than is needed to mount any of the threats the address book is designed to defeat — they could just edit contacts.json directly). Recommended fix: also lstat-check the tmp path before write, mirroring the target check at src/contacts/storage.ts:73-79. Will file as a separate issue if the user wants to track it.

Documentation gap (resolved)

Auditing the post-merge signing surface surfaced that SECURITY.md did not describe the new free-form message-signing surface (sign_message_btc / sign_message_ltc plus the contacts-blob personal_sign path) or the agent-side MESSAGE-PREVIEW discipline. The companion preflight skill was also missing a v3-era carve-out (it would have false-positive-refused legitimate sign_message_btc/ltc calls). Both are addressed in:

• Skill PR: szhygulin/vaultpilot-skill#5 — adds Invariant Release v0.2.0 #8 (MESSAGE-PREVIEW block for free-form sign-message tools), corrects Invariant Fix Compound dust rendering and add tx simulation #7's overreach, adds Safe-tx note to Invariant Add CI workflow: typecheck + tests on Node 20/22 #1.
• MCP pin-bump + SECURITY.md: vaultpilot-mcp#316 — pins skill v4 SHA + sentinel; documents free-form message signing surface and agent-side MESSAGE-PREVIEW defense.

Out-of-scope reminders (per the issue)

• v1.5 chains (Solana / TRON contacts) — re-audit when they land.
• Taproot BTC contacts — deferred until BIP-322 is in the Ledger BTC app.
• v2 cross-LLM verification + second-device attestation — separate audit.

Closing.