Repository navigation
Conversation
|
|
verifyTssTransaction trusted the server-declared txParams.type and only decoded two calldata shapes under transfer. A compromised server could label approve(attacker, MAX) or any contract call as a payment, or redirect a payment on the pending-approval path, and the user key share would sign it (TOB-BITGOEDMPC-2 / INFOSEC-671). The ecdsa and ecdsaMPCv2 signing paths now pass the stored txRequest.intent, the caller's untouched txParams (callerTxParams) and the chain name into verifyTransaction. When the caller declares a type or recipients, only its own params count. The stored intent is used only when the caller declared nothing (pending approval), so server data never fills gaps in a caller's intent. The asset comes from local identity only; unresolvable or conflicting identity is a finding. The verifier decodes the bytes once and runs small checks picked by the intent type, each accepting a closed set of shapes: native send, ERC-20 or ERC-7984 transfer, exact dApp call, batcher batch / batchTransferFrom, tokenApproval to the batcher for MAX, fillNonce, consolidation to the base address, and the DeFi calldata checks. contractCall and customTx throw. A token consolidation must carry no native value, which closes a path where transfer(baseAddress, 0) sent to any contract moved the native balance there. Checks return findings and one policy step decides warn or throw. Fatal findings, the checks that threw before, still throw with the same messages. Every other finding goes to console.warn and the new onVerificationWarning callback, or throws when the client sets the new client-side strictTssVerification BitGoAPI option, which is never read from server data. Stored kebab-case DeFi intents (defi-approve, defi-deposit, defi-withdraw) now pass the missing-recipients guard so their calldata is checked. defi-redeem has no handler and stays rejected. EvmCoin.verifyLegacyTssTransaction, which returned true without decoding, is removed so kavaevm and prividiumeth run the parent verifier. The verifier helpers stay internal to abstract-eth. Tests cover the audit cases on kavaevm, hbarevm and tbsc (batcher), and an end-to-end case runs the real verifier inside EcdsaUtils.signTxRequest to show rejected bytes never reach the signing round. Ticket: CECHO-2460 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
fd8d18d to
90aceaa
Compare
![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.
|
@claude review |
Review of #10121: fail-closed EVM TSS verificationI read Findings
Smaller points
Not verified
View job run | branch |
| if (this.bitgo?.strictTssVerification) { | ||
| await raise(findings[0]); | ||
| } |
There was a problem hiding this comment.
🟡 (optional) Clients that turn on strictTssVerification can no longer sign accelerations, staking (delegate, stake, claim, …), wrap/unwrap, wrap-native, enableToken or bridgeFunds on EVM TSS. Every intent type without a handler gets an unhandled finding (tssVerification.ts:1107). The strict policy throws on the first finding of any severity. Fix: in strict mode, throw only for new findings on types that have a handler, and keep unhandled as a warning until those types get handlers. Or add handlers first so strict mode does not block legitimate operations such as unsticking a tx.
Why this was flagged
A client builds BitGoAPI with strictTssVerification: true and accelerates a stuck EVM TSS transaction (type acceleration), or signs a BSC/CELO staking intent through the same signTxRequest path. canonicalizeEvmTssIntentType returns acceleration or delegate. evaluateEvmTssIntent falls through to default and returns unhandledTypeViolation with severity unhandled (tssVerification.ts:1106-1107). applyTssVerificationPolicy at abstractEthLikeNewCoins.ts:3871 calls raise(findings[0]) whenever strict mode is on, whatever the severity, so signing fails with "no fail-closed verifier for intent type 'acceleration'". On the base branch these intents verified and signed. The option is opt-in, but its stated meaning is that mismatches become failures. A client who turns it on gets every unhandled operation type refused, even when the bytes are correct.
Verification: Intent types without a handler hit default: return [unhandledTypeViolation(ctx, tx)] (tssVerification.ts:1106-1107). applyTssVerificationPolicy (abstractEthLikeNewCoins.ts:3871-3872) does if (this.bitgo?.strictTssVerification) { await raise(findings[0]); }, which throws for any severity. Strict mode is new and opt-in, so default clients only get a console.warn and nothing breaks compared with base.
pranishnepal
left a comment
pranishnepal
left a comment
There was a problem hiding this comment.
wallets own changes look fine to me
Verify legacy transactions using serialized bytes and bind them to the signing payload. Reject caller-declared payments without recipients rather than falling back to server-provided intent. Add regression coverage and update package changelogs. Ticket: CECHO-2460 Co-authored-by: Ralph CLI <ralph-cli@bitgo.com> Ralph: 18.13.2-bitgo.6 openrouter/deepseek/deepseek-v4.1-flash-1017@default:high+smol:high+task:high openrouter/anthropic/claude-opus-5.5@slow:high+plan:xhigh openrouter/openai/gpt-6.1-terra@vision:low+advisor:medium openrouter/z-ai/glm-5.3-flash@commit:high+memory:low openrouter/inclusionai/ling-3.1-flash@tiny:low openrouter/moonshotai/kimi-k3.1-pro@designer:high openrouter/google/gemini-3-pro-image@image web/exa@web openrouter/microsoft/mai-transcribe-2@dictation openrouter/typesafe/jev-1.14@judge
|
@claude review again |
|
Claude finished @dpkjnr's task in 38s —— View job Re-review of #10121 (head
|
There was a problem hiding this comment.
Findings marked 🟡 are optional suggestions and need no follow-up push.
Still open from earlier reviews (3):
- 🔴
modules/abstract-eth/src/tssVerification.ts:383—Direct-send callers who name their token through recipients[].tokenData will have their ERC-20 sends flagged as native-… - 🔴
modules/abstract-eth/src/tssVerification.ts:1013—Receive-address fillNonce transactions will warn on every send and throw under strictTssVerification, even when the byt… - Also unresolved: 1 minor or pre-existing.
If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.
| @@ -1439,14 +1439,6 @@ describe('ERC20 Bulk Transaction Feature', () => { | |||
| }); | |||
|
|
|||
| describe('TSS Verify Use Serialized Tx Hex Feature', () => { | |||
There was a problem hiding this comment.
🟡 nit (optional): Nothing now checks the TSS_VERIFY_USE_SERIALIZED_TX_HEX feature on base coins, so dropping it from a chain would break that chain's TSS signing without any test failing. The deleted test covered bsc, tbsc, xdc, txdc, icp and ticp. Nothing replaced it for the four chains this change relies on. Fix: restore the base-coin assertion and add kavaevm, tkavaevm, prividiumeth and tprividiumeth to its list.
Why this was flagged
The diff deletes the test 'should have TSS_VERIFY_USE_SERIALIZED_TX_HEX for coins whose signableHex is not parseable' (coins.ts:1442-1448 on the base). The only test left checks tokens: bsc:busd, xdc:usdc and txdc:tmt. With the EvmCoin.verifyLegacyTssTransaction bypass removed, the kavaevm/prividiumeth entries at allCoinsAndTokens.ts:2173, 2193, 2883 and 2901 now carry this feature, and signing on those chains depends on it. Without it, decodeTssTransaction parses the EIP-155 signableHex and throws on every payment. If a later statics edit drops the feature from any of these base coins, or from bsc/xdc/icp, CI stays green while TSS sends on that chain fail. On the base branch the deleted test would have caught this for bsc/xdc/icp.
Verification: Severity: nit. Trigger: a later change drops TSS_VERIFY_USE_SERIALIZED_TX_HEX from bsc, tbsc, xdc, txdc, icp or ticp. The diff deletes the test that checked those six coins from modules/statics/test/unit/coins.ts. All that remains (coins.ts:1442-1445) is the token test. If the feature were dropped, ecdsa.ts:834 would verify the unparseable signableHex.
Honor caller token metadata and receive-address nonce fills, and normalize calldata before comparing intent. Reject undecodable payloads, missing token recipients, and stored DeFi intents without a trusted vault binding. Keep warn-by-default rollout and strict opt-in enforcement. Isolate warning callback failures and bound console diagnostics. Ticket: CECHO-2460 Co-authored-by: Ralph CLI <ralph-cli@bitgo.com> Ralph: 18.13.2-bitgo.6 openrouter/deepseek/deepseek-v4.1-flash-1017@default:high+smol:high+task:high openrouter/anthropic/claude-opus-5.5@slow:high+plan:xhigh openrouter/openai/gpt-6.1-terra@vision:low+advisor:medium openrouter/z-ai/glm-5.3-flash@commit:high+memory:low openrouter/inclusionai/ling-3.1-flash@tiny:low openrouter/moonshotai/kimi-k3.1-pro@designer:high openrouter/google/gemini-3-pro-image@image web/exa@web openrouter/microsoft/mai-transcribe-2@dictation openrouter/typesafe/jev-1.14@judge
|
@claude review |
|
Claude finished @dpkjnr's task in 27s —— View job Review of #10121 (head
|
Verify FLR C-to-P atomic exports without routing through the EVM decoder, checking chain direction, recipient and amount bounds. Validate ERC-721 and ERC-1155 transfers against declared token metadata on direct-send and pending-approval paths. Add regression coverage and document verification boundaries. Ticket: CECHO-2460 Co-authored-by: Ralph CLI <ralph-cli@bitgo.com> Ralph: 18.13.2-bitgo.6 openrouter/deepseek/deepseek-v4.1-flash-1017@default:high+smol:high+task:high openrouter/anthropic/claude-opus-5.5@slow:high+plan:xhigh openrouter/openai/gpt-6.1-terra@vision:low+advisor:medium openrouter/z-ai/glm-5.3-flash@commit:high+memory:low openrouter/inclusionai/ling-3.1-flash@tiny:low openrouter/moonshotai/kimi-k3.1-pro@designer:high openrouter/google/gemini-3-pro-image@image web/exa@web openrouter/microsoft/mai-transcribe-2@dictation openrouter/typesafe/jev-1.14@judge
|
@claude review |
|
Claude finished @dpkjnr's task in 43s —— View job Review of #10121 (head
|
| if (!amount.isFinite() || amount.isNegative() || !output.isFinite() || output.isNegative()) { | ||
| throw new Error(`atomic export amount ${declaredAmount} or output amount ${outputAmount} is not a valid amount`); | ||
| } | ||
| const expectedOutput = amount.div(1e9); | ||
| const minOutput = expectedOutput.plus(flrpNetwork.minImportToPFee); | ||
| const maxOutput = expectedOutput.plus(flrpNetwork.maxImportFee); | ||
| if (output.isLessThan(minOutput) || output.isGreaterThan(maxOutput)) { | ||
| throw new Error( | ||
| `atomic export output amount ${outputAmount} is outside the expected range [${minOutput.toFixed()}, ${maxOutput.toFixed()}] for the declared amount ${declaredAmount}` | ||
| ); | ||
| } |
There was a problem hiding this comment.
🔴 FLR TSS exports whose declared wei amount is not a whole number of nanoFLR will be rejected in every mode; on the base branch they signed. assertAtomicExportAmount divides by 1e9 without rounding, so minOutput keeps a fraction. The atomic output is a whole nanoFLR count, so unless WP rounds up, output.isLessThan(minOutput) throws. Fix: round expectedOutput to whole nanoFLR before adding the fees, using the same rounding as the export builder (for example integerValue(BigNumber.ROUND_FLOOR)). The hop check validateHopPrebuild already rounds with toFixed(0).
Why this was flagged
An FLR TSS wallet calls prebuildTransaction({ type: 'Export', recipients: [{ address: pAddr, amount: '1234567890123456789' }] }). That amount is in wei and is not a multiple of 1e9. MPCv2 signing reaches Flr.verifyTssTransaction, then verifyAtomicExportTssTransaction, then assertAtomicExportAmount (flr.ts:516-526). expectedOutput = amount.div(1e9) is 1234567890.123456789. minOutput adds minImportToPFee and keeps the .123456789 fraction. The atomic export output (explained.outputAmount) is an integer nanoFLR value, because Avalanche atomic amounts cannot hold sub-gwei units. So WP must round, and with floor, half-up (which validateHopPrebuild assumes at flr.ts:611 with toFixed(0)) or half-even, a legitimate output lands below minOutput. Then 'atomic export output amount ... is outside the expected range' is thrown. This check is a plain throw, not a finding, so the default warn mode does not soften it. On the base branch the export signed with no amount check. The new tests only use a declared amount that is an exact multiple of 1e9.
Verification: In modules/sdk-coin-flr/src/flr.ts, assertAtomicExportAmount computes const expectedOutput = amount.div(1e9); (line 519) and then minOutput without rounding. explained.outputAmount is always a whole number of nanoFLR, so output.isLessThan(minOutput) throws in every mode. On the base commit, Flr has no verifyTssTransaction override, so these exports signed.
| let tx: BaseTransaction; | ||
| let explained: AtomicTransactionExplanation; | ||
| try { | ||
| tx = await this.getAtomicTransaction(txPrebuild.txHex); | ||
| explained = tx.explainTransaction() as AtomicTransactionExplanation; | ||
| } catch (e) { | ||
| throw new Error(`transaction bytes could not be decoded to verify intent: ${(e as Error).message}`); | ||
| } | ||
|
|
||
| if (explained.type !== TransactionType.Export) { | ||
| throw new Error(`atomic transaction type ${String(explained.type)} does not match the declared export intent`); | ||
| } | ||
| // Explanations map every owner onto this coin's P-chain alias, so matching the output address | ||
| // does not prove the export targets the P-chain. Read the chains encoded in the bytes. | ||
| const { sourceChain, destinationChain } = (tx as FlrPLib.Transaction).toJson(); | ||
| if (sourceChain !== 'C' || destinationChain !== 'P') { | ||
| throw new Error( | ||
| `atomic export must be a C-chain to P-chain export but encodes ${sourceChain} -> ${destinationChain}` | ||
| ); | ||
| } | ||
| const outputs = explained.outputs; | ||
| if (outputs.length !== 1) { | ||
| throw new Error(`atomic export transaction must have exactly 1 output but ${outputs.length} found`); | ||
| } | ||
| if (intent.recipients.length !== 1) { | ||
| throw new Error(`atomic export requires exactly 1 recipient but ${intent.recipients.length} found`); | ||
| } | ||
|
|
||
| const declaredRecipient = intent.recipients[0]; | ||
| const declaredDestination = Flr.normalizePChainAddressSet(declaredRecipient.address); | ||
| if (!declaredDestination) { | ||
| throw new Error(`atomic export recipient ${declaredRecipient.address} is not a valid P-chain address`); | ||
| } | ||
| const decodedDestination = Flr.normalizePChainAddressSet(outputs[0].address); | ||
| if (!decodedDestination) { | ||
| throw new Error(`atomic export destination ${outputs[0].address} is not a valid P-chain address`); | ||
| } | ||
| if (declaredDestination !== decodedDestination) { | ||
| throw new Error( | ||
| `atomic export destination ${outputs[0].address} does not match the declared recipient ${declaredRecipient.address}` | ||
| ); | ||
| } | ||
|
|
||
| this.assertAtomicExportAmount(declaredRecipient.amount, explained.outputAmount); |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: Pre-existing, security-relevant: on FLR MPCv2 exports a compromised server can still get any digest signed. The new export verifier only checks bytes the key share never signs. verifyAtomicExportTssTransaction decodes txPrebuild.txHex (serializedTxHex), but the signing round signs the server's signableHex directly as the hash. Nothing checks that signableHex equals SHA-256 of those bytes. Fix: before returning true, require txPrebuild.txInfo to equal tx.signablePayload (hex), or add the same binding to the prehashed branch at ecdsaMPCv2.ts:1001.
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
An FLR TSS wallet signs an export intent on the MPCv2 path. shouldUsePreHashedSignable is true for 0000-prefixed bytes, so ecdsaMPCv2.ts:992-999 passes txPrebuild: { txHex: unsignedTx.serializedTxHex, txInfo: unsignedTx.signableHex }. assertSignableConsistency runs only when shouldVerifyWithSerializedTxHex is true (ecdsaMPCv2.ts:1001), so nothing binds the digest on this branch. getMpcv2HashBuffer (ecdsaMPCv2.ts:1220-1230) only checks that the buffer is 32 bytes before using signableHex as the message hash. The new Flr.verifyAtomicExportTssTransaction checks the type, the C->P chains, the destination and the amount of serializedTxHex, then returns true without reading txPrebuild.txInfo. A server can send a legitimate export as serializedTxHex together with any 32-byte digest. The verifier passes and the user share signs that digest. The gap exists on base too, but this PR presents FLR exports as verified against intent (the TOB-BITGOEDMPC-2 threat model), and they are not.
Verification: The new verifyAtomicExportTssTransaction (flr.ts) decodes only txPrebuild.txHex and never compares txInfo with tx.signablePayload. assertSignableConsistency at ecdsaMPCv2.ts:1001-1002 skips the pre-hashed branch, and getMpcv2HashBuffer (ecdsaMPCv2.ts:1220-1230) only checks bufferContent.length === 32. Base branch: the code at ecdsaMPCv2.ts 991-1008 is the same.
| const outputs = explained.outputs; | ||
| if (outputs.length !== 1) { | ||
| throw new Error(`atomic export transaction must have exactly 1 output but ${outputs.length} found`); | ||
| } | ||
| if (intent.recipients.length !== 1) { | ||
| throw new Error(`atomic export requires exactly 1 recipient but ${intent.recipients.length} found`); | ||
| } | ||
|
|
||
| const declaredRecipient = intent.recipients[0]; | ||
| const declaredDestination = Flr.normalizePChainAddressSet(declaredRecipient.address); | ||
| if (!declaredDestination) { | ||
| throw new Error(`atomic export recipient ${declaredRecipient.address} is not a valid P-chain address`); | ||
| } | ||
| const decodedDestination = Flr.normalizePChainAddressSet(outputs[0].address); | ||
| if (!decodedDestination) { | ||
| throw new Error(`atomic export destination ${outputs[0].address} is not a valid P-chain address`); | ||
| } | ||
| if (declaredDestination !== decodedDestination) { | ||
| throw new Error( | ||
| `atomic export destination ${outputs[0].address} does not match the declared recipient ${declaredRecipient.address}` | ||
| ); | ||
| } |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: A compromised server can get FLR TSS wallets to sign C-to-P exports that freeze the funds or let one key spend them, and the new export verifier still passes them. verifyAtomicExportTssTransaction compares only the output owner set and amount. It never checks the exported output's locktime or threshold. Fix: require locktime 0 and a threshold that matches the declared P-chain address set, read from exportedOutputs[0].output. Do not use toJson().threshold, which ExportInCTxBuilder.initBuilder never parses. [also at: modules/sdk-coin-flr/src/flr.ts:426 - FLR TSS wallets still sign an atomic export that debits far more C-chain FLR than the declared amount plus fee. The excess is burned as fee.]
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
An FLR TSS wallet signs a declared Export (direct send or pending approval). ecdsaMPCv2.ts:992-999 passes the atomic serializedTxHex, and Flr.verifyTssTransaction (flr.ts:343-360) sends it to verifyAtomicExportTssTransaction. The bytes carry a secp256k1 TransferOutput with owners, threshold and locktime: ExportInCTxBuilder.ts:89-98 reads getLocktime(), and newExportTxFromBaseFee takes both at lines 172-186. The verifier only checks the chain direction, the output count, the owner set (mapOutputToEntry drops threshold and locktime, utils.ts:265-273) and the amount. A server-built export to the right owners P-a~P-b~P-c with threshold: 1 lets the BitGo key spend the P-chain UTXO alone. A far-future locktime freezes the funds. Either way return true and the key share signs. The base branch did no export check at all. This PR presents the check as the safeguard, and it misses these fields. It is separate from the signableHex binding finding: binding the digest would not add these checks.
Verification: Pre-existing and security-relevant. Nothing reads the output's locktime or threshold. ExportInCTxBuilder.initBuilder (modules/sdk-coin-flrp/src/lib/ExportInCTxBuilder.ts:89-98) sets _locktime but never sets _threshold from the parsed output. A far-future locktime freezes the exported funds, and threshold=1 lets any single owner key spend them. The base branch had no export verification at all.
|
Closing in favour of the narrower first release in #10136, which limits the change to The remaining work in this PR is now tracked as separate tickets: CECHO-2680 ( The branch is kept for reference. |
What
Makes EVM TSS signing fail closed. Before the user key share signs,
verifyTssTransactiondecodes the transaction bytes once and comparesto/value/datawith the user intent, on both signing paths: direct send (the caller'stxParams) and pending approval (the storedtxRequest.intent).txParams(callerTxParams) and the chain name intoverifyTransaction. Stored kebab-case DeFi intents (defi-approve,defi-deposit,defi-withdraw) pass the missing-recipients guard so their calldata gets checked.defi-redeemhas no handler and stays rejected, as on master.batch/batchTransferFrom,tokenApprovalto the batcher for MAX,fillNonce, consolidation to the base address, and the DeFi calldata checks.contractCall/customTxthrow.console.warnand the newonVerificationWarningcallback, or throws when the client sets the new client-sidestrictTssVerificationoption onBitGoAPI.transfer(baseAddress, 0)sent to any contract passed the recipient check and could move the native balance.EvmCoin.verifyLegacyTssTransaction, which returnedtruewithout decoding, is removed, so kavaevm and prividiumeth run the parent verifier.@bitgo/abstract-eth.Why
TOB-BITGOEDMPC-2 / INFOSEC-671:
verifyTssTransactiontrusted the server-declaredtxParams.type. A compromised server could labelapprove(attacker, MAX)or any contract call as a payment, or redirect a payment on the pending-approval path, and the key share would sign it.strictTssVerification)approve(attacker, MAX)under a native payment intentcontractCall/customTxintentconsolidationToBaseAddress)defi-redeemintentTest plan
EcdsaUtils.signTxRequest(kavaevm). Rejected bytes never reach the signing round.BitGo SDKCI on this PR.Open items
strictTssVerificationopts into throwing, as agreed on CECHO-2460. Whether that closes TOB-BITGOEDMPC-2 / INFOSEC-671 is still open with security (INFOSEC-691).strictTssVerificationyet; it buildsBitGofrom a fixed option list.callerTxParamstoVerifyTransactionOptions. Out-of-repoverifyTransactionimplementations won't get the caller / stored split until they read it.Supersedes #9972, #9973, #9975, #10114.
Ticket: CECHO-2460
🤖 Generated with Claude Code