Skip to content

Commit 63ddf05

Browse files
mrdanish26claude
andcommitted
fix(tss): require txParams with recipients for TSS tx signing
Without this guard, a compromised BitGo API could modify signableHex to redirect funds to an attacker address. The client SDK would sign without detecting the tampering because verifyTransaction() received an empty recipients array and skipped address/amount verification. - Throw early in signRequestBase() when txParams.recipients is absent or empty for RequestType.tx (ecdsaMPCv2 and ecdsa) - Remove the `|| { recipients: [] }` silent fallback so verifyTransaction() always receives caller-supplied params - Add optional txParams to recreateTxRequest() and propagate it to signTxRequest() to keep the pending-approval re-sign path working - Extract recipients from pendingApproval.info.transactionRequest in recreateAndSignTSSTransaction() so that path passes the guard - Update existing tests to supply txParams with recipients and add negative tests for missing/empty recipients cases Ticket: WAL-375 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent e3b914b commit 63ddf05

File tree

7 files changed

+90
-8
lines changed

7 files changed

+90
-8
lines changed

modules/bitgo/test/v2/unit/internal/tssUtils/ecdsa.ts

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -747,6 +747,7 @@ describe('TSS Ecdsa Utils:', async function () {
747747
backupNShare: backupKeyShare.nShares[1],
748748
}),
749749
reqId,
750+
txParams: { recipients: [{ address: '0xrecipient', amount: '1000' }] },
750751
});
751752
signedTxRequest.unsignedTxs.should.deepEqual(txRequest.unsignedTxs);
752753
const userGpgActual = sendShareSpy.getCalls()[0].args[10] as string;
@@ -764,12 +765,42 @@ describe('TSS Ecdsa Utils:', async function () {
764765
backupNShare: backupKeyShare.nShares[1],
765766
}),
766767
reqId,
768+
txParams: { recipients: [{ address: '0xrecipient', amount: '1000' }] },
767769
});
768770
signedTxRequest.unsignedTxs.should.deepEqual(txRequest.unsignedTxs);
769771
const userGpgActual = sendShareSpy.getCalls()[0].args[10] as string;
770772
userGpgActual.should.startWith('-----BEGIN PGP PUBLIC KEY BLOCK-----');
771773
});
772774

775+
it('signTxRequest should fail when txParams is missing', async function () {
776+
await tssUtils
777+
.signTxRequest({
778+
txRequest,
779+
prv: JSON.stringify({
780+
pShare: userKeyShare.pShare,
781+
bitgoNShare: bitgoKeyShare.nShares[1],
782+
backupNShare: backupKeyShare.nShares[1],
783+
}),
784+
reqId,
785+
})
786+
.should.be.rejectedWith('txParams with recipients is required for TSS transaction signing');
787+
});
788+
789+
it('signTxRequest should fail when txParams has empty recipients', async function () {
790+
await tssUtils
791+
.signTxRequest({
792+
txRequest,
793+
prv: JSON.stringify({
794+
pShare: userKeyShare.pShare,
795+
bitgoNShare: bitgoKeyShare.nShares[1],
796+
backupNShare: backupKeyShare.nShares[1],
797+
}),
798+
reqId,
799+
txParams: { recipients: [] },
800+
})
801+
.should.be.rejectedWith('txParams with recipients is required for TSS transaction signing');
802+
});
803+
773804
it('signTxRequest should fail with wrong recipient', async function () {
774805
// To generate these Hex values, we used the bitgo-ui to create a transaction and then
775806
// used the `signableHex` and `serializedTxHex` values from the prebuild.

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

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -193,6 +193,7 @@ describe('signTxRequest:', function () {
193193
txRequest,
194194
prv: userPrvBase64,
195195
reqId,
196+
txParams: { recipients: [{ address: '0xrecipient', amount: '1000' }] },
196197
});
197198
nockPromises[0].isDone().should.be.true();
198199
nockPromises[1].isDone().should.be.true();
@@ -215,6 +216,7 @@ describe('signTxRequest:', function () {
215216
prv: backupPrvBase64,
216217
mpcv2PartyId: 1,
217218
reqId,
219+
txParams: { recipients: [{ address: '0xrecipient', amount: '1000' }] },
218220
});
219221
nockPromises[0].isDone().should.be.true();
220222
nockPromises[1].isDone().should.be.true();
@@ -236,6 +238,7 @@ describe('signTxRequest:', function () {
236238
txRequest,
237239
prv: userPrvBase64,
238240
reqId,
241+
txParams: { recipients: [{ address: '0xrecipient', amount: '1000' }] },
239242
});
240243
nockPromises[0].isDone().should.be.true();
241244
nockPromises[1].isDone().should.be.true();
@@ -257,6 +260,7 @@ describe('signTxRequest:', function () {
257260
txRequest,
258261
prv: userPrvBase64,
259262
reqId,
263+
txParams: { recipients: [{ address: '0xrecipient', amount: '1000' }] },
260264
});
261265
nockPromises[0].isDone().should.be.true();
262266
nockPromises[1].isDone().should.be.true();
@@ -277,11 +281,37 @@ describe('signTxRequest:', function () {
277281
txRequest,
278282
prv: userPrvBase64,
279283
reqId,
284+
txParams: { recipients: [{ address: '0xrecipient', amount: '1000' }] },
280285
})
281286
.should.be.rejectedWith('Too many requests, slow down!');
282287
nockPromises[0].isDone().should.be.true();
283288
nockPromises[1].isDone().should.be.false();
284289
});
290+
291+
it('rejects signTxRequest when txParams is missing', async function () {
292+
const userShare = fs.readFileSync(shareFiles[vector.party1]);
293+
const userPrvBase64 = Buffer.from(userShare).toString('base64');
294+
await tssUtils
295+
.signTxRequest({
296+
txRequest,
297+
prv: userPrvBase64,
298+
reqId,
299+
})
300+
.should.be.rejectedWith('txParams with recipients is required for TSS transaction signing');
301+
});
302+
303+
it('rejects signTxRequest when txParams has empty recipients', async function () {
304+
const userShare = fs.readFileSync(shareFiles[vector.party1]);
305+
const userPrvBase64 = Buffer.from(userShare).toString('base64');
306+
await tssUtils
307+
.signTxRequest({
308+
txRequest,
309+
prv: userPrvBase64,
310+
reqId,
311+
txParams: { recipients: [] },
312+
})
313+
.should.be.rejectedWith('txParams with recipients is required for TSS transaction signing');
314+
});
285315
});
286316

287317
export function getBitGoPartyGpgKeyPrv(key: openpgp.SerializedKeyPair<string>): DklsTypes.PartyGpgKey {

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

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -246,7 +246,9 @@ export class PendingApproval implements IPendingApproval {
246246
}
247247

248248
const decryptedPrv = await this.wallet.getPrv({ walletPassphrase });
249-
const txRequest = await this.tssUtils!.recreateTxRequest(txRequestId, decryptedPrv, reqId);
249+
const pendingApprovalRecipients = this._pendingApproval.info?.transactionRequest?.recipients;
250+
const txParams = pendingApprovalRecipients?.length ? { recipients: pendingApprovalRecipients } : undefined;
251+
const txRequest = await this.tssUtils!.recreateTxRequest(txRequestId, decryptedPrv, reqId, txParams);
250252
if (txRequest.apiVersion === 'lite') {
251253
if (!txRequest.unsignedTxs || txRequest.unsignedTxs.length === 0) {
252254
throw new Error('Unexpected error, no transactions found in txRequest.');

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

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@ import {
3939
TxRequest,
4040
TxRequestVersion,
4141
} from './baseTypes';
42+
import { TransactionParams } from '../../baseCoin/iBaseCoin';
4243
import { GShare, SignShare } from '../../../account-lib/mpc/tss';
4344
import { RequestTracer } from '../util';
4445
import { envRequiresBitgoPubGpgKeyConfig, getBitgoMpcGpgPubKey } from '../../tss/bitgoPubKeys';
@@ -533,11 +534,16 @@ export default class BaseTssUtils<KeyShare> extends MpcUtils implements ITssUtil
533534
* @param {RequestTracer} reqId id tracer.
534535
* @returns {Promise<any>}
535536
*/
536-
async recreateTxRequest(txRequestId: string, decryptedPrv: string, reqId: IRequestTracer): Promise<TxRequest> {
537+
async recreateTxRequest(
538+
txRequestId: string,
539+
decryptedPrv: string,
540+
reqId: IRequestTracer,
541+
txParams?: TransactionParams
542+
): Promise<TxRequest> {
537543
await this.deleteSignatureShares(txRequestId, reqId);
538544
// after delete signatures shares get the tx without them
539545
const txRequest = await getTxRequest(this.bitgo, this.wallet.id(), txRequestId, reqId);
540-
return await this.signTxRequest({ txRequest, prv: decryptedPrv, reqId });
546+
return await this.signTxRequest({ txRequest, prv: decryptedPrv, reqId, txParams });
541547
}
542548

543549
/**

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

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -748,7 +748,12 @@ export interface ITssUtils<KeyShare = EDDSA.KeyShare> {
748748
deleteSignatureShares(txRequestId: string): Promise<SignatureShareRecord[]>;
749749
// eslint-disable-next-line @typescript-eslint/no-explicit-any
750750
sendTxRequest(txRequestId: string): Promise<any>;
751-
recreateTxRequest(txRequestId: string, decryptedPrv: string, reqId: IRequestTracer): Promise<TxRequest>;
751+
recreateTxRequest(
752+
txRequestId: string,
753+
decryptedPrv: string,
754+
reqId: IRequestTracer,
755+
txParams?: TransactionParams
756+
): Promise<TxRequest>;
752757
getTxRequest(txRequestId: string): Promise<TxRequest>;
753758
supportedTxRequestVersions(): TxRequestVersion[];
754759
}

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

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -745,21 +745,25 @@ export class EcdsaUtils extends BaseEcdsaUtils {
745745
const unsignedTx =
746746
txRequest.apiVersion === 'full' ? txRequest.transactions![0].unsignedTx : txRequest.unsignedTxs[0];
747747

748+
if (!params.txParams?.recipients?.length) {
749+
throw new Error('txParams with recipients is required for TSS transaction signing');
750+
}
751+
748752
// For ICP transactions, the HSM signs the serializedTxHex, while the user signs the signableHex separately.
749753
// Verification cannot be performed directly on the signableHex alone. However, we can parse the serializedTxHex
750754
// to regenerate the signableHex and compare it against the provided value for verification.
751755
// In contrast, for other coin families, verification is typically done using just the signableHex.
752756
if (this.baseCoin.getConfig().family === 'icp') {
753757
await this.baseCoin.verifyTransaction({
754758
txPrebuild: { txHex: unsignedTx.serializedTxHex, txInfo: unsignedTx.signableHex },
755-
txParams: params.txParams || { recipients: [] },
759+
txParams: params.txParams,
756760
wallet: this.wallet,
757761
walletType: this.wallet.multisigType(),
758762
});
759763
} else {
760764
await this.baseCoin.verifyTransaction({
761765
txPrebuild: { txHex: unsignedTx.signableHex },
762-
txParams: params.txParams || { recipients: [] },
766+
txParams: params.txParams,
763767
wallet: this.wallet,
764768
walletType: this.wallet.multisigType(),
765769
});

modules/sdk-core/src/bitgo/utils/tss/ecdsa/ecdsaMPCv2.ts

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -736,21 +736,25 @@ export class EcdsaMPCv2Utils extends BaseEcdsaUtils {
736736
const unsignedTx =
737737
txRequest.apiVersion === 'full' ? txRequest.transactions![0].unsignedTx : txRequest.unsignedTxs[0];
738738

739+
if (!params.txParams?.recipients?.length) {
740+
throw new Error('txParams with recipients is required for TSS transaction signing');
741+
}
742+
739743
// For ICP transactions, the HSM signs the serializedTxHex, while the user signs the signableHex separately.
740744
// Verification cannot be performed directly on the signableHex alone. However, we can parse the serializedTxHex
741745
// to regenerate the signableHex and compare it against the provided value for verification.
742746
// In contrast, for other coin families, verification is typically done using just the signableHex.
743747
if (this.baseCoin.getConfig().family === 'icp') {
744748
await this.baseCoin.verifyTransaction({
745749
txPrebuild: { txHex: unsignedTx.serializedTxHex, txInfo: unsignedTx.signableHex },
746-
txParams: params.txParams || { recipients: [] },
750+
txParams: params.txParams,
747751
wallet: this.wallet,
748752
walletType: this.wallet.multisigType(),
749753
});
750754
} else {
751755
await this.baseCoin.verifyTransaction({
752756
txPrebuild: { txHex: unsignedTx.signableHex },
753-
txParams: params.txParams || { recipients: [] },
757+
txParams: params.txParams,
754758
wallet: this.wallet,
755759
walletType: this.wallet.multisigType(),
756760
});

0 commit comments

Comments
 (0)