Skip to content

Commit 691bdbf

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 287564d commit 691bdbf

3 files changed

Lines changed: 133 additions & 17 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/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({

0 commit comments

Comments
 (0)