Repository navigation
fix(x402): refuse an SVM feePayer equal to the payer and check the signer count - #686
Conversation
…gner count buildUnsignedSvmTransaction and createSvmPaymentPayload always write two signature slots (facilitator at 0, payer at 1), while buildMessageV0 counts signers from the account map and returns numRequiredSignatures for exactly this purpose. A server-supplied extra.feePayer equal to the paying wallet collapsed the header to one required signature, so the transaction carried two slots against a one-signer header and failed sanitization at broadcast with no useful message. Refuse that input up front so the caller moves to the next payment option, and assert the header agrees with the slot layout.
pr-reviewer Summary for #543321c✅ No issues found The code review completed successfully with no findings. Review effort: 2/5 (Simple) SummaryThis is a solid, well-scoped bug fix. The root cause (hardcoded A few things worth noting positively:
No findings. Token usage: 2,522 input, 2,506 output, 199,146 cache read, 27,321 cache write | Usage Guide New pushes are reviewed automatically with a 10-minute cooldown between reviews. To request a review at any time, comment |
There was a problem hiding this comment.
Auto-approved
This PR was automatically approved because:
- Claude recommends approval
- Claude assessed this as a moderate effort change
- The effort level is within the auto-approval threshold of 2
- No high or critical issues were detected
- Review comment contains non-blocking feedback
If you have any concerns, please request a manual review.
The fallback loop in createPaymentSignatures swallowed build errors silently, so a malformed server option (such as a self-sponsored feePayer) only ever surfaced as a generic payment failure after every option was exhausted. Log the skipped option and its reason on stderr, matching the existing [x402] policy messages.
|
Done in 64332ff: the fallback loop now logs |
There was a problem hiding this comment.
Auto-approved
This PR was automatically approved because:
- Claude recommends approval
- Claude assessed this as a moderate effort change
- The effort level is within the auto-approval threshold of 2
- No high or critical issues were detected
- Review comment contains non-blocking feedback
If you have any concerns, please request a manual review.
gulshngill
left a comment
There was a problem hiding this comment.
Thanks for this. The bug is real and the fix is correct; I could not find a way to produce a malformed transaction through the new code. Requesting changes for two follow-ups, plus a few smaller suggestions.
Should fix before merge
1. The advertised fallback is not tested end to end.
Test 12b mocks createEvmPaymentPayload to throw the SVM error text, which exercises the catch and the stderr line but not the scenario the PR describes. Nothing feeds a solana:* requirement with extra.feePayer === wallet into createPaymentSignatures alongside a valid second option and asserts that the second option yields. A regression that let the SVM throw escape the loop, or stopped ranking SVM options, would pass the suite.
Suggest: mock x402-svm.js with isSvmNetwork: () => true and a throwing createSvmPaymentPayload, add a good EVM option, and assert one result plus the stderr reason.
2. The root cause survives in createSvmPaymentPayload.
src/x402-svm.js:301 still rebuilds the wire layout with its own encodeCompactU16(2) + Buffer.alloc(64) + signature-at-slot-1. The new guard protects it only because buildUnsignedSvmTransaction throws first. The PR description names hardcoded slot counts as the root cause, then leaves one copy. Cheaper form: take txBase64 from buildUnsignedSvmTransaction and copy the signature into slot 1, the way src/trading.js:3068 already does, so one function owns the layout.
Nice to have
- Actionable error text. CLAUDE.md asks for a next step in error messages. Both new messages explain the cause but give the user nothing to do. Something like "another payment network will be tried; if none succeeds, pay on Base or report the option to Nansen" would cover it.
- Stale doc comment on
buildMessageV0(src/x402-svm.js:39) still says callers readnumRequiredSignaturesto size the slots. The PR quotes that claim as false but does not correct it. It should say the value is asserted against each caller's fixed layout. - The
numRequiredSignatures !== 2assertion is unreachable. OncefeePayerStr === walletAddressis refused, the only two signers are always distinct, so the header check can never fire and no test can exercise it. Belt and braces is a fair call, but consider one source of truth: either drop the header check, or make the header check the guard and give it the actionable message. - Test strictness. The existing test at
src/__tests__/x402-svm.test.js:144still assertsmessageBytes[1] >= 2while the new test asserts=== 2, and the new test'stxBytes[0]assertion duplicates the one at line 190. Tighten the old one totoBe(2)and drop the duplicate rather than adding a third test.
There was a problem hiding this comment.
Auto-approved
This PR was automatically approved because:
- Claude recommends approval
- Claude assessed this as a moderate effort change
- The effort level is within the auto-approval threshold of 2
- No high or critical issues were detected
If you have any concerns, please request a manual review.
…ack test - createSvmPaymentPayload writes its signature into slot 1 of the transaction from buildUnsignedSvmTransaction, so one function owns the wire layout. - The header signer-count check is the only guard; its message names a self-sponsored feePayer as the cause and says what happens next. - buildMessageV0's doc comment says callers assert numRequiredSignatures against their fixed slot count. - Test 12c feeds a self-sponsored Solana option (real x402-svm) next to a Base option and asserts the Solana option is skipped with its reason before the Base one is signed. - Added a slot-1 signature verification test for createSvmPaymentPayload, tightened the header assertion to toBe(2), and dropped the duplicate test.
There was a problem hiding this comment.
Auto-approved
This PR was automatically approved because:
- Claude recommends approval
- Claude assessed this as a moderate effort change
- The effort level is within the auto-approval threshold of 2
- No high or critical issues were detected
If you have any concerns, please request a manual review.
gulshngill
left a comment
There was a problem hiding this comment.
Thanks, all review points are addressed in 543321c: the end-to-end fallback test (12c) exercises the real SVM builder and fails against main, createSvmPaymentPayload now reuses the unsigned transaction layout, the signer-count check is the single guard with an actionable message, and the doc comment and test assertions are tightened.
Non-blocking nits, fine as a follow-up:
- A wallet without a Solana key (
exportWalletreturnssolana.address: undefined) that reaches an SVM option will now log a raw JS error on the new skip line instead of failing silently. A clear "no Solana key in wallet" skip would read better. - "pay on Base" could be "pay on another network", since X Layer (USDT0) is also supported.
Summary
buildMessageV0(src/x402-svm.js) computesnumRequiredSignaturesfrom the distinct signer accounts and returns it — its doc comment says it is "read back out of the header to size the signature-placeholder slots of the wrapping unsigned transaction (see callers)". Neither caller reads it:Both hardcode two slots on the assumption that
extra.feePayer(fully server-controlled) and the paying wallet are distinct. If a facilitator setsfeePayerto the payer's own address (self-sponsored model, or a buggy/malicious response), the account map has one signer, the header saysnumRequiredSignatures = 1, and the wire carries two 64-byte slots — a transaction Solana rejects at sanitization, surfacing as an opaque broadcast failure instead of a client-side error.Changes
src/x402-svm.jsbuildUnsignedSvmTransactionrefusesextra.feePayer === walletAddresswith an actionable error, so the fallback loop inx402.jsmoves on to the next payment option instead of building a doomed transaction. The client signature is placed at slot 1 by design, so a one-signer layout is not something this code can produce correctly.numRequiredSignatures === 2(belt and braces for the fixed slot layout).src/__tests__/x402-svm.test.js:feePayer === walletis refused by bothbuildUnsignedSvmTransactionandcreateSvmPaymentPayload(fails on currentmain); header signer count equals the compact-u16 slot count for a normal requirement.patch).Not touched by open #628 (spend caps / audit log) or #420 (X Layer).
Tests
npm testpassesnpm run lintpasses