Sign destination-phase EVM transactions exactly once - #1303
Merged
Conversation
Since the morpho flow expanded the direct EVM signing group to Arbitrum, Avalanche, BSC and Ethereum, destination-phase transactions on those networks matched both signing groups and were signed and returned twice; the alreadySigned guard could never match because signing replaces txData before comparison. Derive both groups from one network list so they stay disjoint, and drop the dead guard.
✅ Deploy Preview for vortex-sandbox ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for vrtx-dashboard ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for vortexfi ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
There was a problem hiding this comment.
Pull request overview
Prevents duplicate signing of destination-phase transactions on directly signed EVM networks.
Changes:
- Centralizes directly signed EVM networks and transaction grouping.
- Removes the ineffective duplicate guard.
- Adds regression and cross-network grouping tests.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
packages/shared/src/helpers/signUnsigned.ts |
Makes EVM and destination signing groups disjoint. |
packages/shared/src/helpers/signUnsigned.test.ts |
Tests destination-phase grouping and overlap prevention. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Note
This branch now includes #1302 (both PRs add
signUnsigned.test.ts, so they are stacked to avoid an add/add conflict). Merge #1302 first — this diff then collapses to just the grouping change.Summary
Since 5515a7a ("adjust morpho flow from non-base networks") expanded the direct EVM signing group to Arbitrum, Avalanche, BSC, and Ethereum, destination-phase transactions (
destinationTransfer,backupSquidRouterApprove,backupSquidRouterSwap,backupApprove) on those networks matched both signing groups insignUnsignedTransactionsand were signed and returned twice. ThealreadySignedguard could never catch this: it compares the unsignedtxDataobject against the already-replaced signed hex string, so it never matches anything.Impact: duplicate identical presigned entries in ramp state for onramps to those networks (API validation tolerates them — subset matching, no exact counts), violating the one-signature-per-transaction model. No fund-loss path identified.
Changes
groupUnsignedTxsForSigning, whose destination group excludes exactly that list — the groups are disjoint by construction and can't drift apart again.alreadySignedguard.destinationTransferlands only in the EVM group; non-listed networks stay in the destination group; a property test sweeps every network × destination phase asserting no tx lands in both groups. Verified 2/3 fail against the old filter. Tested via the pure grouping function because viem'ssignTransactionalways fetcheseth_chainId, so the real signing path would hit live RPCs.Known pre-existing quirk, deliberately untouched: a destination-phase tx on Moonbeam would still match both the Moonbeam group and the destination group — that overlap predates 5515a7a; whether Moonbeam can be a ramp destination for these phases needs a product-level answer before changing it.
Spec check:
docs/security-spec/03-ramp-engine/transaction-validation.mddocuments presigned verification, not client-side grouping; this restores documented behavior, so no spec change.Test
bun testinpackages/shared(89 pass)bun lint,tsc --noEmit,bun build:shared