fix(security): repoint custom-call-classifier completeness caveat off closed #741 (#764) - #824
Merged
Merged
Conversation
…losed #741 (#764) The COMPLETENESS caveat in custom-call-classifier.ts (JSDoc above SEND_FAMILY_RECIPIENT_GATE, ~:55) and the trailing note on assertSendFamilyRecipientIsWallet's non-bypassable refusal message (~:150) both cited `(issue #741)` as the open home for the send-family gate's completeness question. #741 was closed by PR #753 on its own acceptance criterion (the nine gated selectors are correct) while explicitly leaving completeness unsettled per SEC's certification. The pointer was dangling: a reader following it landed on a resolved issue and could reasonably (and wrongly) conclude completeness was settled. Repoints both references to #764, the open issue that is itself the "immediate, scoped (DEV)" ask this commit satisfies. Scoped to ONLY the dangling-pointer repoint per ARCH's #776 disposition ("Ship NOW ... Repoint it to the OPEN substrate home ... Mechanical, no design dependency"). Does NOT attempt the substantive completeness fix (#757's structural recipient-decode seam, the 7 confirmed-unclassified selectors) — that is explicitly folded into #776's design, gated on SEC/ARCH, not a DEV task. Adds test/764-completeness-caveat-repoint.test.ts as a source-text-scan regression guard (same pattern as test/presign-annotation-guard-735.test.ts): asserts the classifier source no longer contains a `(issue #741)` pointer, and that the JSDoc and refusal-message caveats each resolve to a non-#741 issue number, independently.
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 #764
What changed and why
src/security/custom-call-classifier.tscarried two in-code pointers marking the send-family recipient gate's COMPLETENESS as an open question:SEND_FAMILY_RECIPIENT_GATE(~:55): "...other third-party-source→recipient selectors may exist and are pending a qualified security review (issue security: ERC-777 operatorSend untracked in custom-call classifier — exfil sibling of #711 (SEC/ARCH weigh in on send-family set) #741)."assertSendFamilyRecipientIsWallet's non-bypassable refusal message (~:150): "...recipient-verified send path, which does not exist yet — tracked as issue feature: recipient-verified send path (ERC-721/1155 transfer, ERC-4626 receiver) — restores #753-retired capability behind durable recipient binding #756. (issue security: ERC-777 operatorSend untracked in custom-call classifier — exfil sibling of #711 (SEC/ARCH weigh in on send-family set) #741)"#741 was closed by PR #753 on its own acceptance criterion (the nine gated selectors are correct — no wrong recipient index, no member that doesn't belong). SEC's certification on that PR explicitly left completeness unsettled; the caveat rode along with the closure anyway. A reader following
(issue #741)lands on a resolved, closed issue and reasonably (and wrongly) concludes completeness was settled.This PR repoints both references from
#741to#764— the open issue that is itself the "Required end state (1): Immediate, scoped (DEV)" target this commit satisfies, and the precise open home for the completeness gap.Scope is intentionally narrow, per ARCH's disposition on #776: "Ship NOW (trivial, DEV-ready — do not wait on the substrate design): the in-code caveat in
custom-call-classifier.ts(~:55 and ~:150) cites CLOSED #741 as its completeness home — a dangling pointer. Repoint it to the OPEN substrate home (#776), or to #757... Mechanical, no design dependency."What was deliberately left out
The substantive completeness fix — #757's structural recipient/beneficiary-parameter decoding on the signing path, needed to cover the 7 confirmed-unclassified selectors (Permit2
approve/transferFrom/batchtransferFrom,unwrapWETH9, ERC-777send, ERC-1363transferAndCall, Yearn-stylewithdraw) — is not attempted here. Per both #764's own body and ARCH's #776 comment, hand-enumeration does not converge (unwrapWETH9sits at the exact index the current model already supports and was still missed) and extending the selector list is explicitly the wrong fix. That work is folded into #776's design, gated on SEC/ARCH sign-off, and is not a DEV task.The #764 body's item (3) — settling the U1 premise (selector signature-strings vs. published EIP standards) against the actual EIPs — is also out of scope here; it's listed as low-priority and separate from the repoint.
Test — falsifier
Added
test/764-completeness-caveat-repoint.test.ts, following the same source-text-scan pattern as the existingtest/presign-annotation-guard-735.test.ts(rawreadFileSync+fileURLToPath(import.meta.url), no source import needed):(issue #741)substring anywhere — RED on unfixedmain(contains it twice), GREEN on this branch.SendFamilyGateEntryand asserts itsqualified security review (issue #NNN)caveat does not resolve to741.assertSendFamilyRecipientIsWalletfunction body and asserts its trailing(issue #NNN)caveat does not resolve to741.Tests 2 and 3 assert "not #741" rather than "must be #764" specifically, so a future re-scope (e.g. onto #757 once the seam ships) can repoint the source and stay green without touching this test — matching #764's own falsifiable acceptance criterion: "every in-code pointer to a completeness/security caveat resolves to an open issue."
No existing test asserts the literal
(issue #741)trailing string (the existing send-family-gate tests intest/custom-call.test.tsmatch the refusal message via/CUSTOM_CALL_REFUSED[\s\S]*NOT[\s\S]*bypassable/, not the trailing issue pointer), so nothing needed updating.Tests were NOT run locally — Node/npm are not installed on the authoring machine. CI (Build & Test on Node 20 + 22) is the authoritative check for this PR.
Blast radius
Two string literals changed inside comments/an error-message string in one file (
src/security/custom-call-classifier.ts); one new, self-contained test file. No behavioral change —assertSendFamilyRecipientIsWallet's refusal logic,SEND_FAMILY_RECIPIENT_GATEcontents, and every other classifier code path are untouched. The refusal message's content changes by two characters (issue number), which is cosmetic — no existing test asserts that trailing substring (see above).Residual concerns
Issue #741 — send-family recipient gate...") was deliberately left unchanged — it documents the design issue security: ERC-777 operatorSend untracked in custom-call classifier — exfil sibling of #711 (SEC/ARCH weigh in on send-family set) #741 shipped under (historical provenance), not a completeness caveat, and is outside both security: exfil-surface completeness has no open home — in-code caveat cites CLOSED #741; 7 confirmed unclassified selectors; hand-enumeration does not converge #764's stated scope (:55and:150only) and ARCH's arch(substrate): hand-maintained fail-open enumerations are this codebase's default guard habit — 4 instances (#757/#764/#772/#771) #776 comment.