Skip to content

fix(abstract-eth): fail-closed EVM TSS handlers for verifyTssTransaction - #9973

Open
ralph-bitgo[bot] wants to merge 1 commit into
CECHO-2460-evm-tss-fail-closed-handlers-pt1from
CECHO-2460-evm-tss-fail-closed-handlers-pt2
Open

ralph-bitgo[bot] wants to merge 1 commit into
CECHO-2460-evm-tss-fail-closed-handlers-pt1from
CECHO-2460-evm-tss-fail-closed-handlers-pt2

Conversation

@ralph-bitgo

@ralph-bitgo ralph-bitgo Bot commented Oct 14, 2026 •

Copy link
Copy Markdown

What

Makes AbstractEthLikeNewCoins.verifyTssTransaction fail-closed: the user's key share must not sign EVM TSS bytes unless the decoded to / value / data match the intent, on both signing paths (direct send = caller txParams; pending approval = stored txRequest.intent).

  • Intent type picks the handler. Stored kebab-case types map to canonical verifier types (payment → transfer, defi-approve → defiApprove, …); caller-supplied txParams.type still wins. Handlers accept a closed set of decoded shapes; anything unmatched or unhandled is a violation.
    • Payment (transfer / payment, rules R1–R5): 1 recipient with data (WalletConnect/dApp) → to/value/data exactly equal to the intent; 1 native recipient → empty data, matching to/value; 1 token recipient → to = token contract (statics or tokenData.tokenContractAddress), value = 0, decoded transfer(address,uint256) (or ERC-7984 confidentialTransfer recipient-only) matching; >1 recipients → to = the network batcherContractAddress, batch(address[],uint256[]) arrays 1:1 with value = sum, or batchTransferFrom(address,address[],uint256[]) with the intent token and value = 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 under verification.consolidationToBaseAddress.
    • defiApprove / defiDeposit / defiWithdraw: existing calldata checks, now reachable on the pending-approval path with defiParams reconstructed from stored amount / shareTokenAmount.
    • contractCall / customTx: throw unsupported intent type for EVM TSS — neither can be created on EVM and there is no intent shape to verify.
  • Unhandled types warn instead of silently passing: 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.
  • Rollout (per settled design): checks that threw before keep throwing (native to/value, ERC-20 inner recipient/amount, DeFi calldata, wrapApprove, consolidation under verification.consolidationToBaseAddress, missing params) — now also on the pending-approval path. Every new check warns (console.warn + optional onVerificationWarning callback carrying decoded vs expected to/value/data); the client-side opt-in strictTssVerification: true turns warnings into hard failures. Strict-by-default is a separate follow-up release.
  • Removes EvmCoin.verifyLegacyTssTransaction so non-EIP-1559 TSS chains (kavaevm, tkavaevm, prividiumeth, tprividiumeth) run the parent verifier instead of returning true without decoding.

Why

TOB-BITGOEDMPC-2 / INFOSEC-671: verifyTssTransaction trusted the server-declared txParams.type and only validated two calldata shapes under transfer. A compromised server could label approve(attacker, MAX) (or any contract call) as a payment, or redirect/inflate a payment on the pending-approval path where type is the stored intentType (payment), and the key share would sign it unchallenged.

Test plan

  • Existing suites stay green with the strengthened contract (counts on the stack rebased onto current master): sdk-coin-eth 517, sdk-coin-bsc 56, sdk-coin-evm 124, abstract-eth 188, sdk-core 1063, sdk-api 253.
  • sdk-coin-eth contract tests updated for the new customTx behaviour (unsupported types reject outright; the wrapper-approve guard's relabel cases re-based onto still-reachable types).
  • Acceptance coverage for the new rules lands in part 3; BitGo SDK CI green on the rebased head.

Superseded in part 4 (#10114): when the caller declares its own type or recipients, the stored intent is ignored entirely (no server tokenData.tokenContractAddress fallback, no server calldata, no server-selected handler), and payment calldata is bound to the locally declared asset. Unresolvable or conflicting token identity is a violation rather than a skipped check.

Stack

This PR is part 2 of 4; review and merge in order:

  1. fix(sdk-core): carry TSS intent and strict policy to verifyTransaction #9972 — sdk-core intent + strict policy plumbing (base: master)
  2. fix(abstract-eth): fail-closed EVM TSS handlers for verifyTssTransaction #9973 — abstract-eth fail-closed verifier (base: CECHO-2460-evm-tss-fail-closed-handlers-pt1) ← you are here
  3. test(sdk-coin-evm): cover fail-closed EVM TSS verification paths #9975 — verification test suites (base: CECHO-2460-evm-tss-fail-closed-handlers-pt2)
  4. fix(abstract-eth): verify EVM TSS payments against declared asset #10114 — caller-declared intent and asset binding (base: CECHO-2460-evm-tss-fail-closed-handlers-pt3)

Release #9972 and #9973 together.

Ticket: CECHO-2460

@linear-code

linear-code Bot commented Oct 14, 2026

Copy link
Copy Markdown
Contributor

CECHO-2460

@github-actions

github-actions Bot commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

⚠️ Compatibility checks are failing on Node.js 27-nightly (experimental, non-blocking). This does not block merge, but it signals an incompatibility to fix before this release becomes supported.

View run

@dpkjnr
dpkjnr force-pushed the CECHO-2460-evm-tss-fail-closed-handlers-pt2 branch from d29510b to 1e03006 Compare November 3, 2026 07:01
@dpkjnr
dpkjnr marked this pull request as ready for review November 3, 2026 10:34
@dpkjnr
dpkjnr requested a review from a team as a code owner November 3, 2026 10:34

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@dpkjnr
dpkjnr force-pushed the CECHO-2460-evm-tss-fail-closed-handlers-pt2 branch from 1e03006 to b0378d7 Compare November 3, 2026 10:49

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants