Skip to content

Commit bef1cda

Browse files
mrdanish26mrdanish26
authored
Merge pull request #8539 from BitGo/fix/WAL-375-recipients-guard-v2
fix(tss): verify recipients before signing
2 parents 3fbaa44 + 1f55399 commit bef1cda

10 files changed

Lines changed: 515 additions & 11 deletions

File tree

‎modules/abstract-eth/src/abstractEthLikeNewCoins.ts‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3109,9 +3109,16 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin {
31093109
txParams.prebuildTx?.consolidateId ||
31103110
txPrebuild?.consolidateId ||
31113111
(txParams.type &&
3112-
['acceleration', 'fillNonce', 'transferToken', 'tokenApproval', 'consolidate', 'bridgeFunds'].includes(
3113-
txParams.type
3114-
))
3112+
[
3113+
'acceleration',
3114+
'fillNonce',
3115+
'transferToken',
3116+
'tokenApproval',
3117+
'consolidate',
3118+
'bridgeFunds',
3119+
'enableToken',
3120+
'customTx',
3121+
].includes(txParams.type))
31153122
)
31163123
) {
31173124
throw new Error('missing txParams');

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

Lines changed: 114 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -749,6 +749,7 @@ describe('TSS Ecdsa Utils:', async function () {
749749
backupNShare: backupKeyShare.nShares[1],
750750
}),
751751
reqId,
752+
txParams: { recipients: [{ address: '0xrecipient', amount: '1000' }] },
752753
});
753754
signedTxRequest.unsignedTxs.should.deepEqual(txRequest.unsignedTxs);
754755
const userGpgActual = sendShareSpy.getCalls()[0].args[10] as string;
@@ -766,12 +767,125 @@ describe('TSS Ecdsa Utils:', async function () {
766767
backupNShare: backupKeyShare.nShares[1],
767768
}),
768769
reqId,
770+
txParams: { recipients: [{ address: '0xrecipient', amount: '1000' }] },
769771
});
770772
signedTxRequest.unsignedTxs.should.deepEqual(txRequest.unsignedTxs);
771773
const userGpgActual = sendShareSpy.getCalls()[0].args[10] as string;
772774
userGpgActual.should.startWith('-----BEGIN PGP PUBLIC KEY BLOCK-----');
773775
});
774776

777+
it('signTxRequest should fail when txParams is missing', async function () {
778+
await tssUtils
779+
.signTxRequest({
780+
txRequest,
781+
prv: JSON.stringify({
782+
pShare: userKeyShare.pShare,
783+
bitgoNShare: bitgoKeyShare.nShares[1],
784+
backupNShare: backupKeyShare.nShares[1],
785+
}),
786+
reqId,
787+
})
788+
.should.be.rejectedWith(
789+
'Recipient details are required to verify this transaction before signing. Pass txParams with at least one recipient.'
790+
);
791+
});
792+
793+
it('signTxRequest should fail when txParams has empty recipients', async function () {
794+
await tssUtils
795+
.signTxRequest({
796+
txRequest,
797+
prv: JSON.stringify({
798+
pShare: userKeyShare.pShare,
799+
bitgoNShare: bitgoKeyShare.nShares[1],
800+
backupNShare: backupKeyShare.nShares[1],
801+
}),
802+
reqId,
803+
txParams: { recipients: [] },
804+
})
805+
.should.be.rejectedWith(
806+
'Recipient details are required to verify this transaction before signing. Pass txParams with at least one recipient.'
807+
);
808+
});
809+
810+
it('signTxRequest should succeed when recipients are only in intent (smart contract interaction)', async function () {
811+
await setupSignTxRequestNocks(false, userSignShare, aShare, dShare, enterpriseData);
812+
const txRequestWithIntentRecipients = {
813+
...txRequest,
814+
intent: {
815+
intentType: 'contractCall',
816+
recipients: [
817+
{
818+
address: { address: '0xrecipient' },
819+
amount: { value: '1000', symbol: 'hteth' },
820+
},
821+
],
822+
},
823+
};
824+
const signedTxRequest = await tssUtils.signTxRequest({
825+
txRequest: txRequestWithIntentRecipients,
826+
prv: JSON.stringify({
827+
pShare: userKeyShare.pShare,
828+
bitgoNShare: bitgoKeyShare.nShares[1],
829+
backupNShare: backupKeyShare.nShares[1],
830+
}),
831+
reqId,
832+
// no txParams.recipients — should fall back to intent.recipients
833+
});
834+
signedTxRequest.unsignedTxs.should.deepEqual(txRequest.unsignedTxs);
835+
});
836+
837+
it('signTxRequest should succeed for no-recipient tx types (tokenApproval)', async function () {
838+
await setupSignTxRequestNocks(false, userSignShare, aShare, dShare, enterpriseData);
839+
const signedTxRequest = await tssUtils.signTxRequest({
840+
txRequest,
841+
prv: JSON.stringify({
842+
pShare: userKeyShare.pShare,
843+
bitgoNShare: bitgoKeyShare.nShares[1],
844+
backupNShare: backupKeyShare.nShares[1],
845+
}),
846+
reqId,
847+
txParams: { type: 'tokenApproval' },
848+
});
849+
signedTxRequest.unsignedTxs.should.deepEqual(txRequest.unsignedTxs);
850+
});
851+
852+
it('signTxRequest should succeed for no-recipient tx types (acceleration)', async function () {
853+
await setupSignTxRequestNocks(false, userSignShare, aShare, dShare, enterpriseData);
854+
const signedTxRequest = await tssUtils.signTxRequest({
855+
txRequest,
856+
prv: JSON.stringify({
857+
pShare: userKeyShare.pShare,
858+
bitgoNShare: bitgoKeyShare.nShares[1],
859+
backupNShare: backupKeyShare.nShares[1],
860+
}),
861+
reqId,
862+
txParams: { type: 'acceleration' },
863+
});
864+
signedTxRequest.unsignedTxs.should.deepEqual(txRequest.unsignedTxs);
865+
});
866+
867+
it('signTxRequest should prefer txParams.recipients over intent.recipients when both are present', async function () {
868+
await setupSignTxRequestNocks(false, userSignShare, aShare, dShare, enterpriseData);
869+
const txRequestWithBothRecipients = {
870+
...txRequest,
871+
intent: {
872+
intentType: 'contractCall',
873+
recipients: [{ address: { address: '0xintentRecipient' }, amount: { value: '9999', symbol: 'hteth' } }],
874+
},
875+
};
876+
const signedTxRequest = await tssUtils.signTxRequest({
877+
txRequest: txRequestWithBothRecipients,
878+
prv: JSON.stringify({
879+
pShare: userKeyShare.pShare,
880+
bitgoNShare: bitgoKeyShare.nShares[1],
881+
backupNShare: backupKeyShare.nShares[1],
882+
}),
883+
reqId,
884+
txParams: { recipients: [{ address: '0xrecipient', amount: '1000' }] },
885+
});
886+
signedTxRequest.unsignedTxs.should.deepEqual(txRequest.unsignedTxs);
887+
});
888+
775889
it('signTxRequest should fail with wrong recipient', async function () {
776890
// To generate these Hex values, we used the bitgo-ui to create a transaction and then
777891
// used the `signableHex` and `serializedTxHex` values from the prebuild.

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

Lines changed: 190 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,197 @@ 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(
301+
'Recipient details are required to verify this transaction before signing. Pass txParams with at least one recipient.'
302+
);
303+
});
304+
305+
it('rejects signTxRequest when txParams has empty recipients', async function () {
306+
const userShare = fs.readFileSync(shareFiles[vector.party1]);
307+
const userPrvBase64 = Buffer.from(userShare).toString('base64');
308+
await tssUtils
309+
.signTxRequest({
310+
txRequest,
311+
prv: userPrvBase64,
312+
reqId,
313+
txParams: { recipients: [] },
314+
})
315+
.should.be.rejectedWith(
316+
'Recipient details are required to verify this transaction before signing. Pass txParams with at least one recipient.'
317+
);
318+
});
319+
320+
it('accepts signTxRequest when recipients are only in intent (smart contract interaction)', async function () {
321+
const txRequestWithIntentRecipients = {
322+
...txRequest,
323+
intent: {
324+
intentType: 'contractCall',
325+
recipients: [
326+
{
327+
address: { address: '0xrecipient' },
328+
amount: { value: '1000', symbol: 'hteth' },
329+
},
330+
],
331+
},
332+
};
333+
const nockPromises = [
334+
await nockTxRequestResponseSignatureShareRoundOne(bitgoParty, txRequestWithIntentRecipients, bitgoGpgKey),
335+
await nockTxRequestResponseSignatureShareRoundTwo(bitgoParty, txRequestWithIntentRecipients, bitgoGpgKey),
336+
await nockTxRequestResponseSignatureShareRoundThree(txRequestWithIntentRecipients),
337+
await nockSendTxRequest(txRequestWithIntentRecipients),
338+
];
339+
await Promise.all(nockPromises);
340+
341+
const userShare = fs.readFileSync(shareFiles[vector.party1]);
342+
const userPrvBase64 = Buffer.from(userShare).toString('base64');
343+
// Falls back to intent.recipients — guard should pass and signing should complete
344+
await tssUtils.signTxRequest({
345+
txRequest: txRequestWithIntentRecipients,
346+
prv: userPrvBase64,
347+
reqId,
348+
// no txParams.recipients
349+
});
350+
nockPromises[0].isDone().should.be.true();
351+
nockPromises[1].isDone().should.be.true();
352+
nockPromises[2].isDone().should.be.true();
353+
});
354+
355+
it('accepts signTxRequest for no-recipient tx types (tokenApproval)', async function () {
356+
const nockPromises = [
357+
await nockTxRequestResponseSignatureShareRoundOne(bitgoParty, txRequest, bitgoGpgKey),
358+
await nockTxRequestResponseSignatureShareRoundTwo(bitgoParty, txRequest, bitgoGpgKey),
359+
await nockTxRequestResponseSignatureShareRoundThree(txRequest),
360+
await nockSendTxRequest(txRequest),
361+
];
362+
await Promise.all(nockPromises);
363+
364+
const userShare = fs.readFileSync(shareFiles[vector.party1]);
365+
const userPrvBase64 = Buffer.from(userShare).toString('base64');
366+
// Type exemption applies — guard passes without recipients
367+
await tssUtils.signTxRequest({
368+
txRequest,
369+
prv: userPrvBase64,
370+
reqId,
371+
txParams: { type: 'tokenApproval' },
372+
});
373+
nockPromises[0].isDone().should.be.true();
374+
nockPromises[1].isDone().should.be.true();
375+
nockPromises[2].isDone().should.be.true();
376+
});
377+
378+
it('accepts signTxRequest for no-recipient tx types (acceleration)', async function () {
379+
const nockPromises = [
380+
await nockTxRequestResponseSignatureShareRoundOne(bitgoParty, txRequest, bitgoGpgKey),
381+
await nockTxRequestResponseSignatureShareRoundTwo(bitgoParty, txRequest, bitgoGpgKey),
382+
await nockTxRequestResponseSignatureShareRoundThree(txRequest),
383+
await nockSendTxRequest(txRequest),
384+
];
385+
await Promise.all(nockPromises);
386+
387+
const userShare = fs.readFileSync(shareFiles[vector.party1]);
388+
const userPrvBase64 = Buffer.from(userShare).toString('base64');
389+
await tssUtils.signTxRequest({
390+
txRequest,
391+
prv: userPrvBase64,
392+
reqId,
393+
txParams: { type: 'acceleration' },
394+
});
395+
nockPromises[0].isDone().should.be.true();
396+
nockPromises[1].isDone().should.be.true();
397+
nockPromises[2].isDone().should.be.true();
398+
});
399+
400+
it('accepts signTxRequest for no-recipient tx types (customTx)', async function () {
401+
const nockPromises = [
402+
await nockTxRequestResponseSignatureShareRoundOne(bitgoParty, txRequest, bitgoGpgKey),
403+
await nockTxRequestResponseSignatureShareRoundTwo(bitgoParty, txRequest, bitgoGpgKey),
404+
await nockTxRequestResponseSignatureShareRoundThree(txRequest),
405+
await nockSendTxRequest(txRequest),
406+
];
407+
await Promise.all(nockPromises);
408+
409+
const userShare = fs.readFileSync(shareFiles[vector.party1]);
410+
const userPrvBase64 = Buffer.from(userShare).toString('base64');
411+
// DeFi/WalletConnect smart contract interactions have no traditional recipients
412+
await tssUtils.signTxRequest({
413+
txRequest,
414+
prv: userPrvBase64,
415+
reqId,
416+
txParams: { type: 'customTx' },
417+
});
418+
nockPromises[0].isDone().should.be.true();
419+
nockPromises[1].isDone().should.be.true();
420+
nockPromises[2].isDone().should.be.true();
421+
});
422+
423+
it('accepts signTxRequest for no-recipient tx types (enableToken)', async function () {
424+
const nockPromises = [
425+
await nockTxRequestResponseSignatureShareRoundOne(bitgoParty, txRequest, bitgoGpgKey),
426+
await nockTxRequestResponseSignatureShareRoundTwo(bitgoParty, txRequest, bitgoGpgKey),
427+
await nockTxRequestResponseSignatureShareRoundThree(txRequest),
428+
await nockSendTxRequest(txRequest),
429+
];
430+
await Promise.all(nockPromises);
431+
432+
const userShare = fs.readFileSync(shareFiles[vector.party1]);
433+
const userPrvBase64 = Buffer.from(userShare).toString('base64');
434+
// TSS wallets do not populate recipients for token enablement — exemption must apply
435+
await tssUtils.signTxRequest({
436+
txRequest,
437+
prv: userPrvBase64,
438+
reqId,
439+
txParams: { type: 'enableToken' },
440+
});
441+
nockPromises[0].isDone().should.be.true();
442+
nockPromises[1].isDone().should.be.true();
443+
nockPromises[2].isDone().should.be.true();
444+
});
445+
446+
it('accepts signTxRequest when txParams.recipients takes priority over intent.recipients', async function () {
447+
const txRequestWithBothRecipients = {
448+
...txRequest,
449+
intent: {
450+
intentType: 'contractCall',
451+
recipients: [{ address: { address: '0xintentRecipient' }, amount: { value: '9999', symbol: 'hteth' } }],
452+
},
453+
};
454+
const nockPromises = [
455+
await nockTxRequestResponseSignatureShareRoundOne(bitgoParty, txRequestWithBothRecipients, bitgoGpgKey),
456+
await nockTxRequestResponseSignatureShareRoundTwo(bitgoParty, txRequestWithBothRecipients, bitgoGpgKey),
457+
await nockTxRequestResponseSignatureShareRoundThree(txRequestWithBothRecipients),
458+
await nockSendTxRequest(txRequestWithBothRecipients),
459+
];
460+
await Promise.all(nockPromises);
461+
462+
const userShare = fs.readFileSync(shareFiles[vector.party1]);
463+
const userPrvBase64 = Buffer.from(userShare).toString('base64');
464+
// txParams.recipients takes priority — guard passes and signing completes
465+
await tssUtils.signTxRequest({
466+
txRequest: txRequestWithBothRecipients,
467+
prv: userPrvBase64,
468+
reqId,
469+
txParams: { recipients: [{ address: '0xrecipient', amount: '1000' }] },
470+
});
471+
nockPromises[0].isDone().should.be.true();
472+
nockPromises[1].isDone().should.be.true();
473+
nockPromises[2].isDone().should.be.true();
474+
});
285475
});
286476

287477
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
@@ -250,7 +250,9 @@ export class PendingApproval implements IPendingApproval {
250250
}
251251

252252
const decryptedPrv = await this.wallet.getPrv({ walletPassphrase });
253-
const txRequest = await this.tssUtils!.recreateTxRequest(txRequestId, decryptedPrv, reqId);
253+
const pendingApprovalRecipients = this._pendingApproval.info?.transactionRequest?.recipients;
254+
const txParams = pendingApprovalRecipients?.length ? { recipients: pendingApprovalRecipients } : undefined;
255+
const txRequest = await this.tssUtils!.recreateTxRequest(txRequestId, decryptedPrv, reqId, txParams);
254256
if (txRequest.apiVersion === 'lite') {
255257
if (!txRequest.unsignedTxs || txRequest.unsignedTxs.length === 0) {
256258
throw new Error('Unexpected error, no transactions found in txRequest.');

0 commit comments

Comments
 (0)