Skip to content

Swap: a provider's transaction must match the quote before it is signed - #1739

Closed
0xh3rman wants to merge 4 commits into
mainfrom
hm/swap-quote-binding
Closed

0xh3rman wants to merge 4 commits into
mainfrom
hm/swap-quote-binding

Conversation

@0xh3rman

Copy link
Copy Markdown
Collaborator

Swap transactions that a provider writes are now checked on the device against what the quote says, and stopped before signing if they differ. Each commit stands alone.

  1. Chainflip: the destination chain, asset and address are encoded on the device and must match the broker's EVM vault call, Solana instruction and Tron note (chainflip/destination.rs).
  2. DeDust: the vault address is derived from the factory's deploy code and the asset swapped, so the TON node is no longer asked for it.
  3. Relay, OKX and Near Intents: the coin sent must be exactly the quoted amount for a native coin and zero for a token. Celo's native coin is pulled as a token, so it sends zero. Near Intents' deposit amount must match the quote for every asset.
  4. Chainflip: the refund address, retry window and minimum price must match the broker's cfParameters on all three source networks (chainflip/refund.rs).

Behavior to review

  • A cfParameters version other than 1 stops Chainflip vault swaps at Confirm until the app is updated, because a layout the app can't read can't be checked. New fields after the minimum price don't affect it.
  • Relay's check runs at quote time, so a mismatch removes the quote. The other checks run when the quote data is built.

Verified

  • cargo test -p swapper --lib passes on each commit (567, 569, 573, 575); cargo fmt is clean on each, and clippy on swapper and gem_ton at the tip.
  • A live sweep through each provider's real get_quote and get_quote_data, on this branch against unmodified main: 99 routes pass on both, and 17 fail on both at the quote step.
  • The one difference is Relay from native Tempo, which main quotes and this branch refuses. Tempo rejects any transaction carrying a native value, and the app doesn't open that asset.
  • The live broker encoding was checked for 8 routes at 3 prices.

Not covered

  • Native TRX through Chainflip returned no quote on both main and this branch, so it didn't run end to end; its broker encoding was checked.
  • The sweep ran before the final tidy, which only moved code and reordered the same checks.
  • The broker fee inside cfParameters is not checked.
  • DeDust pool and jetton wallet addresses still come from the node.

… Confirm

The broker encodes the vault swap, and the app checked its vault and amount but not where the output goes. The destination chain, asset and address the quote was made for are now encoded on the device and must match the broker's EVM vault call, Solana instruction and Tron note, or the swap is stopped before it is signed.
The vault address came from the factory's get_vault_address on a TON node. It is now derived from the factory's deploy code and the asset swapped, the way the factory deploys it, for TON and every jetton, so the node is no longer asked.
The coin sent with the transaction came from the provider's response. It must now be the quoted amount for a native coin and zero for a token, or the swap is stopped before it is signed; Celo's native coin is pulled as a token, so it sends zero. Near Intents' deposit amount must match the quoted amount for every asset.
…ice on Confirm

The broker encodes the refund parameters into the vault swap, and the app checked what it sent but not what came back. The refund address, retry window and minimum price are now encoded on the device and must match the broker's EVM vault call, Solana instruction and Tron note, or the swap is stopped before it is signed. Solana and Tron are read at the exact position of those parameters, and a layout the app doesn't know stops the swap too.
@0xh3rman 0xh3rman self-assigned this Oct 30, 2026
@0xh3rman
0xh3rman marked this pull request as ready for review October 30, 2026 13:02

@gemcoder21 gemcoder21 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it looks like a code smell, it does not give enough protection that's worth adding those checks

@0xh3rman

Copy link
Copy Markdown
Collaborator Author

Split into #1742 and #1743. The Relay, OKX and Near Intents value checks are dropped.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants