Skip to content

fix(sdk-core): enforce recipient verification in ECDSA TSS signing#8924

Open
mrdanish26 wants to merge 2 commits into
masterfrom
wcn-151/security-issue
Open

fix(sdk-core): enforce recipient verification in ECDSA TSS signing#8924
mrdanish26 wants to merge 2 commits into
masterfrom
wcn-151/security-issue

Conversation

@mrdanish26

@mrdanish26 mrdanish26 commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

Summary

Closes the ECDSA TSS default-path signing vulnerability (WCN-151 Phase 3)
where a compromised BitGo API server could swap signableHex undetected.

The Vulnerability

Both EcdsaUtils and EcdsaMPCv2Utils defaulted txParams when absent:

// Before — vulnerable
txParams: params.txParams || { recipients: [] }

With no txParams (Classic UI pending approval, Express), verifyTransaction()
gets an empty recipients list and skips all address/amount checks.

Root cause of prior failures: stale hardcoded arrays in coin-level overrides
were missing staking types, causing false throws on legitimate staking transactions.

Loki is authoritative. Kibana misses wallet-platform-admin-nocluster entries.

┌────────┬────────────────┬──────┬──────────────────┐
  Coin      tx_type      Loki       Status      
├────────┼────────────────┼──────┼──────────────────┤
 canton  enableToken      206  already exempt   
├────────┼────────────────┼──────┼──────────────────┤
 sol     customTx         106  already exempt   
├────────┼────────────────┼──────┼──────────────────┤
 canton  createAccount     89  added in this PR 
├────────┼────────────────┼──────┼──────────────────┤
 sol     enableToken       22  already exempt   
├────────┼────────────────┼──────┼──────────────────┤
 canton  transferAccept     8  added in this PR 
├────────┼────────────────┼──────┼──────────────────┤
 bsc     tokenApproval      6  already exempt   
├────────┼────────────────┼──────┼──────────────────┤
 hash    contractCall       3  added in this PR 
├────────┼────────────────┼──────┼──────────────────┤
 ada     pledge             2  added in this PR 
├────────┼────────────────┼──────┼──────────────────┤
 apt     customTx           2  already exempt   
└────────┴────────────────┴──────┴──────────────────┘

Changes

Commit 1  Enforce recipient verification

- recipientUtils.ts (new)  27-type NO_RECIPIENT_TX_TYPES set and
resolveEffectiveTxParams(). Falls back to intent.recipients, resolves
txType from intent.intentType, propagates stakingRequestId, throws
InvalidTransactionError only when no recipients and no exemption applies.

- ecdsa.ts, ecdsaMPCv2.ts  replace vulnerable default:
txParams: resolveEffectiveTxParams(txRequest, params.txParams)

- bsc.ts, bscToken.ts, xdc.ts, xdcToken.ts, evmCoin.ts,
abstractEthLikeNewCoins.ts  replace stale hardcoded arrays with
NO_RECIPIENT_TX_TYPES.has(txParams.type) + stakingRequestId bypass.

- iBaseCoin.ts, baseTypes.ts, iWallet.ts  add
stakingRequestId?: string to TransactionParams, PopulatedIntent,
PrebuildTransactionResult.

Commit 2  skipTssRecipientVerification opt-out flag

Safety-net for callers whose intent type has no explicit recipients. Lets
callers bypass verification without reverting the PR if a new type appears
outside NO_RECIPIENT_TX_TYPES.


Default behavior:

┌────────────────────────┬────────────────────────┐
          Path                Enforcement       
├────────────────────────┼────────────────────────┤
 Direct SDK callers      ON (flag not set)      
├────────────────────────┼────────────────────────┤
 bitgo-ui (separate PR)  OFF (flag set to true) 
└────────────────────────┴────────────────────────┘

Test Plan

- yarn test --filter=@bitgo/sdk-core passes
- Normal ETH/BTC send  address + amount still verified
- skipTssRecipientVerification: true  returns true immediately
- skipTssRecipientVerification not set  enforcement runs as before
- Post-deploy Loki: zero new "TSS signing with no intent recipients" hits
- TypeScript compiles clean across all affected packages

@linear-code

linear-code Bot commented Jun 3, 2026

Copy link
Copy Markdown

WCN-151

@mrdanish26 mrdanish26 force-pushed the wcn-151/security-issue branch 2 times, most recently from 8959f9d to 8c93794 Compare June 4, 2026 00:01
@mrdanish26 mrdanish26 marked this pull request as ready for review June 4, 2026 16:55
@mrdanish26 mrdanish26 requested review from a team as code owners June 4, 2026 16:55
@mrdanish26 mrdanish26 marked this pull request as draft June 4, 2026 16:55
@mrdanish26 mrdanish26 marked this pull request as ready for review June 5, 2026 18:17
@mrdanish26 mrdanish26 marked this pull request as draft June 5, 2026 19:02
@mrdanish26 mrdanish26 marked this pull request as ready for review June 5, 2026 21:37
@mrdanish26 mrdanish26 force-pushed the wcn-151/security-issue branch 2 times, most recently from 23705bd to 1f45916 Compare June 8, 2026 23:22
Comment thread modules/abstract-eth/src/abstractEthLikeNewCoins.ts Outdated
Comment on lines +5 to +21
/**
* Transaction types that legitimately carry no explicit recipients.
* These are the intentType strings as stored in TxRequest.intent.intentType by WP.
* verifyTransaction handles no-recipient validation for these internally.
* Mirrors the bypass list in abstractEthLikeNewCoins.ts verifyTssTransaction.
*
* ECDSA types: acceleration, fillNonce, transferToken, tokenApproval, consolidate,
* bridgeFunds, enableToken, customTx, contractCall
* BSC/BNB delegation-based staking: delegate, undelegate, switchValidator
* CELO/ETH lock-based staking: stake, unstake, stakeWithCallData, unstakeWithCallData,
* transferStake, increaseStake, goUnstake
* Claim rewards (BSC, CELO — TRX/SOL use EdDSA and are unaffected): claim, stakeClaimRewards
* Dry-run confirmed (2026-05-20 investigation): createAccount, transferAccept, transferReject,
* transferOfferWithdrawn, cantonCommand, pledge
* Dry-run confirmed (2026-06-03 investigation): contractCall (hash/Provenance — secp256k1,
* smart contract invocation with no explicit recipients)
*/

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

please trim this comment to only report context for a generic reader./

'claim',
'stakeClaimRewards',

// Dry-run confirmed no-recipient types (13-day observation, 2026-05-20)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

redundant

@mrdanish26 mrdanish26 force-pushed the wcn-151/security-issue branch from bc97127 to 94e9421 Compare June 9, 2026 18:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants