Repository navigation
fix(sdk-core): carry TSS intent and strict policy to verifyTransaction - #9972
ralph-bitgo[bot] wants to merge 1 commit into
Conversation
|
|
dpkjnr
left a comment
dpkjnr
left a comment
There was a problem hiding this comment.
Stack review: request changes
Reviewed #9972 → #9973 → #9975, all 20 changed files, with two parallel reviewers. This is a consolidated review on the stack root; the three blocking findings below are in #9973, modules/abstract-eth/src/tssVerification.ts, not standalone defects in the additive API/type plumbing in #9972.
1. [P1] Server calldata overrides a local no-calldata intent — lines 240–245
buildEvmTssIntentContext fills missing local recipient.data from the stored server intent. Both ECDSA signing paths in #9972 now supply that intent alongside local txParams, so the merge also runs on direct sends.
A caller declaring { address: tokenContract, amount: '0' } with no calldata can receive matching outer to/value plus approve(attacker, MAX) in both transaction bytes and stored intent. The merge selects verifyExactCall, which returns zero violations, bypassing the new strict-mode violation check. Without the server-data merge, the same approval produces a violation. This concerns overwriting local intent, not the separate problem of authenticating a stored pending-approval intent.
Requested change: when local recipients exist, preserve their calldata semantics. Absent local data must not authorize server-provided calldata.
2. [P1] Server token metadata overrides the locally selected asset — lines 244–252
The merge copies server tokenContractAddress into a local recipient even when its local tokenName already identifies token A. tokenContractForRecipient then prefers that copied address. Top-level tokenName also prefers the server intent over txParams.
A local USDC transfer accepts identical recipient/amount calldata sent to substituted contract B: zero violations. Without the injected metadata, the token-contract mismatch is flagged. The intended recipient and amount do not establish that the intended asset is being transferred.
Requested change: resolve explicit local token identity first; server metadata must not supersede it.
3. [P1] Native and token payment shapes are interchangeable — lines 368–369, 519–548
Payment handlers choose native/token shapes from decoded calldata rather than binding them to the intended asset. An unresolved token contract skips the contract check.
The following mismatches returned zero violations:
- Native payment intent → ERC-20 transfer on an arbitrary contract.
- Token batch intent → native
batch()carrying the summed amount. - Native batch intent →
batchTransferFrom()using an arbitrary token. - Batch recipients declaring
tokenAddress→ transfer of a different token, because that field is ignored here.
Requested change: constrain selectors to the intended asset, resolve its contract consistently, and report unresolved token identity instead of silently skipping validation.
Verification
Executed the added verifier and batch decoders extracted from the PR diffs against real published dependencies in an isolated throwaway smoke harness. The bypass cases above returned zero violations; calldata/token-substitution negative controls returned violations. Honest token-transfer and native-batch controls returned zero violations. Full SDK signing and repository test suites were not run. The new strict-mode gate throws only when violations are present, so these empty results leave it unenforced for the described inputs. Add regression coverage in #9975 for these local-vs-server precedence and wrong-asset cases.
Release ordering
Keep #9972 and #9973 in the same release: #9972 relaxes the DeFi recipient guard before #9973 supplies the canonical handler mapping. Do not publish part 1 independently of part 2.
fac3fee to
c773059
Compare
Findings 1–3 are fixed in #10114 (part 4 of this stack): caller-declared intent is the only source when the caller declares type/recipients, token identity resolves locally and fails closed when unresolved or conflicting, and payment/batch calldata is bound to the declared asset. Regression tests for each are in #10114.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
What: add an optional VerifyTransactionOptions.intent (the persisted PopulatedIntent), the TssVerificationWarning reporting types, and the BitGoBase strictTssVerification / onVerificationWarning client options. The ecdsa and ecdsaMPCv2 signing paths now pass txRequest.intent and the chain name into verifyTransaction / resolveEffectiveTxParams, and the kebab-case DeFi intent spellings (defi-approve, defi-deposit, defi-withdraw, defi-redeem) join NO_RECIPIENT_TX_TYPES so stored defi-* intents reach the verifier instead of failing the missing-recipients guard first. Why: the pending-approval signing path only passes resolveEffectiveTxParams output, so fail-closed EVM TSS verification cannot see the stored intent fields it must compare against, and the kebab-case DeFi throw rejects legitimate DeFi requests before any verification runs (TOB-BITGOEDMPC-2 / INFOSEC-671). Ticket: CECHO-2460 Session-Id: 22db0dc1-cf47-40e2-b717-1bb226a6304c Task-Id: a8a26387-a332-4be1-aab5-f39deddfc502
c773059 to
e95e7a8
Compare
|
@claude review |
|
Claude finished @dpkjnr's task in 35s —— View job Review of #9972 (sdk-core intent and strict-policy plumbing)I read the full diff and checked Findings
VerdictThe plumbing is sound. I'd ask for: the |
| 'defiApprove', | ||
| 'defi-approve', | ||
| 'defiDeposit', | ||
| 'defi-deposit', | ||
| 'defiWithdraw', | ||
| 'defi-withdraw', | ||
| // defi-redeem has no EVM verifier handler and is flagged as a violation there; | ||
| // listed here so the pending-approval path reaches the verifier instead of | ||
| // failing earlier on the missing-recipients guard. | ||
| 'defi-redeem', | ||
| 'defiRedeem', |
There was a problem hiding this comment.
🔴 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/defiRedeem entries pass both no-recipient guards. The EVM DeFi checks in abstractEthLikeNewCoins.ts:3659 only match camelCase defiApprove/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/data the 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.
| assert.strictEqual( | ||
| verifyCallArgs.intent, | ||
| (txRequest as any).intent, |
There was a problem hiding this comment.
🟡 Nit (optional): the new assertion adds an avoidable any cast, (txRequest as any).intent, which goes against the CLAUDE.md rule to avoid any. Fix: use txRequest.intent, since txRequest is already typed as TxRequest and has an intent field.
Why this was flagged
There is no runtime impact. At ecdsaMPCv2.ts:811, txRequest is declared as unknown as TxRequest, and TxRequest declares intent. That makes the as any in the new assertion unnecessary, and it goes against the root CLAUDE.md TypeScript rule to avoid the any type.
Verification: Severity: nit. There is no runtime impact. The diff adds (txRequest as any).intent in the new assertion at modules/sdk-core/test/unit/bitgo/utils/tss/ecdsa/ecdsaMPCv2.ts:837. TxRequest in modules/sdk-core/src/bitgo/utils/tss/baseTypes.ts:621 declares intent: unknown; at line 630. Since assert.strictEqual takes unknown, writing txRequest.intent type-checks without the cast. It goes against the root CLAUDE.md line 88.
What
VerifyTransactionOptions.intent(the persistedPopulatedIntent),TssVerificationMismatch/TssVerificationWarningreporting types, and client-sidestrictTssVerification/onVerificationWarningoptions onBitGoBase/BitGoAPI(set only from local options, never read from server data).ecdsa/ecdsaMPCv2signing paths now passtxRequest.intentandthis.baseCoin.getChain()intoverifyTransaction/resolveEffectiveTxParams, so the pending-approval path verifies against the intent the user approved (chain arg also fixes token-name resolution fromamount.symbol, matching the EdDSA path).defi-approve,defi-deposit,defi-withdraw,defi-redeem) inNO_RECIPIENT_TX_TYPES. This only waives the missing-recipients guard — the DeFi calldata checks still validate (part 2 of this stack).Why
verifyTssTransactionmust compare the decodedto/value/dataagainst user intent before the key share signs. On the pending-approval path the intent lives only intxRequest.intent, which the verifier could not see.Test plan
NO_RECIPIENT_TX_TYPEScontract updated (new DeFi spellings;paymentstill excluded), kebab DeFi no-recipients guard test, and the MPCv2signRequestBaseregression test now pins that the stored intent reachesverifyTransaction.BitGo SDKCI green on the rebased head.Stack
This PR is part 1 of 4; review and merge in order:
master) ← you are hereCECHO-2460-evm-tss-fail-closed-handlers-pt1)CECHO-2460-evm-tss-fail-closed-handlers-pt2)CECHO-2460-evm-tss-fail-closed-handlers-pt3)Release #9972 and #9973 together: this PR relaxes the DeFi recipient guard that #9973's handlers rely on.
Ticket: CECHO-2460