Bind Solana swap execution to the persisted request intent - #512
Conversation
pr-reviewer Summary for #f5c4051✅ No issues found The code review completed successfully with no findings. Review effort: 4/5 (Complex) SummaryThis PR correctly closes the intent-binding gap for Solana swap signing. The implementation is well-structured and the security properties are sound. No actionable bugs found. Overall assessment: This is a solid security hardening PR. The three signing paths (local, Privy, WalletConnect) all have Token usage: 2,878 input, 10,233 output, 2,312,641 cache read, 68,071 cache write | Usage Guide New pushes are reviewed automatically with a 10-minute cooldown between reviews. To request a review at any time, comment |
|
Follow-up review findings:
|
Solana previously signed the aggregator's serialized transaction verbatim, with no validation against what the user actually requested. Wire the existing assertQuoteMatchesRequest/completeness checks (already used on EVM) into all three Solana signing paths (local, Privy, WalletConnect) so a compromised or buggy quote can't swap in a different token pair, inflate the input, or target a different signer. exactOut on Solana now also persists a derived spend ceiling so the check always has a bound, without requiring --max-input. Also fixes a false-positive this surfaced: some aggregators report native SOL as the System Program ID rather than the wrapped-SOL mint, which the new token-pair check would have rejected as a mismatch. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The auto-derived maxInputAmount for Solana exactOut was computed from the same quotes it was later checked against, so assertInputWithinMax could never reject anything — a compromised or buggy quote could inflate the input by 10x and the "cap" would just inflate along with it. Require an explicit --max-input on Solana too, matching EVM, so the ceiling is an independently-supplied value instead of the API checking itself. Also makes the assertInputWithinMax over-cap error chain-aware, since Solana now exercises it for real (no ERC-20 "approval" concept to reference in the message).
Adds a fail-closed null check for the Privy wallet address before the quote-binding assertions run, so the signer-binding check cannot be silently skipped when address resolution fails. Updates the surrounding comments for clarity, and rewords the changeset description to be more user-facing. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The description still said "optional on Solana" after the previous commit made it required on Solana too, matching EVM behaviour. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
assertInputWithinMax picked the EVM-worded (approval/native-value) over-cap message for a Solana swap whenever request.chain was persisted in non-lowercase form (e.g. `--chain Solana`), since request.chain stores the raw --chain input verbatim. Normalize the case before choosing the message. Wording-only; the throw itself was already correct. Add integration coverage proving assertQuoteMatchesRequest / assertCompleteSolanaRequestIntent are actually wired into all three Solana signing paths (local, Privy, WalletConnect): each test poisons a different quote field (output token, inflated input, swapped signer) and asserts the execute path refuses before signing. Mutation-verified as non-vacuous. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…signer, dedupe tests Comments at the Privy and local Solana signing paths implied the intent-binding checks validate the serialized transaction itself; they only validate persisted request/quote metadata. Reworded to say so explicitly. The local wallet signing branch resolved solanaWalletAddress without checking it, unlike the Privy path's existing fail-closed guard — an unresolved address would silently skip the signer-binding comparison. Added the matching check. Consolidated the "Solana execute binds to persisted request intent" and "Solana intent binding (adversarial)" describe blocks: several cases (local/WalletConnect signer-swap) were tested near-identically in both. Merged the distinct cases (local output-token mismatch, Privy inflated-input) into the adversarial suite and removed the duplicate block. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
7b59fac to
f5c4051
Compare
@gulshngill Thanks addressed your findings. Rebased from the latest main as well. In terms of scope, this PR only addresses the metadata binding and not the transaction instructions. Follow up PRs will address this. |
2 participants
Summary
assertQuoteMatchesRequest, a newassertCompleteSolanaRequestIntent) into all three Solana signing paths (local, Privy, WalletConnect), closing the gap where Solana previously signed the aggregator's transaction verbatim with no validation. This is the Solana counterpart to PR Harden EVM swap signing: scope approvals and validate target #494's EVM hardening.--swap-mode exactOutnow requires--max-inputon Solana too (previously EVM-only). A cap auto-derived from the API's own quote response can't independently guard against an inflated one — an explicit, user-supplied ceiling is required on every chain now, matching EVM.Known follow-up (not in scope here)
While doing live e2e testing, found a pre-existing bug on
main, unrelated to this diff: Relay-sourced Solana bridge quotes return raw, uncompiled instructions ({instructions, addressLookupTableAddresses}) rather than a ready-to-sign transaction, and the signing code has no branch for that shape — crashes withALL_QUOTES_FAILED. Filed separately as API-353; confirmed viagit diff origin/mainthat the affected code is untouched by this PR.Two smaller/cosmetic review findings also left as follow-up (not correctness bugs): duplicated
assertCompleteEvmRequestIntent/assertCompleteSolanaRequestIntent(copy-paste instead of a shared helper), and duplicated native-SOL-mint literals acrosstrading.js/trade-validation.js(to avoid a circular import).Test plan
npm test— 2105 passed, 2 skippednpm run lint— cleanbridge-testwallet): SOL ↔ USDC same-chain round-trip via the local-wallet signing path this PR changes — both legs succeeded, net cost ~143,584 lamports (fees/slippage only)maxInputAmountcap), see commit c1b8ffb🤖 Generated with Claude Code