Skip to content

Commit 71040ea

Browse files
committed
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: 1c178dac-6528-4ee7-937d-974216871d68 Task-Id: e91df1ba-6cf4-4b0c-8df2-2588f555481e
1 parent f7b8d3a commit 71040ea

5 files changed

Lines changed: 135 additions & 112 deletions

File tree

modules/bitgo/test/v2/unit/internal/tssUtils/eddsaMPCv2/signTxRequest.ts

Lines changed: 129 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,11 @@
1+
import * as sinon from 'sinon';
12
import {
23
BaseCoin,
34
BitgoGPGPublicKey,
45
common,
56
ECDSAUtils,
67
EDDSAUtils,
8+
InvalidTransactionError,
79
RequestTracer,
810
RequestType,
911
SignatureShareRecord,
@@ -419,6 +421,133 @@ describe('signTxRequest:', function () {
419421
nockPromises[3].isDone().should.be.false();
420422
});
421423

424+
describe('resolveEffectiveTxParams guard (WCI-1111)', function () {
425+
let sandbox: sinon.SinonSandbox;
426+
427+
beforeEach(function () {
428+
sandbox = sinon.createSandbox();
429+
});
430+
431+
afterEach(function () {
432+
sandbox.restore();
433+
});
434+
435+
it('throws InvalidTransactionError when txParams is absent and intent has no recipients (malicious/empty-recipient path)', async function () {
436+
// Simulate the stakingAuthorize attack vector: intent has no recipients
437+
// and intentType is not on the NO_RECIPIENT_TX_TYPES allowlist.
438+
const maliciousTxRequest: TxRequest = {
439+
...txRequest,
440+
intent: { intentType: 'stakingAuthorize' } as any,
441+
};
442+
const userPrvBase64 = Buffer.from(userKeyShare).toString('base64');
443+
await tssUtils
444+
.signTxRequest({
445+
txRequest: maliciousTxRequest,
446+
prv: userPrvBase64,
447+
reqId,
448+
// No txParams — the re-sign path that was previously vulnerable
449+
})
450+
.should.be.rejectedWith(InvalidTransactionError);
451+
});
452+
453+
it('throws InvalidTransactionError when txParams has empty recipients and intentType is not allowlisted', async function () {
454+
const maliciousTxRequest: TxRequest = {
455+
...txRequest,
456+
intent: { intentType: 'payment' } as any,
457+
};
458+
const userPrvBase64 = Buffer.from(userKeyShare).toString('base64');
459+
await tssUtils
460+
.signTxRequest({
461+
txRequest: maliciousTxRequest,
462+
prv: userPrvBase64,
463+
reqId,
464+
txParams: { recipients: [] },
465+
})
466+
.should.be.rejectedWith(InvalidTransactionError);
467+
});
468+
469+
it('does not throw for allowlisted no-recipient intentType (deactivate)', async function () {
470+
sandbox.stub(baseCoin, 'verifyTransaction').resolves(true);
471+
const nockPromises = await getNockPromisesForEddsaSigning(txRequest);
472+
await Promise.all(nockPromises);
473+
474+
const noRecipientTxRequest: TxRequest = {
475+
...txRequest,
476+
intent: { intentType: 'deactivate' } as any,
477+
};
478+
const userPrvBase64 = Buffer.from(userKeyShare).toString('base64');
479+
await tssUtils.signTxRequest({
480+
txRequest: noRecipientTxRequest,
481+
prv: userPrvBase64,
482+
reqId,
483+
// No txParams — legitimate no-recipient flow
484+
});
485+
});
486+
487+
it('does not throw for allowlisted no-recipient intentType (consolidate)', async function () {
488+
sandbox.stub(baseCoin, 'verifyTransaction').resolves(true);
489+
const nockPromises = await getNockPromisesForEddsaSigning(txRequest);
490+
await Promise.all(nockPromises);
491+
492+
const consolidateTxRequest: TxRequest = {
493+
...txRequest,
494+
intent: { intentType: 'consolidate' } as any,
495+
};
496+
const userPrvBase64 = Buffer.from(userKeyShare).toString('base64');
497+
await tssUtils.signTxRequest({
498+
txRequest: consolidateTxRequest,
499+
prv: userPrvBase64,
500+
reqId,
501+
});
502+
});
503+
504+
it('uses intent recipients when txParams is absent and intent has recipients', async function () {
505+
sandbox.stub(baseCoin, 'verifyTransaction').resolves(true);
506+
const nockPromises = await getNockPromisesForEddsaSigning(txRequest);
507+
await Promise.all(nockPromises);
508+
509+
const intentRecipientTxRequest: TxRequest = {
510+
...txRequest,
511+
intent: {
512+
intentType: 'payment',
513+
recipients: [
514+
{
515+
address: { address: 'HMEgbR4S2hLKfst2VZUVpHVUu4FioFPyW5iUuJvZdMvs' },
516+
amount: { value: '999990000', symbol: 'sol' },
517+
},
518+
],
519+
} as any,
520+
};
521+
const userPrvBase64 = Buffer.from(userKeyShare).toString('base64');
522+
// Should not throw — intent provides the recipients
523+
await tssUtils.signTxRequest({
524+
txRequest: intentRecipientTxRequest,
525+
prv: userPrvBase64,
526+
reqId,
527+
});
528+
});
529+
530+
it('does not throw for staking intent with stakingRequestId (generic staking signal)', async function () {
531+
sandbox.stub(baseCoin, 'verifyTransaction').resolves(true);
532+
const nockPromises = await getNockPromisesForEddsaSigning(txRequest);
533+
await Promise.all(nockPromises);
534+
535+
const stakingTxRequest: TxRequest = {
536+
...txRequest,
537+
intent: {
538+
intentType: 'delegate',
539+
stakingRequestId: 'staking-req-id-123',
540+
} as any,
541+
};
542+
const userPrvBase64 = Buffer.from(userKeyShare).toString('base64');
543+
await tssUtils.signTxRequest({
544+
txRequest: stakingTxRequest,
545+
prv: userPrvBase64,
546+
reqId,
547+
});
548+
});
549+
});
550+
422551
async function getNockPromisesForEddsaSigning(
423552
txRequest: TxRequest,
424553
requestType: RequestType = RequestType.tx,

modules/sdk-core/src/bitgo/utils/tss/baseTSSUtils.ts

Lines changed: 1 addition & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,6 @@ import {
3131
IntentOptionsForMessage,
3232
IntentOptionsForTypedData,
3333
ITssUtils,
34-
PopulatedIntent,
3534
PopulatedIntentForMessageSigning,
3635
PopulatedIntentForTypedDataSigning,
3736
PrebuildTransactionWithIntentOptions,
@@ -50,28 +49,6 @@ import { getBitgoGpgPubKey } from '../opengpgUtils';
5049
import assert from 'assert';
5150
import { MessageStandardType } from '../messageTypes';
5251

53-
/**
54-
* Derives txParams from the persisted intent on a TxRequest for EdDSA MPCv2 signing paths
55-
* where no SDK-local buildParams is available (PA path and UI re-sign path).
56-
* Native coin transfers (where symbol equals the chain name) are excluded from tokenName.
57-
*/
58-
export function txParamsFromIntent(intent: unknown, chainName: string): TransactionParams | undefined {
59-
if (typeof intent !== 'object' || intent === null || !('recipients' in intent)) {
60-
return undefined;
61-
}
62-
const { recipients } = intent as PopulatedIntent;
63-
if (!recipients?.length) {
64-
return undefined;
65-
}
66-
return {
67-
recipients: recipients.map((r) => ({
68-
address: r.address.address,
69-
amount: String(r.amount.value),
70-
...(r.amount.symbol && r.amount.symbol !== chainName && { tokenName: r.amount.symbol }),
71-
})),
72-
};
73-
}
74-
7552
/**
7653
* BaseTssUtil class which different signature schemes have to extend
7754
*/
@@ -604,14 +581,7 @@ export default class BaseTssUtils<KeyShare> extends MpcUtils implements ITssUtil
604581
await this.deleteSignatureShares(txRequestId, reqId);
605582
// after delete signatures shares get the tx without them
606583
const txRequest = await this.getTxRequest(txRequestId, reqId);
607-
// EdDSA MPCv2 re-verifies the transaction against txParams.recipients before DSG starts.
608-
// On the PA path there is no SDK-local buildParams, so derive txParams from the persisted
609-
// intent. Other TSS variants either skip recipient verification or already work without txParams.
610-
const txParams =
611-
this.wallet.multisigTypeVersion() === 'MPCv2' && this.baseCoin.getMPCAlgorithm() === 'eddsa'
612-
? txParamsFromIntent(txRequest.intent, this.baseCoin.getChain())
613-
: undefined;
614-
return await this.signTxRequest({ txRequest, prv: decryptedPrv, reqId, txParams });
584+
return await this.signTxRequest({ txRequest, prv: decryptedPrv, reqId });
615585
}
616586

617587
/**

modules/sdk-core/src/bitgo/utils/tss/eddsa/eddsaMPCv2.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@ import {
4747
import { EncryptionVersion } from '../../../../api';
4848
import { BitGoBase } from '../../../bitgoBase';
4949
import { BaseEddsaUtils } from './base';
50+
import { resolveEffectiveTxParams } from '../recipientUtils';
5051
import { EddsaMPCv2KeyGenSendFn, KeyGenSenderForEnterprise } from './eddsaMPCv2KeyGenSender';
5152
import { EddsaMPCv2RecoveryKeyShares } from './types';
5253

@@ -553,7 +554,7 @@ export class EddsaMPCv2Utils extends BaseEddsaUtils {
553554
bufferContent = Buffer.from(txOrMessageToSign, 'hex');
554555
await this.baseCoin.verifyTransaction({
555556
txPrebuild: { txHex: unsignedTx.serializedTxHex ?? txOrMessageToSign },
556-
txParams: params.txParams || { recipients: [] },
557+
txParams: resolveEffectiveTxParams(txRequest, params.txParams, this.baseCoin.getChain()),
557558
wallet: this.wallet,
558559
walletType: this.wallet.multisigType(),
559560
});

modules/sdk-core/src/bitgo/wallet/wallet.ts

Lines changed: 2 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,6 @@ import {
5454
} from '../utils';
5555
import { decodeWithCodec } from '../utils/codecs';
5656
import { postWithCodec } from '../utils/postWithCodec';
57-
import { txParamsFromIntent } from '../utils/tss/baseTSSUtils';
5857
import { EcdsaMPCv2Utils, EcdsaUtils } from '../utils/tss/ecdsa';
5958
import EddsaUtils, { EddsaMPCv2Utils } from '../utils/tss/eddsa';
6059
import { getTxRequestApiVersion, validateTxRequestApiVersion } from '../utils/txRequest';
@@ -5056,21 +5055,8 @@ export class Wallet implements IWallet {
50565055
throw new Error('prv required to sign transactions with TSS');
50575056
}
50585057

5059-
let txRequest: string | TxRequest = params.txPrebuild.txRequestId;
5060-
let txParams: TransactionParams | undefined = params.txPrebuild.buildParams;
5061-
5062-
// EdDSA MPCv2 re-sign path: buildParams is absent when the UI calls signAndSendTxRequest with
5063-
// only txRequestId. Derive txParams from the persisted intent so verifyTransaction receives
5064-
// the correct recipients before DSG starts. Other TSS variants are unaffected by the guard.
5065-
if (!txParams && this.multisigTypeVersion() === 'MPCv2' && this.baseCoin.getMPCAlgorithm() === 'eddsa') {
5066-
txRequest = await getTxRequest(
5067-
this.bitgo,
5068-
this.id(),
5069-
params.txPrebuild.txRequestId,
5070-
params.reqId || new RequestTracer()
5071-
);
5072-
txParams = txParamsFromIntent(txRequest.intent, this.baseCoin.getChain());
5073-
}
5058+
const txRequest: string | TxRequest = params.txPrebuild.txRequestId;
5059+
const txParams: TransactionParams | undefined = params.txPrebuild.buildParams;
50745060

50755061
try {
50765062
return await this.tssUtils!.signTxRequest({

modules/sdk-core/test/unit/bitgo/utils/tss/baseTSSUtils.ts

Lines changed: 1 addition & 64 deletions
Original file line numberDiff line numberDiff line change
@@ -338,7 +338,7 @@ describe('Base TSS Utils', function () {
338338
return coin;
339339
}
340340

341-
it('derives txParams from intent for EdDSA MPCv2 wallets', async function () {
341+
it('passes undefined txParams to signTxRequest', async function () {
342342
const txRequestId = 'tx-req-id-1';
343343
const reqId = new RequestTracer();
344344
const txRequest = buildTxRequest({
@@ -356,69 +356,6 @@ describe('Base TSS Utils', function () {
356356

357357
await utils.recreateTxRequest(txRequestId, 'prv', reqId);
358358

359-
// Native SOL: symbol equals the chain name, so tokenName must be omitted
360-
assert.deepStrictEqual(signTxRequestStub.firstCall.args[0].txParams, {
361-
recipients: [{ address: 'solAddr1', amount: '5000000' }],
362-
});
363-
});
364-
365-
it('sets tokenName for SPL tokens (symbol differs from chain name)', async function () {
366-
const txRequestId = 'tx-req-id-spl';
367-
const reqId = new RequestTracer();
368-
const txRequest = buildTxRequest({
369-
txRequestId,
370-
intent: {
371-
intentType: 'payment',
372-
recipients: [{ address: { address: 'splAddr1' }, amount: { value: '1000', symbol: 'tsol:usdc' } }],
373-
},
374-
});
375-
376-
const utils = new TestBaseTssUtils(mockBitgo, makeCoin('eddsa', 'tsol'), makeWallet('MPCv2'));
377-
sinon.stub(utils, 'deleteSignatureShares').resolves();
378-
sinon.stub(utils, 'getTxRequest').resolves(txRequest);
379-
const signTxRequestStub = sinon.stub(utils, 'signTxRequest').resolves(txRequest);
380-
381-
await utils.recreateTxRequest(txRequestId, 'prv', reqId);
382-
383-
// SPL token: symbol differs from chain name, so tokenName must be set
384-
assert.deepStrictEqual(signTxRequestStub.firstCall.args[0].txParams, {
385-
recipients: [{ address: 'splAddr1', amount: '1000', tokenName: 'tsol:usdc' }],
386-
});
387-
});
388-
389-
it('passes undefined txParams for EdDSA MPCv2 when intent has no recipients', async function () {
390-
const txRequestId = 'tx-req-id-2';
391-
const reqId = new RequestTracer();
392-
const txRequest = buildTxRequest({ txRequestId, intent: { intentType: 'enableToken' } });
393-
394-
const utils = new TestBaseTssUtils(mockBitgo, makeCoin('eddsa'), makeWallet('MPCv2'));
395-
sinon.stub(utils, 'deleteSignatureShares').resolves();
396-
sinon.stub(utils, 'getTxRequest').resolves(txRequest);
397-
const signTxRequestStub = sinon.stub(utils, 'signTxRequest').resolves(txRequest);
398-
399-
await utils.recreateTxRequest(txRequestId, 'prv', reqId);
400-
401-
assert.strictEqual(signTxRequestStub.firstCall.args[0].txParams, undefined);
402-
});
403-
404-
it('passes undefined txParams for ECDSA MPCv2 wallets (guard does not apply)', async function () {
405-
const txRequestId = 'tx-req-id-3';
406-
const reqId = new RequestTracer();
407-
const txRequest = buildTxRequest({
408-
txRequestId,
409-
intent: {
410-
intentType: 'payment',
411-
recipients: [{ address: { address: 'ethAddr1' }, amount: { value: '1000000', symbol: 'eth' } }],
412-
},
413-
});
414-
415-
const utils = new TestBaseTssUtils(mockBitgo, makeCoin('ecdsa'), makeWallet('MPCv2'));
416-
sinon.stub(utils, 'deleteSignatureShares').resolves();
417-
sinon.stub(utils, 'getTxRequest').resolves(txRequest);
418-
const signTxRequestStub = sinon.stub(utils, 'signTxRequest').resolves(txRequest);
419-
420-
await utils.recreateTxRequest(txRequestId, 'prv', reqId);
421-
422359
assert.strictEqual(signTxRequestStub.firstCall.args[0].txParams, undefined);
423360
});
424361
});

0 commit comments

Comments
 (0)