Skip to content

Commit 63e90fc

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: 94a72c0d-fce8-4672-a6a4-26df25a64dfc Task-Id: 76f4312b-c52c-4478-94f6-457913e8c0b7
1 parent 287564d commit 63e90fc

3 files changed

Lines changed: 132 additions & 15 deletions

File tree

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

Lines changed: 130 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,134 @@ describe('signTxRequest:', function () {
419421
nockPromises[3].isDone().should.be.false();
420422
});
421423

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

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-
}
5074-
50755061
try {
50765062
return await this.tssUtils!.signTxRequest({
50775063
txRequest,

0 commit comments

Comments
 (0)