Skip to content

Commit e444156

Browse files
mrdanish26mrdanish26
committed
fix(tss): verify recipients before signing
TICKET: WAL-375
1 parent 4584ba5 commit e444156

7 files changed

Lines changed: 322 additions & 10 deletions

File tree

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

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

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

Lines changed: 115 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,122 @@ 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 userShare = fs.readFileSync(shareFiles[vector.party1]);
322+
const userPrvBase64 = Buffer.from(userShare).toString('base64');
323+
const txRequestWithIntentRecipients = {
324+
...txRequest,
325+
intent: {
326+
intentType: 'contractCall',
327+
recipients: [
328+
{
329+
address: { address: '0xrecipient' },
330+
amount: { value: '1000', symbol: 'hteth' },
331+
},
332+
],
333+
},
334+
};
335+
// Should not throw the recipients guard error — falls back to intent.recipients
336+
await tssUtils
337+
.signTxRequest({
338+
txRequest: txRequestWithIntentRecipients,
339+
prv: userPrvBase64,
340+
reqId,
341+
// no txParams.recipients
342+
})
343+
.should.not.be.rejectedWith(
344+
'Recipient details are required to verify this transaction before signing. Pass txParams with at least one recipient.'
345+
);
346+
});
347+
348+
it('accepts signTxRequest for no-recipient tx types (tokenApproval)', async function () {
349+
const userShare = fs.readFileSync(shareFiles[vector.party1]);
350+
const userPrvBase64 = Buffer.from(userShare).toString('base64');
351+
// Should not throw the recipients guard error — type exemption applies
352+
await tssUtils
353+
.signTxRequest({
354+
txRequest,
355+
prv: userPrvBase64,
356+
reqId,
357+
txParams: { type: 'tokenApproval' },
358+
})
359+
.should.not.be.rejectedWith(
360+
'Recipient details are required to verify this transaction before signing. Pass txParams with at least one recipient.'
361+
);
362+
});
363+
364+
it('accepts signTxRequest for no-recipient tx types (acceleration)', async function () {
365+
const userShare = fs.readFileSync(shareFiles[vector.party1]);
366+
const userPrvBase64 = Buffer.from(userShare).toString('base64');
367+
await tssUtils
368+
.signTxRequest({
369+
txRequest,
370+
prv: userPrvBase64,
371+
reqId,
372+
txParams: { type: 'acceleration' },
373+
})
374+
.should.not.be.rejectedWith(
375+
'Recipient details are required to verify this transaction before signing. Pass txParams with at least one recipient.'
376+
);
377+
});
378+
379+
it('accepts signTxRequest when txParams.recipients takes priority over intent.recipients', async function () {
380+
const userShare = fs.readFileSync(shareFiles[vector.party1]);
381+
const userPrvBase64 = Buffer.from(userShare).toString('base64');
382+
const txRequestWithBothRecipients = {
383+
...txRequest,
384+
intent: {
385+
intentType: 'contractCall',
386+
recipients: [{ address: { address: '0xintentRecipient' }, amount: { value: '9999', symbol: 'hteth' } }],
387+
},
388+
};
389+
await tssUtils
390+
.signTxRequest({
391+
txRequest: txRequestWithBothRecipients,
392+
prv: userPrvBase64,
393+
reqId,
394+
txParams: { recipients: [{ address: '0xrecipient', amount: '1000' }] },
395+
})
396+
.should.not.be.rejectedWith(
397+
'Recipient details are required to verify this transaction before signing. Pass txParams with at least one recipient.'
398+
);
399+
});
285400
});
286401

287402
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
@@ -759,7 +759,12 @@ export interface ITssUtils<KeyShare = EDDSA.KeyShare> {
759759
deleteSignatureShares(txRequestId: string): Promise<SignatureShareRecord[]>;
760760
// eslint-disable-next-line @typescript-eslint/no-explicit-any
761761
sendTxRequest(txRequestId: string): Promise<any>;
762-
recreateTxRequest(txRequestId: string, decryptedPrv: string, reqId: IRequestTracer): Promise<TxRequest>;
762+
recreateTxRequest(
763+
txRequestId: string,
764+
decryptedPrv: string,
765+
reqId: IRequestTracer,
766+
txParams?: TransactionParams
767+
): Promise<TxRequest>;
763768
getTxRequest(txRequestId: string): Promise<TxRequest>;
764769
supportedTxRequestVersions(): TxRequestVersion[];
765770
}

0 commit comments

Comments
 (0)