Repository navigation
fix(sdk-core): carry TSS intent and strict policy to verifyTransaction #9972
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -636,6 +636,7 @@ describe('ECDSA MPC v2', async () => { | |
| verifyTransaction: sinon.stub().resolves(true), | ||
| getMPCAlgorithm: sinon.stub().returns('ecdsa'), | ||
| getConfig: sinon.stub().returns({ family: 'flrp' }), | ||
| getChain: sinon.stub().returns('flrp'), | ||
| isSignablePreHashed: (unsignedTx: { serializedTxHex?: string }) => | ||
| unsignedTx.serializedTxHex?.startsWith('0000') ?? false, | ||
| } as unknown as IBaseCoin; | ||
|
|
@@ -707,6 +708,7 @@ describe('ECDSA MPC v2', async () => { | |
| verifyTransaction: sinon.stub().resolves(true), | ||
| getMPCAlgorithm: sinon.stub().returns('ecdsa'), | ||
| getConfig: sinon.stub().returns({ family: 'flr' }), | ||
| getChain: sinon.stub().returns('flr'), | ||
| } as unknown as IBaseCoin; | ||
|
|
||
| const mockWallet = { | ||
|
|
@@ -781,6 +783,7 @@ describe('ECDSA MPC v2', async () => { | |
| verifyTransaction: verifyTransactionSpy, | ||
| getMPCAlgorithm: sinon.stub().returns('ecdsa'), | ||
| getConfig: sinon.stub().returns({ family: 'hteth' }), | ||
| getChain: sinon.stub().returns('hteth'), | ||
| } as unknown as IBaseCoin; | ||
|
|
||
| const mockWallet = { | ||
|
|
@@ -827,6 +830,13 @@ describe('ECDSA MPC v2', async () => { | |
| const verifyCallArgs = verifyTransactionSpy.firstCall.args[0]; | ||
| assert.strictEqual(verifyCallArgs.txParams.type, intentType); | ||
| assert.strictEqual(verifyCallArgs.txParams.recipients, undefined); | ||
| // The stored intent must reach the verifier so fail-closed EVM checks can compare | ||
| // decoded to / value / data against it on the pending-approval path. | ||
| assert.strictEqual( | ||
| verifyCallArgs.intent, | ||
| (txRequest as any).intent, | ||
|
Comment on lines
+835
to
+837
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Nit (optional): the new assertion adds an avoidable Why this was flaggedThere is no runtime impact. At ecdsaMPCv2.ts:811, Verification: Severity: nit. There is no runtime impact. The diff adds |
||
| 'stored intent must reach verifyTransaction' | ||
| ); | ||
| }); | ||
| }); | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 Security: with only this PR merged, stored DeFi intents with no txParams get signed with no calldata check at all, where the base branch rejected them. The new
defi-approve/defi-deposit/defi-withdraw/defi-redeem/defiRedeementries pass both no-recipient guards. The EVM DeFi checks in abstractEthLikeNewCoins.ts:3659 only match camelCasedefiApprove/defiDeposit/defiWithdraw. Fix: keep these spellings out of NO_RECIPIENT_TX_TYPES until the verifier rejects or checks every one of them, or ship that verifier change in this PR. The comments at recipientUtils.ts:35 and :43 describe checks that do not exist on master. [also at: modules/sdk-coin-evm/src/evmCoin.ts:113 - Security: TSS wallets on non-EIP-1559 EVM chains would sign pending-approval DeFi transactions without any calldata check.]Why this was flagged
The trigger is the pending-approval path: pendingApproval.approve() -> recreateTxRequest() -> signTxRequest() with txParams undefined, on a stored intent whose intentType is 'defi-deposit' (the spelling wallet.ts:5004 and defiVault.ts:144 write). resolveEffectiveTxParams (recipientUtils.ts:170-175) sets txParams.type = 'defi-deposit'. The guard at recipientUtils.ts:216 no longer throws. In verifyTssTransaction, the guard at abstractEthLikeNewCoins.ts:3583 (and evmCoin.ts:113) passes because NO_RECIPIENT_TX_TYPES now has the kebab spelling. The DeFi block at abstractEthLikeNewCoins.ts:3659 only checks ['defiApprove','defiDeposit','defiWithdraw'], so it is skipped, and the key share signs whatever
to/value/datathe server built. 'defi-redeem' and 'defiRedeem' have no handler at all, so the comment's claim that they are "flagged as a violation there" is false on master. On the base branch, resolveEffectiveTxParams threw InvalidTransactionError before signing. The PR says the verifier lands in a separate part 2 (#9973), but nothing in this change enforces that the two ship together.Verification: On the base, the guard at recipientUtils.ts:214-222 threw "Recipient details are required..." because the kebab spellings were not in the set. This diff adds them, so it no longer throws. The DeFi calldata block at abstractEthLikeNewCoins.ts:3659 only matches
['defiApprove', 'defiDeposit', 'defiWithdraw']. With this PR they are signed with no check of the server-built calldata.