Skip to content

fix(tss): verify recipients before signing#8539

Merged
mrdanish26 merged 2 commits intomasterfrom
fix/WAL-375-recipients-guard-v2
Apr 27, 2026
Merged

fix(tss): verify recipients before signing#8539
mrdanish26 merged 2 commits intomasterfrom
fix/WAL-375-recipients-guard-v2

Conversation

@mrdanish26
Copy link
Copy Markdown
Contributor

@mrdanish26 mrdanish26 commented Apr 16, 2026

TICKET: WAL-375

Description

Ensures verifyTransaction always receives recipient details before any signing material is sent to BitGo, preventing a compromised API from silently substituting signableHex with attacker-controlled bytes.

A resolveEffectiveTxParams() guard is added to both ECDSA and MPCv2 signRequestBase() paths. It resolves recipients from three sources in priority order:

  1. txParams.recipients — normal case.
  2. txRequest.intent.recipients — fallback for smart-contract interactions (e.g. Tempo supplyController) where native ETH amount = 0 so buildParams is empty but intent carries the recipients.
  3. NO_RECIPIENT_TX_TYPES exemption — types that legitimately carry no recipients; verifyTransaction handles validation internally for these. Throws InvalidTransactionError if none of the above apply.

A database query against marts.wallet_platform.bitgo2__transaction_request identified all intent types where INTENT:recipients IS NULL OR ARRAY_SIZE(INTENT:recipients) = 0. Two ECDSA-path types with significant
record counts were missing from the exemption list:

  • enableToken (17,765 records) — TSS wallets call buildTokenEnablements() which does not populate buildParams.recipients
  • customTx (7,082 records) — DeFi/WalletConnect smart contract interactions; already exempted at populateIntent() server-side but inconsistently enforced at signing

Both have been added to NO_RECIPIENT_TX_TYPES and the abstractEthLikeNewCoins.ts bypass list.

NO_RECIPIENT_TX_TYPES (8 types)

Type Reason
acceleration, fillNonce, transferToken, tokenApproval, consolidate, bridgeFunds Pre-existing exemptions
enableToken TSS wallets do not populate recipients for token enablement — confirmed via prod DB
customTx DeFi/WalletConnect smart contract interactions — already exempted at populateIntent() server-side; must be consistent at signing

Both recipientUtils.ts and the inline bypass list in abstractEthLikeNewCoins.ts verifyTssTransaction are kept in sync.

A previous PR (#8462) hard-threw on any empty recipients, breaking cases 2 and 3. It was reverted in #8538. This PR supersedes it.

Testing

New unit tests — tss/recipientUtils.ts (16 tests):

  • All 8 exempt types pass without recipients
  • txParams.recipients takes priority over intent
  • Intent fallback when txParams is empty or undefined
  • Throws for non-exempt types with no recipients

Updated integration tests — ecdsaMPCv2/signTxRequest.ts (13 tests):

  • Replaced 4 false-confidence .should.not.be.rejectedWith() assertions with nock-backed isDone() assertions that prove signing completes end-to-end
  • Added acceptance tests for enableToken and customTx exemptions
  • Existing reject tests unchanged

@linear
Copy link
Copy Markdown

linear Bot commented Apr 16, 2026

@mrdanish26 mrdanish26 force-pushed the fix/WAL-375-recipients-guard-v2 branch from e444156 to 2d64d76 Compare April 17, 2026 00:17
@mrdanish26 mrdanish26 marked this pull request as ready for review April 17, 2026 02:28
@mrdanish26 mrdanish26 requested review from a team as code owners April 17, 2026 02:28
@sachushaji
Copy link
Copy Markdown
Contributor

@claude

Comment thread modules/sdk-core/src/bitgo/utils/tss/ecdsa/base.ts Outdated
zhongxishen
zhongxishen previously approved these changes Apr 22, 2026
@mrdanish26 mrdanish26 force-pushed the fix/WAL-375-recipients-guard-v2 branch from 2d64d76 to a4915fb Compare April 22, 2026 23:48
@mrdanish26 mrdanish26 requested a review from a team as a code owner April 22, 2026 23:48
@mrdanish26 mrdanish26 force-pushed the fix/WAL-375-recipients-guard-v2 branch from a4915fb to 1f55399 Compare April 22, 2026 23:57
@mrdanish26 mrdanish26 merged commit bef1cda into master Apr 27, 2026
34 of 35 checks passed
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.

5 participants