fix(security): send-family gate follow-ups - NFT refusal wording + gate dedupe (#755) - #825
Merged
Merged
Conversation
…mily gate match (#755) Scope: items 3, 5, 6 from the #755 ARCH re-scope comment (item 4 is closed as intentional product scope at #756; items 1/2 were absorbed into #776/ #757 by the same comment). - The 0x23b872dd (transferFrom) refusal messages in applyCustomCallClassifier unconditionally told the caller to "use prepare_token_send instead". That selector's signature (address,address,uint256) is identical for ERC-20 and ERC-721 transferFrom, and no prepare_*-nft send tool exists yet (src/modules/nft/index.ts only exports read-only tools). Reworded both throw messages to note the ambiguity and stop asserting prepare_token_send unconditionally resolves it. - Added a one-line invariant comment at the send-family gate binding in buildCustomCall documenting that `data` must come from encodeFunctionData(p.args) — a future refactor accepting pre-encoded calldata would silently decouple the checked recipient from the signed recipient. - assertSendFamilyRecipientIsWallet now takes the already-matched gate entry instead of re-running matchSendFamilyGate(data) internally; buildCustomCall already computes the entry once and now passes it through instead of triggering a second redundant match.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #755
Scope
Per the ARCH re-scope comment on #755, this PR covers items 3, 5, 6
only:
intentional product scope, tracked at feature: recipient-verified send path (ERC-721/1155 transfer, ERC-4626 receiver) — restores #753-retired capability behind durable recipient binding #756 — not touched here.
were absorbed into arch(substrate): hand-maintained fail-open enumerations are this codebase's default guard habit — 4 instances (#757/#764/#772/#771) #776 and INCIDENT: pre-sign block 5 checks selector but never arguments — recipient-bearing fns on RECOGNIZED destinations are signable drain paths (Aave/Morpho/Uniswap/Lido) #757 respectively — not touched here.
What changed and why
Item 3 — misleading NFT refusal wording. The two
transferFrom(
0x23b872dd) refusal messages inapplyCustomCallClassifier(
src/security/custom-call-classifier.ts) unconditionally told thecaller to "use
prepare_token_sendinstead." ButtransferFrom'sselector/signature (
address,address,uint256) is byte-for-byteidentical for ERC-20
transferFromand ERC-721transferFrom— theclassifier cannot distinguish the two from the selector alone (see the
existing comment at line ~174-177), and no
prepare_*-nft send toolexists yet (
src/modules/nft/index.tsexports four read-only tools:getNftPortfolio,getNftCollection,getNftListings,getNftHistory— no send tool). So an NFT-shapedtransferFromcallhitting either the self-as-from or recipient-mismatch refusal was told
to use a tool (
prepare_token_send) that only handles ERC-20 andcannot actually resolve the call. Reworded both throw messages to
state the ambiguity explicitly and note that no dedicated NFT send
tool exists yet, instead of unconditionally pointing at
prepare_token_send.Item 5 — invariant comment at the gate binding. Added a one-line
(well, one-paragraph) comment at the send-family gate binding in
buildCustomCall(src/modules/custom-call/actions.ts) documentingthat the gate's correctness depends on
databeing built fromencodeFunctionData(p.args)— a future refactor that acceptedpre-encoded calldata directly would silently decouple the checked
recipient arg from the recipient actually being signed.
Item 6 — dedupe the double selector match.
buildCustomCallcalledmatchSendFamilyGate(data)to computesendFamilyGate, then calledassertSendFamilyRecipientIsWallet(data, ...), which re-ranmatchSendFamilyGate(data)internally on the same calldata. ChangedassertSendFamilyRecipientIsWallet's signature to take thealready-matched
SendFamilyGateEntry | nullinstead of rawdata, andupdated the one real call site to pass the entry it already computed.
Purely a redundancy removal — behavior is unchanged (both paths matched
the same selector against the same table).
Tests
Not run locally — Node/npm are not installed on the authoring
machine, so nothing here was executed. CI (Build & Test on Node 20 +
22) is the authoritative check for whether this compiles and passes.
Tests were written by matching
test/custom-call.test.ts's existingimports, ABI fixtures, and
try/catch-into-caughtassertion patternexactly.
Falsifier tests added (
test/custom-call.test.ts):"transferFrom(self,...) refusal doesn't unconditionally claim prepare_token_send handles it — the selector is shared with ERC-721 (issue #755)"— drivesbuildCustomCallwith atransferFrom(wallet, ATTACKER, "1")call (args decode as aplausible NFT transferFrom: a small integer "tokenId" rather than a
token amount) and asserts the thrown message matches both
/identical for ERC-20 and ERC-721 transferFrom/and/no dedicated send tool exists yet/. On unfixed code this message doesn't containeither phrase — the test FAILS. On the fixed code the reworded
message contains both — the test PASSES.
"transferFrom(other, ATTACKER) refusal doesn't unconditionally claim prepare_token_send handles it — the selector is shared with ERC-721 (issue #755)"— same shape, but exercises therecipient-mismatch throw path (
args[1] != wallet) instead of theself-as-from path. Same fail-on-unfixed / pass-on-fixed logic.
Existing test updated (not weakened — same assertions, new calling
convention): the
assertSendFamilyRecipientIsWallet — unit (issue #741)describe block's three tests calledassertSendFamilyRecipientIsWallet("0x...", bool)directly with rawcalldata, relying on the function's old internal re-match. Since the
function's signature changed to take the pre-matched entry (item 6),
these three tests now call
matchSendFamilyGate(...)first and passthe resulting entry in. The assertions themselves (throws
non-bypassably / no-op when recipient is wallet / no-op for a
non-member selector) are unchanged.
Blast radius
src/security/custom-call-classifier.ts: two error-message stringliterals reworded (no logic change);
assertSendFamilyRecipientIsWalletsignature changed from
(data, recipientIsWallet)to(entry, recipientIsWallet).src/modules/custom-call/actions.ts: one comment added; one call siteupdated to pass the already-computed entry instead of raw
data.test/custom-call.test.ts: two new tests, three existing tests updatedfor the new signature.
assertSendFamilyRecipientIsWalletis exported fromcustom-call-classifier.ts— grepped the repo for other importersbefore changing the signature;
src/modules/custom-call/actions.tsand
test/custom-call.test.tsare the only two call sites, bothupdated in this diff.
gate's actual verdicts — this PR is wording + a redundant-call
removal, not a security-behavior change.
Residual concerns
transferFromrefusal message — the one built from
rule.messageat the "ack !==true" branch — still frames itself as "ERC-20 transferFrom" and
points at "the protocol-specific prepare_* tool (Aave Pool, Uniswap
Router, etc.)" without the same ERC-721 caveat. That message never
literally asserts
prepare_token_sendresolves the call (it says"protocol-specific prepare_* tool", not
prepare_token_send), whichis why the security: #741 send-family gate follow-ups — Permit2 completeness + struct-recipient model + NFT-refusal message + ERC-4626 over-refusal #755 plan and this PR scoped the fix to the two throws
that do make that specific claim. Left as-is to keep this diff
targeted; flagging in case a reviewer wants that third message
reworded too for full consistency.
transferFromthat fails the self-as-from or recipient checks still has no
in-product remediation path beyond
prepare_revoke_approval— that'sa product gap, not something this PR's wording fix claims to close.