Repository navigation
fix(abstract-eth): fail-closed EVM TSS handlers for verifyTssTransaction - #9973
ralph-bitgo[bot] wants to merge 1 commit into
Conversation
|
|
d29510b to
1e03006
Compare
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: make AbstractEthLikeNewCoins.verifyTssTransaction compare the decoded to/value/data against the user intent on every EVM TSS signing path. The intent type picks the handler (payment rules R1-R5, transferToken, tokenApproval, fillNonce, consolidate, DeFi); anything unmatched or unhandled is a violation instead of falling through to true. contractCall/customTx now throw on EVM. Stored kebab-case intent types (payment, defi-*) map to canonical verifier types and the stored intent supplies defiParams / tokenName fields. EvmCoin.verifyLegacyTssTransaction is removed so non-EIP-1559 chains (kavaevm, prividiumeth) run the parent verifier. Why: TOB-BITGOEDMPC-2 / INFOSEC-671 - a compromised server could label arbitrary calldata (e.g. approve(attacker, MAX)) as a transfer and the client key share would sign it unchallenged. Checks that threw before keep throwing; every new check warns (console.warn plus an optional onVerificationWarning callback) and throws under the client-side strictTssVerification option, so the checks roll out without rejecting legitimate requests. Ticket: CECHO-2460 Session-Id: 22db0dc1-cf47-40e2-b717-1bb226a6304c Task-Id: a8a26387-a332-4be1-aab5-f39deddfc502
1e03006 to
b0378d7
Compare
|
Superseded by #10121, which carries the whole CECHO-2460 stack as three commits against |
What
Makes
AbstractEthLikeNewCoins.verifyTssTransactionfail-closed: the user's key share must not sign EVM TSS bytes unless the decodedto/value/datamatch the intent, on both signing paths (direct send = callertxParams; pending approval = storedtxRequest.intent).payment → transfer,defi-approve → defiApprove, …); caller-suppliedtxParams.typestill wins. Handlers accept a closed set of decoded shapes; anything unmatched or unhandled is a violation.transfer/payment, rules R1–R5): 1 recipient withdata(WalletConnect/dApp) →to/value/dataexactly equal to the intent; 1 native recipient → emptydata, matchingto/value; 1 token recipient →to= token contract (statics ortokenData.tokenContractAddress),value= 0, decodedtransfer(address,uint256)(or ERC-7984confidentialTransferrecipient-only) matching; >1 recipients →to= the networkbatcherContractAddress,batch(address[],uint256[])arrays 1:1 withvalue= sum, orbatchTransferFrom(address,address[],uint256[])with the intent token andvalue= 0; anything else (e.g.approve(attacker, MAX)disguised as a payment) is a violation.tokenApproval:to= token contract,approve(spender = batcher, MAX),value= 0.transferToken: same as the token-recipient rule, exactly one recipient.fillNonce: 1-wei self-send to the wallet base address.consolidate/consolidateId: the base-address check now runs automatically, not only underverification.consolidationToBaseAddress.defiApprove/defiDeposit/defiWithdraw: existing calldata checks, now reachable on the pending-approval path withdefiParamsreconstructed from storedamount/shareTokenAmount.contractCall/customTx: throwunsupported intent type for EVM TSS— neither can be created on EVM and there is no intent shape to verify.acceleration,bridgeFunds,enableToken/enabletoken/disableToken/disabletoken,wrapNative/wrap-native,unwrapNative/unwrap-native,wrap,unwrap,finalizeUnwrap,defiRedeem/defi-redeem, staking (delegate,undelegate,switchValidator,stake,unstake,stakeWithCallData,unstakeWithCallData,transferStake,increaseStake,goUnstake,claim,stakeClaimRewards),createAccount,import,importtoc,pledge, and any other type without a handler.to/value, ERC-20 inner recipient/amount, DeFi calldata, wrapApprove, consolidation underverification.consolidationToBaseAddress, missing params) — now also on the pending-approval path. Every new check warns (console.warn+ optionalonVerificationWarningcallback carrying decoded vs expectedto/value/data); the client-side opt-instrictTssVerification: trueturns warnings into hard failures. Strict-by-default is a separate follow-up release.EvmCoin.verifyLegacyTssTransactionso non-EIP-1559 TSS chains (kavaevm,tkavaevm,prividiumeth,tprividiumeth) run the parent verifier instead of returningtruewithout decoding.Why
TOB-BITGOEDMPC-2 / INFOSEC-671:
verifyTssTransactiontrusted the server-declaredtxParams.typeand only validated two calldata shapes undertransfer. A compromised server could labelapprove(attacker, MAX)(or any contract call) as a payment, or redirect/inflate a payment on the pending-approval path wheretypeis the storedintentType(payment), and the key share would sign it unchallenged.Test plan
customTxbehaviour (unsupported types reject outright; the wrapper-approve guard's relabel cases re-based onto still-reachable types).BitGo SDKCI green on the rebased head.Stack
This PR is part 2 of 4; review and merge in order:
master)CECHO-2460-evm-tss-fail-closed-handlers-pt1) ← you are hereCECHO-2460-evm-tss-fail-closed-handlers-pt2)CECHO-2460-evm-tss-fail-closed-handlers-pt3)Release #9972 and #9973 together.
Ticket: CECHO-2460