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 thread modules/sdk-core/src/bitgo/utils/tss/recipientUtils.ts
Comment thread modules/sdk-core/src/bitgo/utils/tss/recipientUtils.ts Outdated
@mrdanish26 mrdanish26 force-pushed the wcn-151/security-issue branch 2 times, most recently from 94e9421 to 9515b8e Compare June 9, 2026 18:13
Comment thread modules/sdk-core/src/bitgo/utils/tss/recipientUtils.ts
Comment thread modules/sdk-core/src/bitgo/utils/tss/recipientUtils.ts Outdated
mukeshsp
mukeshsp previously approved these changes Jun 12, 2026
joeypinz
joeypinz previously approved these changes Jun 12, 2026
joeypinz
joeypinz previously approved these changes Jun 15, 2026

@joeypinz joeypinz 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.

approved before conflicts resolved. code looks good after conflict resolved 👍

mukeshsp
mukeshsp previously approved these changes Jun 16, 2026
@mrdanish26 mrdanish26 dismissed stale reviews from mukeshsp and joeypinz via b791a15 June 16, 2026 14:57
@mrdanish26 mrdanish26 force-pushed the wcn-151/security-issue branch from b87f004 to b791a15 Compare June 16, 2026 14:57
@mrdanish26 mrdanish26 requested review from joeypinz and mukeshsp June 16, 2026 15:18
@zahin-mohammad

Copy link
Copy Markdown
Contributor

Review summary

This routes both ECDSA TSS signing paths through a single resolveEffectiveTxParams() that resolves recipients (txParamsintent.recipients), exempts known recipient-less types via NO_RECIPIENT_TX_TYPES, and throws otherwise — replacing the txParams: params.txParams || { recipients: [] } default that disabled recipient verification when txParams was absent. Core logic looks sound and the test coverage is good. A few things worth resolving before merge, especially given the revert history below.

Context: this is the third attempt

The same fix has merged and been reverted twice (#8462 → reverted by #8538; #8539 → reverted by #8665). Both reverts were caused by being too strict and throwing on legitimate recipient-less transactions:

This PR's intent.intentType resolution + expanded exemption set is the right fix for what broke #8539. The recurring failure mode (exemption set under-counts legitimate cases) is worth keeping in mind — the bar for completeness should be high here.

Blocking concerns

1. The skipTssRecipientVerification opt-out may undercut the fix on the main path.
Per the description, a companion client-side PR sets this flag to true, which disables recipient verification for that path. But resolveEffectiveTxParams already falls back to intent.recipients for intent-only flows — so what case requires the blanket opt-out? As written, the most common signing path would skip the exact check this PR adds. Could you clarify why the intent fallback is insufficient there, or scope the opt-out more narrowly than a per-call boolean? The flag also reads as insurance against the exemption set being incomplete, which is worth being explicit about.

2. Pending-approval recipient threading was dropped vs. the prior attempts.
#8462 and #8539 both threaded the pending-approval recipients explicitly through recreateTxRequest. This PR removes that and relies entirely on txRequest.intent.recipients being populated after recreation. Can you confirm recreated pending-approval txRequests carry intent.recipients? If not, this could reintroduce a false-throw on that path.

3. enableToken casing.
NO_RECIPIENT_TX_TYPES uses camelCase 'enableToken' (matching intentType), but the EVM coin layer sets txParams.type to lowercase 'enabletoken' in some paths. The intent-sourced path is covered, but a caller passing the lowercase string with no recipients would now throw. Adding 'enabletoken' to the set as well is a cheap safeguard.

Lower-priority notes

  • skipTssRecipientVerification is exposed on the Express route schema. Consistent with the threat model (the flag is set client-side), but it does broaden the bypass surface to every request — worth a confirming comment.
  • The exemption set mixes intent types across coin families. Worth confirming each newly added non-EVM type is genuinely recipient-less and routes through this path, rather than being a value transfer.

Strengths

  • The intent.intentType resolution directly addresses the regression that reverted the previous attempt, and the isPopulatedIntent() type guard replaces the prior unsafe cast.
  • Test coverage asserts value-transfer types (payment, fanout, vote) are not exempt, and the new coin-level suite covers the flag, hop, and missing-param ordering.
  • Centralizing the previously divergent per-coin arrays into one set removes the drift that contributed to the earlier reverts.
  • Reordering the wallet/hop checks ahead of the recipient guard is a genuine correctness improvement and is tested.

Recommendation

Worth resolving (1) and (2) before merge — given two prior production reverts, the key questions are whether the opt-out leaves the main path protected, and whether the pending-approval path still resolves recipients. (3) is a one-line safeguard. The core logic is solid; the remaining risk is in completeness and in whether the opt-out hollows out the protection.

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.

5 participants