Skip to content

design(security): recipient-resolution seam — where the recipient invariant lives (substrate review, #711/#727/#741/#737/#757 class) #759

Description

@angelazhy

Design task (Role: ARCH) — substrate review, not a point fix

Five separately-root-caused issues share one failure class on one substrate (the pre-sign safety gate): #711#727 (ERC-20 transferFrom recipient) → #741 (ERC-777 operatorSend) → #737 (ERC-20 transfer) → #757 (recipient-bearing functions on RECOGNIZED destinations — INCIDENT). Per the repo CLAUDE.md "Pre-Sign Gate Surface Sweeps" rule and the global ≥3-same-failure-class trigger, the next step is an architecture review of the layer, not a fifth selector patch. SEC filed the substrate-review; this issue is the ARCH design home for it.

Structural cause (SEC's diagnosis, verified in #757)

The gate keys on selector identity while the asset moves via an argument. Block 5 is allowedSelectors.has(selector) where the sets are computeSelectorsFromAbi(<protocol ABI>) — a selector allowlist derived from a protocol's own ABI necessarily admits that protocol's own recipient-bearing functions (withdraw(asset,amount,to), collect(...,recipient), borrow(...,receiver)). Block 4 (the only argument-agnostic catch-all, which consults the ack flags) is skipped precisely when the destination is recognized. So every recipient-bearing function in a pinned protocol ABI is a signable drain path to an arbitrary address, and no ack flag is load-bearing on that path. Sharp instance in #757: an ack-stamped prepare_custom_call to the real Aave V3 Pool with withdraw(USDC, MAX, ATTACKER) passes every block as designed.

The design question

Where should the recipient invariant live, such that ONE invariant closes the whole class instead of a perpetually-growing selector list? SEC's direction (input, not a ruling): an argument-level recipient-resolution seam — any address-typed recipient/receiver/owner/to argument resolves, and refuses non-bypassably when it is neither the wallet nor a saved contact.

Questions the verified artifact must settle

  1. Seam locus — pre-sign (argument decode inside assertTransactionSafe) vs prepare-time; and how it composes with blocks 4/5 for recognized vs unknown destinations.
  2. Per-ABI enumeration — the canonical set of address-typed recipient params (recipient/receiver/owner/to/dst/beneficiary) per recognized ABI, and how it stays complete as ABIs are added.
  3. Resolution + allow — contact-book resolution and the == wallet allow; behavior when the contact book is empty.
  4. Ack non-bypassability — the recipient invariant MUST NOT be bypassable by any ack flag (that is the entire exfil-defense point).
  5. Drain-target vs legitimate-protocol-target (the crux) — some recipient args legitimately point at a non-wallet contract (a router/vault/receiver). A naive wallet-or-contact-only rule would over-block real flows. The artifact must enumerate these exceptions and handle them explicitly, or the seam is unshippable.

Two shapes the design must cover (SEC inputs; reachability UNVERIFIED — recon confirming)

Acceptance criterion

A VERIFIED design artifact (design doc, reviewed to fixpoint and approved by REVIEW and SEC — the pre-sign exception surface takes a SEC verdict beside REVIEW's) that:

DEV implements the fix (#757) from the approved artifact; DEV is holding implementation pending it.

Status / dependencies

Role: ARCH

Metadata

Metadata

Assignees

No one assigned

    Labels

    security_findingSecurity gap surfaced via adversarial smoke-testtrackingGated on an external precondition; not actionable today

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions