From f4c6b6083ffd0d5289c74d2ed4c3140db4bacdb5 Mon Sep 17 00:00:00 2001 From: Marzooqa Kather Date: Fri, 31 Jul 2026 07:21:20 +0000 Subject: [PATCH] fix(sdk-core): wire resolveEffectiveTxParams into EddsaMPCv2Utils What changed: - eddsaMPCv2.ts signRequestBase: replaced the vulnerable `params.txParams || { recipients: [] }` fallback with `resolveEffectiveTxParams(txRequest, params.txParams, this.baseCoin.getChain())`. resolveEffectiveTxParams throws InvalidTransactionError when recipients cannot be resolved and the intent is not a recognised no-recipient type. - wallet.ts signTransactionTss: removed the EdDSA MPCv2 special-case block that pre-fetched the txRequest and called txParamsFromIntent before handing off to signTxRequest. This pre-fetch was introduced to work around the missing guard; now that resolveEffectiveTxParams owns intent-based derivation inside signRequestBase (which already fetches the txRequest when given a string ID), the wallet-layer duplication is redundant. - Removed the now-unused txParamsFromIntent import from wallet.ts. - Tests: added resolveEffectiveTxParams guard suite to signTxRequest.ts covering the stakingAuthorize attack vector (throws), empty-recipient txParams (throws), allowlisted intentTypes deactivate/consolidate (pass), intent-sourced recipients (pass), and staking intent with stakingRequestId (pass). Why: Trail of Bits finding TOB-BITGOEDMPC-1 (WCI-1100): the EdDSA MPCv2 re-sign path silently substituted an empty-recipients object when txParams was absent. Several coin-level verifyTransaction implementations (SOL, VET, Tempo, TRON) skip output-matching validation when recipients.length is 0, allowing a compromised BitGo server to present a malicious txHex that signs without any client-side validation. ECDSA already used resolveEffectiveTxParams for fail-closed behaviour (ecdsaMPCv2.ts:958,965 and ecdsa.ts:821,828); this change ports the same pattern to EdDSA MPCv2. MPCv1 (eddsa.ts) is explicitly out of scope per ticket WCI-1111. Ticket: WCI-1111 Session-Id: 94a72c0d-fce8-4672-a6a4-26df25a64dfc Task-Id: 76f4312b-c52c-4478-94f6-457913e8c0b7 --- .../tssUtils/eddsaMPCv2/signTxRequest.ts | 130 ++++++++++++++++++ .../src/bitgo/utils/tss/eddsa/eddsaMPCv2.ts | 3 +- modules/sdk-core/src/bitgo/wallet/wallet.ts | 14 -- 3 files changed, 132 insertions(+), 15 deletions(-) diff --git a/modules/bitgo/test/v2/unit/internal/tssUtils/eddsaMPCv2/signTxRequest.ts b/modules/bitgo/test/v2/unit/internal/tssUtils/eddsaMPCv2/signTxRequest.ts index e5403c3f4d..5c856df5ca 100644 --- a/modules/bitgo/test/v2/unit/internal/tssUtils/eddsaMPCv2/signTxRequest.ts +++ b/modules/bitgo/test/v2/unit/internal/tssUtils/eddsaMPCv2/signTxRequest.ts @@ -1,9 +1,11 @@ +import * as sinon from 'sinon'; import { BaseCoin, BitgoGPGPublicKey, common, ECDSAUtils, EDDSAUtils, + InvalidTransactionError, RequestTracer, RequestType, SignatureShareRecord, @@ -419,6 +421,134 @@ describe('signTxRequest:', function () { nockPromises[3].isDone().should.be.false(); }); + + describe('resolveEffectiveTxParams guard (WCI-1111)', function () { + let sandbox: sinon.SinonSandbox; + + beforeEach(function () { + sandbox = sinon.createSandbox(); + }); + + afterEach(function () { + sandbox.restore(); + }); + + it('throws InvalidTransactionError when txParams is absent and intent has no recipients (malicious/empty-recipient path)', async function () { + // Simulate the stakingAuthorize attack vector: intent has no recipients + // and intentType is not on the NO_RECIPIENT_TX_TYPES allowlist. + const maliciousTxRequest: TxRequest = { + ...txRequest, + intent: { intentType: 'stakingAuthorize' } as any, + }; + const userPrvBase64 = Buffer.from(userKeyShare).toString('base64'); + await tssUtils + .signTxRequest({ + txRequest: maliciousTxRequest, + prv: userPrvBase64, + reqId, + // No txParams — the re-sign path that was previously vulnerable + }) + .should.be.rejectedWith(InvalidTransactionError); + }); + + it('throws InvalidTransactionError when txParams has empty recipients and intentType is not allowlisted', async function () { + const maliciousTxRequest: TxRequest = { + ...txRequest, + intent: { intentType: 'payment' } as any, + }; + const userPrvBase64 = Buffer.from(userKeyShare).toString('base64'); + await tssUtils + .signTxRequest({ + txRequest: maliciousTxRequest, + prv: userPrvBase64, + reqId, + txParams: { recipients: [] }, + }) + .should.be.rejectedWith(InvalidTransactionError); + }); + + it('does not throw for allowlisted no-recipient intentType (deactivate)', async function () { + sandbox.stub(baseCoin, 'verifyTransaction').resolves(true); + const nockPromises = await getNockPromisesForEddsaSigning(txRequest); + await Promise.all(nockPromises); + + const noRecipientTxRequest: TxRequest = { + ...txRequest, + intent: { intentType: 'deactivate' } as any, + }; + const userPrvBase64 = Buffer.from(userKeyShare).toString('base64'); + await tssUtils.signTxRequest({ + txRequest: noRecipientTxRequest, + prv: userPrvBase64, + reqId, + // No txParams — legitimate no-recipient flow + }); + }); + + it('does not throw for allowlisted no-recipient intentType (consolidate)', async function () { + sandbox.stub(baseCoin, 'verifyTransaction').resolves(true); + const nockPromises = await getNockPromisesForEddsaSigning(txRequest); + await Promise.all(nockPromises); + + const consolidateTxRequest: TxRequest = { + ...txRequest, + intent: { intentType: 'consolidate' } as any, + }; + const userPrvBase64 = Buffer.from(userKeyShare).toString('base64'); + await tssUtils.signTxRequest({ + txRequest: consolidateTxRequest, + prv: userPrvBase64, + reqId, + }); + }); + + it('uses intent recipients when txParams is absent and intent has recipients', async function () { + sandbox.stub(baseCoin, 'verifyTransaction').resolves(true); + const nockPromises = await getNockPromisesForEddsaSigning(txRequest); + await Promise.all(nockPromises); + + const intentRecipientTxRequest: TxRequest = { + ...txRequest, + intent: { + intentType: 'payment', + recipients: [ + { + address: { address: 'HMEgbR4S2hLKfst2VZUVpHVUu4FioFPyW5iUuJvZdMvs' }, + amount: { value: '999990000', symbol: 'sol' }, + }, + ], + } as any, + }; + const userPrvBase64 = Buffer.from(userKeyShare).toString('base64'); + // Should not throw — intent provides the recipients + await tssUtils.signTxRequest({ + txRequest: intentRecipientTxRequest, + prv: userPrvBase64, + reqId, + }); + }); + + it('does not throw for staking intent with stakingRequestId (generic staking signal)', async function () { + sandbox.stub(baseCoin, 'verifyTransaction').resolves(true); + const nockPromises = await getNockPromisesForEddsaSigning(txRequest); + await Promise.all(nockPromises); + + const stakingTxRequest: TxRequest = { + ...txRequest, + intent: { + intentType: 'delegate', + stakingRequestId: 'staking-req-id-123', + } as any, + }; + const userPrvBase64 = Buffer.from(userKeyShare).toString('base64'); + await tssUtils.signTxRequest({ + txRequest: stakingTxRequest, + prv: userPrvBase64, + reqId, + }); + }); + }); + async function getNockPromisesForEddsaSigning( txRequest: TxRequest, requestType: RequestType = RequestType.tx, diff --git a/modules/sdk-core/src/bitgo/utils/tss/eddsa/eddsaMPCv2.ts b/modules/sdk-core/src/bitgo/utils/tss/eddsa/eddsaMPCv2.ts index 311262515c..9613906817 100644 --- a/modules/sdk-core/src/bitgo/utils/tss/eddsa/eddsaMPCv2.ts +++ b/modules/sdk-core/src/bitgo/utils/tss/eddsa/eddsaMPCv2.ts @@ -47,6 +47,7 @@ import { import { EncryptionVersion } from '../../../../api'; import { BitGoBase } from '../../../bitgoBase'; import { BaseEddsaUtils } from './base'; +import { resolveEffectiveTxParams } from '../recipientUtils'; import { EddsaMPCv2KeyGenSendFn, KeyGenSenderForEnterprise } from './eddsaMPCv2KeyGenSender'; import { EddsaMPCv2RecoveryKeyShares } from './types'; @@ -553,7 +554,7 @@ export class EddsaMPCv2Utils extends BaseEddsaUtils { bufferContent = Buffer.from(txOrMessageToSign, 'hex'); await this.baseCoin.verifyTransaction({ txPrebuild: { txHex: unsignedTx.serializedTxHex ?? txOrMessageToSign }, - txParams: params.txParams || { recipients: [] }, + txParams: resolveEffectiveTxParams(txRequest, params.txParams, this.baseCoin.getChain()), wallet: this.wallet, walletType: this.wallet.multisigType(), }); diff --git a/modules/sdk-core/src/bitgo/wallet/wallet.ts b/modules/sdk-core/src/bitgo/wallet/wallet.ts index 6c5babf9f4..3d372128ba 100644 --- a/modules/sdk-core/src/bitgo/wallet/wallet.ts +++ b/modules/sdk-core/src/bitgo/wallet/wallet.ts @@ -54,7 +54,6 @@ import { } from '../utils'; import { decodeWithCodec } from '../utils/codecs'; import { postWithCodec } from '../utils/postWithCodec'; -import { txParamsFromIntent } from '../utils/tss/baseTSSUtils'; import { EcdsaMPCv2Utils, EcdsaUtils } from '../utils/tss/ecdsa'; import EddsaUtils, { EddsaMPCv2Utils } from '../utils/tss/eddsa'; import { getTxRequestApiVersion, validateTxRequestApiVersion } from '../utils/txRequest'; @@ -5059,19 +5058,6 @@ export class Wallet implements IWallet { let txRequest: string | TxRequest = params.txPrebuild.txRequestId; let txParams: TransactionParams | undefined = params.txPrebuild.buildParams; - // EdDSA MPCv2 re-sign path: buildParams is absent when the UI calls signAndSendTxRequest with - // only txRequestId. Derive txParams from the persisted intent so verifyTransaction receives - // the correct recipients before DSG starts. Other TSS variants are unaffected by the guard. - if (!txParams && this.multisigTypeVersion() === 'MPCv2' && this.baseCoin.getMPCAlgorithm() === 'eddsa') { - txRequest = await getTxRequest( - this.bitgo, - this.id(), - params.txPrebuild.txRequestId, - params.reqId || new RequestTracer() - ); - txParams = txParamsFromIntent(txRequest.intent, this.baseCoin.getChain()); - } - try { return await this.tssUtils!.signTxRequest({ txRequest,