fix(x402): select payment asset instead of hardcoding accepts[0] (#613)#622
Conversation
detectTokenType returned 'STX' for any unrecognized asset. Combined with the interceptor taking the first accepts[] entry, a USDCx-denominated 402 (asset SP120...usdcx, amount 29000000) selected USDCx, classified it as STX, and fell into the makeSTXTokenTransfer branch — signing a real 29 STX native transfer for an invoice the endpoint would never credit. The same fallback made the probe render "29 STX" while payment.asset said USDCx. - detectTokenType identifies STX positively and returns 'unsupported' for anything that is not STX or sBTC, so unknown assets are refused not guessed - selectPaymentOption picks the requested asset, else the first payable entry in the server's order; errors list the accepted assets rather than substituting a different one - Add an `asset` param to probe_x402_endpoint and execute_x402_endpoint, and echo it through callWith so executing a quote pays what was quoted - Probe returns accepts[] with a payable flag per asset, and the message text is derived from the same asset the payment block reports - formatPaymentAmount renders unknown tokens in base units with their symbol instead of scaling by 1e6 and labelling them STX - checkSufficientBalance refuses unsupported assets instead of validating STX Selection is deliberately not balance-aware: picking by wallet contents would let a manifest reorder change which asset leaves the wallet. Two existing tests asserted the unknown-asset -> STX fallback as intended behavior and are updated; the substring-guard test keeps its intent.
|
Correction to the severity framing in the PR description above, verified after merge. The description says the pre-fix code "signs and broadcasts 29 real STX". It does not. The per-payment cap check runs before signing, and because the USDCx amount ( So no funds were at risk on arc0btc, and there is no evidence of any loss having occurred. The wrong-asset routing was still real, but the exposure is narrower than stated: an unknown token whose base-unit amount is below the cap (≤ The fix and its justification are unchanged: an asset the client cannot sign must be refused, not silently rerouted onto the STX rail. Only the blast-radius claim was overstated. |
1 participant
Fixes #613.
The reported bugs are real, and there is a third one underneath them
detectTokenTypereturned'STX'for any unrecognized asset. The interceptor then took the firstaccepts[]entry and branched on that type. On arc0btc's live 402 header:selection took USDCx →
detectTokenTypeclassified it'STX'→ theelsebranch ranmakeSTXTokenTransfer({ recipient: payTo, amount: 29000000n }).That signs and broadcasts 29 real STX as a native transfer to satisfy an invoice denominated in 29 USDCx. Wrong asset, funds gone, endpoint never credits it. The "29 STX" display bug in the issue is the same root cause surfacing harmlessly; this is it surfacing expensively.
Changes
detectTokenTypeidentifies STX positively (bareSTX, or CAIP-19native/slip44:5773) and returns'unsupported'for anything that is not STX or sBTC. Unknown assets are refused, never guessed into a transfer.selectPaymentOptionreplacesaccepts[0]/find(startsWith("stacks:")). Honors a requested asset; otherwise takes the first entry this client can actually sign, in the server's own order. On a mismatch it throws listing the accepted assets — it never substitutes a different asset than the one asked for.assetparameter onprobe_x402_endpointandexecute_x402_endpoint, matching on symbol or full contract id, case-insensitively.callWithechoes it so executing a quote pays what was quoted.accepts[]with apayableflag per asset, and the message text is derived from the same asset thepaymentblock reports — the two can no longer disagree.formatPaymentAmountrenders unknown tokens as29000000 USDCx (base units). Decimals are unknown for third-party tokens, so it does not invent a scaled figure.checkSufficientBalancerefuses unsupported assets instead of falling through to the STX balance check.Selection is deliberately not balance-aware, despite the issue suggesting it. Auto-picking by wallet contents would let a server reordering its manifest change which asset leaves the wallet without the caller ever naming it. Explicit
asset+ first-payable default is predictable; the probe now surfaces every option so the caller can choose.Verification
Live against arc0btc (probe only, no payment):
asset45385— the sBTC-only wallet case from the bounty, previously blockedsBTC45385STX159911365USDCxDOGEdoes not accept "DOGE", lists accepted assets542 tests pass. New tests use the real arc0btc
accepts[](USDCx first, so the ordering that caused this is exercised). Confirmed the new tests fail against the pre-fix source — 11 failures, includingexpected '29 STX' not to match /STX/.Behavior change
Endpoints offering only unsupported assets now fail with a clear error where they previously "succeeded" by mis-sending STX. That is the point of the fix, but it is a visible change for anyone who was unknowingly relying on the broken path.