From 31cbea7fe2aa0a12279147b410dc748d322aa4a9 Mon Sep 17 00:00:00 2001 From: mrdanish26 Date: Thu, 16 Apr 2026 17:13:00 -0700 Subject: [PATCH 1/2] fix(sdk-core): verify recipients before signing to prevent signableHex substitution TICKET: WAL-375 --- .../test/v2/unit/internal/tssUtils/ecdsa.ts | 114 +++++++++++++++++ .../tssUtils/ecdsaMPCv2/signTxRequest.ts | 115 ++++++++++++++++++ .../bitgo/pendingApproval/pendingApproval.ts | 4 +- .../src/bitgo/utils/tss/baseTSSUtils.ts | 10 +- .../sdk-core/src/bitgo/utils/tss/baseTypes.ts | 7 +- .../src/bitgo/utils/tss/ecdsa/base.ts | 53 +++++++- .../src/bitgo/utils/tss/ecdsa/ecdsa.ts | 8 +- .../src/bitgo/utils/tss/ecdsa/ecdsaMPCv2.ts | 8 +- 8 files changed, 307 insertions(+), 12 deletions(-) diff --git a/modules/bitgo/test/v2/unit/internal/tssUtils/ecdsa.ts b/modules/bitgo/test/v2/unit/internal/tssUtils/ecdsa.ts index 9786e02c2a..123430f937 100644 --- a/modules/bitgo/test/v2/unit/internal/tssUtils/ecdsa.ts +++ b/modules/bitgo/test/v2/unit/internal/tssUtils/ecdsa.ts @@ -749,6 +749,7 @@ describe('TSS Ecdsa Utils:', async function () { backupNShare: backupKeyShare.nShares[1], }), reqId, + txParams: { recipients: [{ address: '0xrecipient', amount: '1000' }] }, }); signedTxRequest.unsignedTxs.should.deepEqual(txRequest.unsignedTxs); const userGpgActual = sendShareSpy.getCalls()[0].args[10] as string; @@ -766,12 +767,125 @@ describe('TSS Ecdsa Utils:', async function () { backupNShare: backupKeyShare.nShares[1], }), reqId, + txParams: { recipients: [{ address: '0xrecipient', amount: '1000' }] }, }); signedTxRequest.unsignedTxs.should.deepEqual(txRequest.unsignedTxs); const userGpgActual = sendShareSpy.getCalls()[0].args[10] as string; userGpgActual.should.startWith('-----BEGIN PGP PUBLIC KEY BLOCK-----'); }); + it('signTxRequest should fail when txParams is missing', async function () { + await tssUtils + .signTxRequest({ + txRequest, + prv: JSON.stringify({ + pShare: userKeyShare.pShare, + bitgoNShare: bitgoKeyShare.nShares[1], + backupNShare: backupKeyShare.nShares[1], + }), + reqId, + }) + .should.be.rejectedWith( + 'Recipient details are required to verify this transaction before signing. Pass txParams with at least one recipient.' + ); + }); + + it('signTxRequest should fail when txParams has empty recipients', async function () { + await tssUtils + .signTxRequest({ + txRequest, + prv: JSON.stringify({ + pShare: userKeyShare.pShare, + bitgoNShare: bitgoKeyShare.nShares[1], + backupNShare: backupKeyShare.nShares[1], + }), + reqId, + txParams: { recipients: [] }, + }) + .should.be.rejectedWith( + 'Recipient details are required to verify this transaction before signing. Pass txParams with at least one recipient.' + ); + }); + + it('signTxRequest should succeed when recipients are only in intent (smart contract interaction)', async function () { + await setupSignTxRequestNocks(false, userSignShare, aShare, dShare, enterpriseData); + const txRequestWithIntentRecipients = { + ...txRequest, + intent: { + intentType: 'contractCall', + recipients: [ + { + address: { address: '0xrecipient' }, + amount: { value: '1000', symbol: 'hteth' }, + }, + ], + }, + }; + const signedTxRequest = await tssUtils.signTxRequest({ + txRequest: txRequestWithIntentRecipients, + prv: JSON.stringify({ + pShare: userKeyShare.pShare, + bitgoNShare: bitgoKeyShare.nShares[1], + backupNShare: backupKeyShare.nShares[1], + }), + reqId, + // no txParams.recipients — should fall back to intent.recipients + }); + signedTxRequest.unsignedTxs.should.deepEqual(txRequest.unsignedTxs); + }); + + it('signTxRequest should succeed for no-recipient tx types (tokenApproval)', async function () { + await setupSignTxRequestNocks(false, userSignShare, aShare, dShare, enterpriseData); + const signedTxRequest = await tssUtils.signTxRequest({ + txRequest, + prv: JSON.stringify({ + pShare: userKeyShare.pShare, + bitgoNShare: bitgoKeyShare.nShares[1], + backupNShare: backupKeyShare.nShares[1], + }), + reqId, + txParams: { type: 'tokenApproval' }, + }); + signedTxRequest.unsignedTxs.should.deepEqual(txRequest.unsignedTxs); + }); + + it('signTxRequest should succeed for no-recipient tx types (acceleration)', async function () { + await setupSignTxRequestNocks(false, userSignShare, aShare, dShare, enterpriseData); + const signedTxRequest = await tssUtils.signTxRequest({ + txRequest, + prv: JSON.stringify({ + pShare: userKeyShare.pShare, + bitgoNShare: bitgoKeyShare.nShares[1], + backupNShare: backupKeyShare.nShares[1], + }), + reqId, + txParams: { type: 'acceleration' }, + }); + signedTxRequest.unsignedTxs.should.deepEqual(txRequest.unsignedTxs); + }); + + it('signTxRequest should prefer txParams.recipients over intent.recipients when both are present', async function () { + await setupSignTxRequestNocks(false, userSignShare, aShare, dShare, enterpriseData); + const txRequestWithBothRecipients = { + ...txRequest, + intent: { + intentType: 'contractCall', + recipients: [{ address: { address: '0xintentRecipient' }, amount: { value: '9999', symbol: 'hteth' } }], + }, + }; + const signedTxRequest = await tssUtils.signTxRequest({ + txRequest: txRequestWithBothRecipients, + prv: JSON.stringify({ + pShare: userKeyShare.pShare, + bitgoNShare: bitgoKeyShare.nShares[1], + backupNShare: backupKeyShare.nShares[1], + }), + reqId, + txParams: { recipients: [{ address: '0xrecipient', amount: '1000' }] }, + }); + signedTxRequest.unsignedTxs.should.deepEqual(txRequest.unsignedTxs); + }); + it('signTxRequest should fail with wrong recipient', async function () { // To generate these Hex values, we used the bitgo-ui to create a transaction and then // used the `signableHex` and `serializedTxHex` values from the prebuild. diff --git a/modules/bitgo/test/v2/unit/internal/tssUtils/ecdsaMPCv2/signTxRequest.ts b/modules/bitgo/test/v2/unit/internal/tssUtils/ecdsaMPCv2/signTxRequest.ts index 9b2a43be4e..de5f65516e 100644 --- a/modules/bitgo/test/v2/unit/internal/tssUtils/ecdsaMPCv2/signTxRequest.ts +++ b/modules/bitgo/test/v2/unit/internal/tssUtils/ecdsaMPCv2/signTxRequest.ts @@ -193,6 +193,7 @@ describe('signTxRequest:', function () { txRequest, prv: userPrvBase64, reqId, + txParams: { recipients: [{ address: '0xrecipient', amount: '1000' }] }, }); nockPromises[0].isDone().should.be.true(); nockPromises[1].isDone().should.be.true(); @@ -215,6 +216,7 @@ describe('signTxRequest:', function () { prv: backupPrvBase64, mpcv2PartyId: 1, reqId, + txParams: { recipients: [{ address: '0xrecipient', amount: '1000' }] }, }); nockPromises[0].isDone().should.be.true(); nockPromises[1].isDone().should.be.true(); @@ -236,6 +238,7 @@ describe('signTxRequest:', function () { txRequest, prv: userPrvBase64, reqId, + txParams: { recipients: [{ address: '0xrecipient', amount: '1000' }] }, }); nockPromises[0].isDone().should.be.true(); nockPromises[1].isDone().should.be.true(); @@ -257,6 +260,7 @@ describe('signTxRequest:', function () { txRequest, prv: userPrvBase64, reqId, + txParams: { recipients: [{ address: '0xrecipient', amount: '1000' }] }, }); nockPromises[0].isDone().should.be.true(); nockPromises[1].isDone().should.be.true(); @@ -277,11 +281,122 @@ describe('signTxRequest:', function () { txRequest, prv: userPrvBase64, reqId, + txParams: { recipients: [{ address: '0xrecipient', amount: '1000' }] }, }) .should.be.rejectedWith('Too many requests, slow down!'); nockPromises[0].isDone().should.be.true(); nockPromises[1].isDone().should.be.false(); }); + + it('rejects signTxRequest when txParams is missing', async function () { + const userShare = fs.readFileSync(shareFiles[vector.party1]); + const userPrvBase64 = Buffer.from(userShare).toString('base64'); + await tssUtils + .signTxRequest({ + txRequest, + prv: userPrvBase64, + reqId, + }) + .should.be.rejectedWith( + 'Recipient details are required to verify this transaction before signing. Pass txParams with at least one recipient.' + ); + }); + + it('rejects signTxRequest when txParams has empty recipients', async function () { + const userShare = fs.readFileSync(shareFiles[vector.party1]); + const userPrvBase64 = Buffer.from(userShare).toString('base64'); + await tssUtils + .signTxRequest({ + txRequest, + prv: userPrvBase64, + reqId, + txParams: { recipients: [] }, + }) + .should.be.rejectedWith( + 'Recipient details are required to verify this transaction before signing. Pass txParams with at least one recipient.' + ); + }); + + it('accepts signTxRequest when recipients are only in intent (smart contract interaction)', async function () { + const userShare = fs.readFileSync(shareFiles[vector.party1]); + const userPrvBase64 = Buffer.from(userShare).toString('base64'); + const txRequestWithIntentRecipients = { + ...txRequest, + intent: { + intentType: 'contractCall', + recipients: [ + { + address: { address: '0xrecipient' }, + amount: { value: '1000', symbol: 'hteth' }, + }, + ], + }, + }; + // Should not throw the recipients guard error — falls back to intent.recipients + await tssUtils + .signTxRequest({ + txRequest: txRequestWithIntentRecipients, + prv: userPrvBase64, + reqId, + // no txParams.recipients + }) + .should.not.be.rejectedWith( + 'Recipient details are required to verify this transaction before signing. Pass txParams with at least one recipient.' + ); + }); + + it('accepts signTxRequest for no-recipient tx types (tokenApproval)', async function () { + const userShare = fs.readFileSync(shareFiles[vector.party1]); + const userPrvBase64 = Buffer.from(userShare).toString('base64'); + // Should not throw the recipients guard error — type exemption applies + await tssUtils + .signTxRequest({ + txRequest, + prv: userPrvBase64, + reqId, + txParams: { type: 'tokenApproval' }, + }) + .should.not.be.rejectedWith( + 'Recipient details are required to verify this transaction before signing. Pass txParams with at least one recipient.' + ); + }); + + it('accepts signTxRequest for no-recipient tx types (acceleration)', async function () { + const userShare = fs.readFileSync(shareFiles[vector.party1]); + const userPrvBase64 = Buffer.from(userShare).toString('base64'); + await tssUtils + .signTxRequest({ + txRequest, + prv: userPrvBase64, + reqId, + txParams: { type: 'acceleration' }, + }) + .should.not.be.rejectedWith( + 'Recipient details are required to verify this transaction before signing. Pass txParams with at least one recipient.' + ); + }); + + it('accepts signTxRequest when txParams.recipients takes priority over intent.recipients', async function () { + const userShare = fs.readFileSync(shareFiles[vector.party1]); + const userPrvBase64 = Buffer.from(userShare).toString('base64'); + const txRequestWithBothRecipients = { + ...txRequest, + intent: { + intentType: 'contractCall', + recipients: [{ address: { address: '0xintentRecipient' }, amount: { value: '9999', symbol: 'hteth' } }], + }, + }; + await tssUtils + .signTxRequest({ + txRequest: txRequestWithBothRecipients, + prv: userPrvBase64, + reqId, + txParams: { recipients: [{ address: '0xrecipient', amount: '1000' }] }, + }) + .should.not.be.rejectedWith( + 'Recipient details are required to verify this transaction before signing. Pass txParams with at least one recipient.' + ); + }); }); export function getBitGoPartyGpgKeyPrv(key: openpgp.SerializedKeyPair): DklsTypes.PartyGpgKey { diff --git a/modules/sdk-core/src/bitgo/pendingApproval/pendingApproval.ts b/modules/sdk-core/src/bitgo/pendingApproval/pendingApproval.ts index 5ca48252d4..510539e4a2 100644 --- a/modules/sdk-core/src/bitgo/pendingApproval/pendingApproval.ts +++ b/modules/sdk-core/src/bitgo/pendingApproval/pendingApproval.ts @@ -250,7 +250,9 @@ export class PendingApproval implements IPendingApproval { } const decryptedPrv = await this.wallet.getPrv({ walletPassphrase }); - const txRequest = await this.tssUtils!.recreateTxRequest(txRequestId, decryptedPrv, reqId); + const pendingApprovalRecipients = this._pendingApproval.info?.transactionRequest?.recipients; + const txParams = pendingApprovalRecipients?.length ? { recipients: pendingApprovalRecipients } : undefined; + const txRequest = await this.tssUtils!.recreateTxRequest(txRequestId, decryptedPrv, reqId, txParams); if (txRequest.apiVersion === 'lite') { if (!txRequest.unsignedTxs || txRequest.unsignedTxs.length === 0) { throw new Error('Unexpected error, no transactions found in txRequest.'); diff --git a/modules/sdk-core/src/bitgo/utils/tss/baseTSSUtils.ts b/modules/sdk-core/src/bitgo/utils/tss/baseTSSUtils.ts index e4dfebe65c..ab9a0bf8da 100644 --- a/modules/sdk-core/src/bitgo/utils/tss/baseTSSUtils.ts +++ b/modules/sdk-core/src/bitgo/utils/tss/baseTSSUtils.ts @@ -39,6 +39,7 @@ import { TxRequest, TxRequestVersion, } from './baseTypes'; +import { TransactionParams } from '../../baseCoin/iBaseCoin'; import { GShare, SignShare } from '../../../account-lib/mpc/tss'; import { RequestTracer } from '../util'; import { envRequiresBitgoPubGpgKeyConfig, getBitgoMpcGpgPubKey } from '../../tss/bitgoPubKeys'; @@ -538,11 +539,16 @@ export default class BaseTssUtils extends MpcUtils implements ITssUtil * @param {RequestTracer} reqId id tracer. * @returns {Promise} */ - async recreateTxRequest(txRequestId: string, decryptedPrv: string, reqId: IRequestTracer): Promise { + async recreateTxRequest( + txRequestId: string, + decryptedPrv: string, + reqId: IRequestTracer, + txParams?: TransactionParams + ): Promise { await this.deleteSignatureShares(txRequestId, reqId); // after delete signatures shares get the tx without them const txRequest = await getTxRequest(this.bitgo, this.wallet.id(), txRequestId, reqId); - return await this.signTxRequest({ txRequest, prv: decryptedPrv, reqId }); + return await this.signTxRequest({ txRequest, prv: decryptedPrv, reqId, txParams }); } /** diff --git a/modules/sdk-core/src/bitgo/utils/tss/baseTypes.ts b/modules/sdk-core/src/bitgo/utils/tss/baseTypes.ts index f773e370a7..4b173123b0 100644 --- a/modules/sdk-core/src/bitgo/utils/tss/baseTypes.ts +++ b/modules/sdk-core/src/bitgo/utils/tss/baseTypes.ts @@ -760,7 +760,12 @@ export interface ITssUtils { deleteSignatureShares(txRequestId: string): Promise; // eslint-disable-next-line @typescript-eslint/no-explicit-any sendTxRequest(txRequestId: string): Promise; - recreateTxRequest(txRequestId: string, decryptedPrv: string, reqId: IRequestTracer): Promise; + recreateTxRequest( + txRequestId: string, + decryptedPrv: string, + reqId: IRequestTracer, + txParams?: TransactionParams + ): Promise; getTxRequest(txRequestId: string): Promise; supportedTxRequestVersions(): TxRequestVersion[]; } diff --git a/modules/sdk-core/src/bitgo/utils/tss/ecdsa/base.ts b/modules/sdk-core/src/bitgo/utils/tss/ecdsa/base.ts index d6512327fb..46b9fcd6c3 100644 --- a/modules/sdk-core/src/bitgo/utils/tss/ecdsa/base.ts +++ b/modules/sdk-core/src/bitgo/utils/tss/ecdsa/base.ts @@ -1,12 +1,61 @@ import { secp256k1 } from '@noble/curves/secp256k1'; -import { IBaseCoin } from '../../../baseCoin'; +import { IBaseCoin, TransactionParams } from '../../../baseCoin'; import baseTSSUtils from '../baseTSSUtils'; import { KeyShare } from './types'; -import { BackupGpgKey } from '../baseTypes'; +import { BackupGpgKey, PopulatedIntent, TxRequest } from '../baseTypes'; import { generateGPGKeyPair } from '../../opengpgUtils'; import { BitGoBase } from '../../../bitgoBase'; import { IWallet } from '../../../wallet'; +import { InvalidTransactionError } from '../../../errors'; + +/** + * Transaction types that legitimately carry no explicit recipients. + * verifyTransaction handles no-recipient validation for these internally. + * Mirrors the bypass list in abstractEthLikeNewCoins.ts verifyTssTransaction. + */ +export const NO_RECIPIENT_TX_TYPES = new Set([ + 'acceleration', + 'fillNonce', + 'transferToken', + 'tokenApproval', + 'consolidate', + 'bridgeFunds', +]); + +/** + * Resolves the effective txParams for TSS signing recipient verification. + * + * For smart contract interactions, recipients live in txRequest.intent.recipients + * (native amount = 0, so buildParams is empty). Falls back to intent recipients + * mapped to ITransactionRecipient shape when txParams.recipients is absent. + * + * Throws InvalidTransactionError if no recipients can be resolved and the + * transaction type is not a known no-recipient type. + */ +export function resolveEffectiveTxParams( + txRequest: TxRequest, + txParams: TransactionParams | undefined +): TransactionParams { + const intentRecipients = (txRequest.intent as PopulatedIntent)?.recipients?.map((intentRecipient) => ({ + address: intentRecipient.address.address, + amount: intentRecipient.amount.value, + data: intentRecipient.data, + })); + + const effectiveTxParams: TransactionParams = { + ...txParams, + recipients: txParams?.recipients?.length ? txParams.recipients : intentRecipients, + }; + + if (!effectiveTxParams.recipients?.length && !NO_RECIPIENT_TX_TYPES.has(effectiveTxParams.type ?? '')) { + throw new InvalidTransactionError( + 'Recipient details are required to verify this transaction before signing. Pass txParams with at least one recipient.' + ); + } + + return effectiveTxParams; +} /** @inheritdoc */ export class BaseEcdsaUtils extends baseTSSUtils { diff --git a/modules/sdk-core/src/bitgo/utils/tss/ecdsa/ecdsa.ts b/modules/sdk-core/src/bitgo/utils/tss/ecdsa/ecdsa.ts index 41391d7232..614ee75f3f 100644 --- a/modules/sdk-core/src/bitgo/utils/tss/ecdsa/ecdsa.ts +++ b/modules/sdk-core/src/bitgo/utils/tss/ecdsa/ecdsa.ts @@ -45,7 +45,7 @@ import { TssEcdsaStep2ReturnMessage, TxRequestChallengeResponse, } from '../../../tss/types'; -import { BaseEcdsaUtils } from './base'; +import { BaseEcdsaUtils, resolveEffectiveTxParams } from './base'; import { IRequestTracer } from '../../../../api'; const encryptNShare = ECDSAMethods.encryptNShare; @@ -745,6 +745,8 @@ export class EcdsaUtils extends BaseEcdsaUtils { const unsignedTx = txRequest.apiVersion === 'full' ? txRequest.transactions![0].unsignedTx : txRequest.unsignedTxs[0]; + const effectiveTxParams = resolveEffectiveTxParams(txRequest, params.txParams); + // For ICP transactions, the HSM signs the serializedTxHex, while the user signs the signableHex separately. // Verification cannot be performed directly on the signableHex alone. However, we can parse the serializedTxHex // to regenerate the signableHex and compare it against the provided value for verification. @@ -752,14 +754,14 @@ export class EcdsaUtils extends BaseEcdsaUtils { if (this.baseCoin.getConfig().family === 'icp') { await this.baseCoin.verifyTransaction({ txPrebuild: { txHex: unsignedTx.serializedTxHex, txInfo: unsignedTx.signableHex }, - txParams: params.txParams || { recipients: [] }, + txParams: effectiveTxParams, wallet: this.wallet, walletType: this.wallet.multisigType(), }); } else { await this.baseCoin.verifyTransaction({ txPrebuild: { txHex: unsignedTx.signableHex }, - txParams: params.txParams || { recipients: [] }, + txParams: effectiveTxParams, wallet: this.wallet, walletType: this.wallet.multisigType(), }); diff --git a/modules/sdk-core/src/bitgo/utils/tss/ecdsa/ecdsaMPCv2.ts b/modules/sdk-core/src/bitgo/utils/tss/ecdsa/ecdsaMPCv2.ts index 129877a4af..eaeedd5f75 100644 --- a/modules/sdk-core/src/bitgo/utils/tss/ecdsa/ecdsaMPCv2.ts +++ b/modules/sdk-core/src/bitgo/utils/tss/ecdsa/ecdsaMPCv2.ts @@ -46,7 +46,7 @@ import { TSSParamsWithPrv, TxRequest, } from '../baseTypes'; -import { BaseEcdsaUtils } from './base'; +import { BaseEcdsaUtils, resolveEffectiveTxParams } from './base'; import { EcdsaMPCv2KeyGenSendFn, KeyGenSenderForEnterprise } from './ecdsaMPCv2KeyGenSender'; import { envRequiresBitgoPubGpgKeyConfig, isBitgoMpcPubKey } from '../../../tss/bitgoPubKeys'; @@ -737,6 +737,8 @@ export class EcdsaMPCv2Utils extends BaseEcdsaUtils { const unsignedTx = txRequest.apiVersion === 'full' ? txRequest.transactions![0].unsignedTx : txRequest.unsignedTxs[0]; + const effectiveTxParams = resolveEffectiveTxParams(txRequest, params.txParams); + // For ICP transactions, the HSM signs the serializedTxHex, while the user signs the signableHex separately. // Verification cannot be performed directly on the signableHex alone. However, we can parse the serializedTxHex // to regenerate the signableHex and compare it against the provided value for verification. @@ -744,14 +746,14 @@ export class EcdsaMPCv2Utils extends BaseEcdsaUtils { if (this.baseCoin.getConfig().family === 'icp') { await this.baseCoin.verifyTransaction({ txPrebuild: { txHex: unsignedTx.serializedTxHex, txInfo: unsignedTx.signableHex }, - txParams: params.txParams || { recipients: [] }, + txParams: effectiveTxParams, wallet: this.wallet, walletType: this.wallet.multisigType(), }); } else { await this.baseCoin.verifyTransaction({ txPrebuild: { txHex: unsignedTx.signableHex }, - txParams: params.txParams || { recipients: [] }, + txParams: effectiveTxParams, wallet: this.wallet, walletType: this.wallet.multisigType(), }); From 1f553991806987d0c2a568dcd046b40e3536df93 Mon Sep 17 00:00:00 2001 From: mrdanish26 Date: Wed, 22 Apr 2026 16:56:00 -0700 Subject: [PATCH 2/2] fix(sdk-core): verify recipients before signing (ECDSA) TICKET: WAL-375 --- .../src/abstractEthLikeNewCoins.ts | 13 +- .../tssUtils/ecdsaMPCv2/signTxRequest.ts | 165 +++++++++++++----- .../src/bitgo/utils/tss/ecdsa/base.ts | 53 +----- .../src/bitgo/utils/tss/ecdsa/ecdsa.ts | 3 +- .../src/bitgo/utils/tss/ecdsa/ecdsaMPCv2.ts | 3 +- .../src/bitgo/utils/tss/recipientUtils.ts | 53 ++++++ .../unit/bitgo/utils/tss/recipientUtils.ts | 121 +++++++++++++ 7 files changed, 310 insertions(+), 101 deletions(-) create mode 100644 modules/sdk-core/src/bitgo/utils/tss/recipientUtils.ts create mode 100644 modules/sdk-core/test/unit/bitgo/utils/tss/recipientUtils.ts diff --git a/modules/abstract-eth/src/abstractEthLikeNewCoins.ts b/modules/abstract-eth/src/abstractEthLikeNewCoins.ts index 7f830bd1a9..80c6b623b7 100644 --- a/modules/abstract-eth/src/abstractEthLikeNewCoins.ts +++ b/modules/abstract-eth/src/abstractEthLikeNewCoins.ts @@ -3109,9 +3109,16 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin { txParams.prebuildTx?.consolidateId || txPrebuild?.consolidateId || (txParams.type && - ['acceleration', 'fillNonce', 'transferToken', 'tokenApproval', 'consolidate', 'bridgeFunds'].includes( - txParams.type - )) + [ + 'acceleration', + 'fillNonce', + 'transferToken', + 'tokenApproval', + 'consolidate', + 'bridgeFunds', + 'enableToken', + 'customTx', + ].includes(txParams.type)) ) ) { throw new Error('missing txParams'); diff --git a/modules/bitgo/test/v2/unit/internal/tssUtils/ecdsaMPCv2/signTxRequest.ts b/modules/bitgo/test/v2/unit/internal/tssUtils/ecdsaMPCv2/signTxRequest.ts index de5f65516e..739abb3506 100644 --- a/modules/bitgo/test/v2/unit/internal/tssUtils/ecdsaMPCv2/signTxRequest.ts +++ b/modules/bitgo/test/v2/unit/internal/tssUtils/ecdsaMPCv2/signTxRequest.ts @@ -318,8 +318,6 @@ describe('signTxRequest:', function () { }); it('accepts signTxRequest when recipients are only in intent (smart contract interaction)', async function () { - const userShare = fs.readFileSync(shareFiles[vector.party1]); - const userPrvBase64 = Buffer.from(userShare).toString('base64'); const txRequestWithIntentRecipients = { ...txRequest, intent: { @@ -332,53 +330,120 @@ describe('signTxRequest:', function () { ], }, }; - // Should not throw the recipients guard error — falls back to intent.recipients - await tssUtils - .signTxRequest({ - txRequest: txRequestWithIntentRecipients, - prv: userPrvBase64, - reqId, - // no txParams.recipients - }) - .should.not.be.rejectedWith( - 'Recipient details are required to verify this transaction before signing. Pass txParams with at least one recipient.' - ); + const nockPromises = [ + await nockTxRequestResponseSignatureShareRoundOne(bitgoParty, txRequestWithIntentRecipients, bitgoGpgKey), + await nockTxRequestResponseSignatureShareRoundTwo(bitgoParty, txRequestWithIntentRecipients, bitgoGpgKey), + await nockTxRequestResponseSignatureShareRoundThree(txRequestWithIntentRecipients), + await nockSendTxRequest(txRequestWithIntentRecipients), + ]; + await Promise.all(nockPromises); + + const userShare = fs.readFileSync(shareFiles[vector.party1]); + const userPrvBase64 = Buffer.from(userShare).toString('base64'); + // Falls back to intent.recipients — guard should pass and signing should complete + await tssUtils.signTxRequest({ + txRequest: txRequestWithIntentRecipients, + prv: userPrvBase64, + reqId, + // no txParams.recipients + }); + nockPromises[0].isDone().should.be.true(); + nockPromises[1].isDone().should.be.true(); + nockPromises[2].isDone().should.be.true(); }); it('accepts signTxRequest for no-recipient tx types (tokenApproval)', async function () { + const nockPromises = [ + await nockTxRequestResponseSignatureShareRoundOne(bitgoParty, txRequest, bitgoGpgKey), + await nockTxRequestResponseSignatureShareRoundTwo(bitgoParty, txRequest, bitgoGpgKey), + await nockTxRequestResponseSignatureShareRoundThree(txRequest), + await nockSendTxRequest(txRequest), + ]; + await Promise.all(nockPromises); + const userShare = fs.readFileSync(shareFiles[vector.party1]); const userPrvBase64 = Buffer.from(userShare).toString('base64'); - // Should not throw the recipients guard error — type exemption applies - await tssUtils - .signTxRequest({ - txRequest, - prv: userPrvBase64, - reqId, - txParams: { type: 'tokenApproval' }, - }) - .should.not.be.rejectedWith( - 'Recipient details are required to verify this transaction before signing. Pass txParams with at least one recipient.' - ); + // Type exemption applies — guard passes without recipients + await tssUtils.signTxRequest({ + txRequest, + prv: userPrvBase64, + reqId, + txParams: { type: 'tokenApproval' }, + }); + nockPromises[0].isDone().should.be.true(); + nockPromises[1].isDone().should.be.true(); + nockPromises[2].isDone().should.be.true(); }); it('accepts signTxRequest for no-recipient tx types (acceleration)', async function () { + const nockPromises = [ + await nockTxRequestResponseSignatureShareRoundOne(bitgoParty, txRequest, bitgoGpgKey), + await nockTxRequestResponseSignatureShareRoundTwo(bitgoParty, txRequest, bitgoGpgKey), + await nockTxRequestResponseSignatureShareRoundThree(txRequest), + await nockSendTxRequest(txRequest), + ]; + await Promise.all(nockPromises); + const userShare = fs.readFileSync(shareFiles[vector.party1]); const userPrvBase64 = Buffer.from(userShare).toString('base64'); - await tssUtils - .signTxRequest({ - txRequest, - prv: userPrvBase64, - reqId, - txParams: { type: 'acceleration' }, - }) - .should.not.be.rejectedWith( - 'Recipient details are required to verify this transaction before signing. Pass txParams with at least one recipient.' - ); + await tssUtils.signTxRequest({ + txRequest, + prv: userPrvBase64, + reqId, + txParams: { type: 'acceleration' }, + }); + nockPromises[0].isDone().should.be.true(); + nockPromises[1].isDone().should.be.true(); + nockPromises[2].isDone().should.be.true(); }); - it('accepts signTxRequest when txParams.recipients takes priority over intent.recipients', async function () { + it('accepts signTxRequest for no-recipient tx types (customTx)', async function () { + const nockPromises = [ + await nockTxRequestResponseSignatureShareRoundOne(bitgoParty, txRequest, bitgoGpgKey), + await nockTxRequestResponseSignatureShareRoundTwo(bitgoParty, txRequest, bitgoGpgKey), + await nockTxRequestResponseSignatureShareRoundThree(txRequest), + await nockSendTxRequest(txRequest), + ]; + await Promise.all(nockPromises); + + const userShare = fs.readFileSync(shareFiles[vector.party1]); + const userPrvBase64 = Buffer.from(userShare).toString('base64'); + // DeFi/WalletConnect smart contract interactions have no traditional recipients + await tssUtils.signTxRequest({ + txRequest, + prv: userPrvBase64, + reqId, + txParams: { type: 'customTx' }, + }); + nockPromises[0].isDone().should.be.true(); + nockPromises[1].isDone().should.be.true(); + nockPromises[2].isDone().should.be.true(); + }); + + it('accepts signTxRequest for no-recipient tx types (enableToken)', async function () { + const nockPromises = [ + await nockTxRequestResponseSignatureShareRoundOne(bitgoParty, txRequest, bitgoGpgKey), + await nockTxRequestResponseSignatureShareRoundTwo(bitgoParty, txRequest, bitgoGpgKey), + await nockTxRequestResponseSignatureShareRoundThree(txRequest), + await nockSendTxRequest(txRequest), + ]; + await Promise.all(nockPromises); + const userShare = fs.readFileSync(shareFiles[vector.party1]); const userPrvBase64 = Buffer.from(userShare).toString('base64'); + // TSS wallets do not populate recipients for token enablement — exemption must apply + await tssUtils.signTxRequest({ + txRequest, + prv: userPrvBase64, + reqId, + txParams: { type: 'enableToken' }, + }); + nockPromises[0].isDone().should.be.true(); + nockPromises[1].isDone().should.be.true(); + nockPromises[2].isDone().should.be.true(); + }); + + it('accepts signTxRequest when txParams.recipients takes priority over intent.recipients', async function () { const txRequestWithBothRecipients = { ...txRequest, intent: { @@ -386,16 +451,26 @@ describe('signTxRequest:', function () { recipients: [{ address: { address: '0xintentRecipient' }, amount: { value: '9999', symbol: 'hteth' } }], }, }; - await tssUtils - .signTxRequest({ - txRequest: txRequestWithBothRecipients, - prv: userPrvBase64, - reqId, - txParams: { recipients: [{ address: '0xrecipient', amount: '1000' }] }, - }) - .should.not.be.rejectedWith( - 'Recipient details are required to verify this transaction before signing. Pass txParams with at least one recipient.' - ); + const nockPromises = [ + await nockTxRequestResponseSignatureShareRoundOne(bitgoParty, txRequestWithBothRecipients, bitgoGpgKey), + await nockTxRequestResponseSignatureShareRoundTwo(bitgoParty, txRequestWithBothRecipients, bitgoGpgKey), + await nockTxRequestResponseSignatureShareRoundThree(txRequestWithBothRecipients), + await nockSendTxRequest(txRequestWithBothRecipients), + ]; + await Promise.all(nockPromises); + + const userShare = fs.readFileSync(shareFiles[vector.party1]); + const userPrvBase64 = Buffer.from(userShare).toString('base64'); + // txParams.recipients takes priority — guard passes and signing completes + await tssUtils.signTxRequest({ + txRequest: txRequestWithBothRecipients, + prv: userPrvBase64, + reqId, + txParams: { recipients: [{ address: '0xrecipient', amount: '1000' }] }, + }); + nockPromises[0].isDone().should.be.true(); + nockPromises[1].isDone().should.be.true(); + nockPromises[2].isDone().should.be.true(); }); }); diff --git a/modules/sdk-core/src/bitgo/utils/tss/ecdsa/base.ts b/modules/sdk-core/src/bitgo/utils/tss/ecdsa/base.ts index 46b9fcd6c3..d6512327fb 100644 --- a/modules/sdk-core/src/bitgo/utils/tss/ecdsa/base.ts +++ b/modules/sdk-core/src/bitgo/utils/tss/ecdsa/base.ts @@ -1,61 +1,12 @@ import { secp256k1 } from '@noble/curves/secp256k1'; -import { IBaseCoin, TransactionParams } from '../../../baseCoin'; +import { IBaseCoin } from '../../../baseCoin'; import baseTSSUtils from '../baseTSSUtils'; import { KeyShare } from './types'; -import { BackupGpgKey, PopulatedIntent, TxRequest } from '../baseTypes'; +import { BackupGpgKey } from '../baseTypes'; import { generateGPGKeyPair } from '../../opengpgUtils'; import { BitGoBase } from '../../../bitgoBase'; import { IWallet } from '../../../wallet'; -import { InvalidTransactionError } from '../../../errors'; - -/** - * Transaction types that legitimately carry no explicit recipients. - * verifyTransaction handles no-recipient validation for these internally. - * Mirrors the bypass list in abstractEthLikeNewCoins.ts verifyTssTransaction. - */ -export const NO_RECIPIENT_TX_TYPES = new Set([ - 'acceleration', - 'fillNonce', - 'transferToken', - 'tokenApproval', - 'consolidate', - 'bridgeFunds', -]); - -/** - * Resolves the effective txParams for TSS signing recipient verification. - * - * For smart contract interactions, recipients live in txRequest.intent.recipients - * (native amount = 0, so buildParams is empty). Falls back to intent recipients - * mapped to ITransactionRecipient shape when txParams.recipients is absent. - * - * Throws InvalidTransactionError if no recipients can be resolved and the - * transaction type is not a known no-recipient type. - */ -export function resolveEffectiveTxParams( - txRequest: TxRequest, - txParams: TransactionParams | undefined -): TransactionParams { - const intentRecipients = (txRequest.intent as PopulatedIntent)?.recipients?.map((intentRecipient) => ({ - address: intentRecipient.address.address, - amount: intentRecipient.amount.value, - data: intentRecipient.data, - })); - - const effectiveTxParams: TransactionParams = { - ...txParams, - recipients: txParams?.recipients?.length ? txParams.recipients : intentRecipients, - }; - - if (!effectiveTxParams.recipients?.length && !NO_RECIPIENT_TX_TYPES.has(effectiveTxParams.type ?? '')) { - throw new InvalidTransactionError( - 'Recipient details are required to verify this transaction before signing. Pass txParams with at least one recipient.' - ); - } - - return effectiveTxParams; -} /** @inheritdoc */ export class BaseEcdsaUtils extends baseTSSUtils { diff --git a/modules/sdk-core/src/bitgo/utils/tss/ecdsa/ecdsa.ts b/modules/sdk-core/src/bitgo/utils/tss/ecdsa/ecdsa.ts index 614ee75f3f..163b97d66c 100644 --- a/modules/sdk-core/src/bitgo/utils/tss/ecdsa/ecdsa.ts +++ b/modules/sdk-core/src/bitgo/utils/tss/ecdsa/ecdsa.ts @@ -45,7 +45,8 @@ import { TssEcdsaStep2ReturnMessage, TxRequestChallengeResponse, } from '../../../tss/types'; -import { BaseEcdsaUtils, resolveEffectiveTxParams } from './base'; +import { BaseEcdsaUtils } from './base'; +import { resolveEffectiveTxParams } from '../recipientUtils'; import { IRequestTracer } from '../../../../api'; const encryptNShare = ECDSAMethods.encryptNShare; diff --git a/modules/sdk-core/src/bitgo/utils/tss/ecdsa/ecdsaMPCv2.ts b/modules/sdk-core/src/bitgo/utils/tss/ecdsa/ecdsaMPCv2.ts index eaeedd5f75..e76eee3e09 100644 --- a/modules/sdk-core/src/bitgo/utils/tss/ecdsa/ecdsaMPCv2.ts +++ b/modules/sdk-core/src/bitgo/utils/tss/ecdsa/ecdsaMPCv2.ts @@ -46,7 +46,8 @@ import { TSSParamsWithPrv, TxRequest, } from '../baseTypes'; -import { BaseEcdsaUtils, resolveEffectiveTxParams } from './base'; +import { BaseEcdsaUtils } from './base'; +import { resolveEffectiveTxParams } from '../recipientUtils'; import { EcdsaMPCv2KeyGenSendFn, KeyGenSenderForEnterprise } from './ecdsaMPCv2KeyGenSender'; import { envRequiresBitgoPubGpgKeyConfig, isBitgoMpcPubKey } from '../../../tss/bitgoPubKeys'; diff --git a/modules/sdk-core/src/bitgo/utils/tss/recipientUtils.ts b/modules/sdk-core/src/bitgo/utils/tss/recipientUtils.ts new file mode 100644 index 0000000000..682228c4b0 --- /dev/null +++ b/modules/sdk-core/src/bitgo/utils/tss/recipientUtils.ts @@ -0,0 +1,53 @@ +import { TransactionParams } from '../../baseCoin'; +import { InvalidTransactionError } from '../../errors'; +import { PopulatedIntent, TxRequest } from './baseTypes'; + +/** + * Transaction types that legitimately carry no explicit recipients. + * verifyTransaction handles no-recipient validation for these internally. + * Mirrors the bypass list in abstractEthLikeNewCoins.ts verifyTssTransaction. + */ +export const NO_RECIPIENT_TX_TYPES = new Set([ + 'acceleration', + 'fillNonce', + 'transferToken', + 'tokenApproval', + 'consolidate', + 'bridgeFunds', + 'enableToken', + 'customTx', // DeFi/WalletConnect smart contract interactions have no traditional recipients +]); + +/** + * Resolves the effective txParams for TSS signing recipient verification. + * + * For smart contract interactions, recipients live in txRequest.intent.recipients + * (native amount = 0, so buildParams is empty). Falls back to intent recipients + * mapped to ITransactionRecipient shape when txParams.recipients is absent. + * + * Throws InvalidTransactionError if no recipients can be resolved and the + * transaction type is not a known no-recipient type. + */ +export function resolveEffectiveTxParams( + txRequest: TxRequest, + txParams: TransactionParams | undefined +): TransactionParams { + const intentRecipients = (txRequest.intent as PopulatedIntent)?.recipients?.map((intentRecipient) => ({ + address: intentRecipient.address.address, + amount: intentRecipient.amount.value, + data: intentRecipient.data, + })); + + const effectiveTxParams: TransactionParams = { + ...txParams, + recipients: txParams?.recipients?.length ? txParams.recipients : intentRecipients, + }; + + if (!effectiveTxParams.recipients?.length && !NO_RECIPIENT_TX_TYPES.has(effectiveTxParams.type ?? '')) { + throw new InvalidTransactionError( + 'Recipient details are required to verify this transaction before signing. Pass txParams with at least one recipient.' + ); + } + + return effectiveTxParams; +} diff --git a/modules/sdk-core/test/unit/bitgo/utils/tss/recipientUtils.ts b/modules/sdk-core/test/unit/bitgo/utils/tss/recipientUtils.ts new file mode 100644 index 0000000000..9ff4ad37c8 --- /dev/null +++ b/modules/sdk-core/test/unit/bitgo/utils/tss/recipientUtils.ts @@ -0,0 +1,121 @@ +import 'should'; +import * as assert from 'assert'; + +// Import directly from source so no top-level export change is needed +const getModule = () => require('../../../../../src/bitgo/utils/tss/recipientUtils'); + +function makeTxRequest( + intentRecipients?: { address: { address: string }; amount: { value: string }; data?: string }[] +): any { + return { + txRequestId: 'test-req-id', + intent: intentRecipients ? { intentType: 'contractCall', recipients: intentRecipients } : { intentType: 'payment' }, + transactions: [], + unsignedTxs: [], + state: 'pendingUserSignature', + walletType: 'hot', + walletId: 'walletId', + policiesChecked: true, + version: 1, + userId: 'userId', + }; +} + +describe('recipientUtils', function () { + describe('NO_RECIPIENT_TX_TYPES', function () { + it('contains exactly the 8 expected exempted types', function () { + const { NO_RECIPIENT_TX_TYPES } = getModule(); + const expected = [ + 'acceleration', + 'fillNonce', + 'transferToken', + 'tokenApproval', + 'consolidate', + 'bridgeFunds', + 'enableToken', + 'customTx', + ]; + expected.forEach((t) => assert.ok(NO_RECIPIENT_TX_TYPES.has(t), `${t} should be in NO_RECIPIENT_TX_TYPES`)); + assert.strictEqual(NO_RECIPIENT_TX_TYPES.size, expected.length); + }); + }); + + describe('resolveEffectiveTxParams', function () { + it('uses txParams.recipients when provided', function () { + const { resolveEffectiveTxParams } = getModule(); + const txRequest = makeTxRequest(); + const result = resolveEffectiveTxParams(txRequest, { recipients: [{ address: '0xabc', amount: '100' }] }); + result.recipients[0].address.should.equal('0xabc'); + result.recipients[0].amount.should.equal('100'); + }); + + it('falls back to intent.recipients when txParams has no recipients', function () { + const { resolveEffectiveTxParams } = getModule(); + const txRequest = makeTxRequest([{ address: { address: '0xintent' }, amount: { value: '500' } }]); + const result = resolveEffectiveTxParams(txRequest, {}); + result.recipients[0].address.should.equal('0xintent'); + result.recipients[0].amount.should.equal('500'); + }); + + it('falls back to intent.recipients when txParams is undefined', function () { + const { resolveEffectiveTxParams } = getModule(); + const txRequest = makeTxRequest([{ address: { address: '0xintent' }, amount: { value: '999' } }]); + const result = resolveEffectiveTxParams(txRequest, undefined); + result.recipients[0].address.should.equal('0xintent'); + }); + + it('prefers txParams.recipients over intent.recipients when both present', function () { + const { resolveEffectiveTxParams } = getModule(); + const txRequest = makeTxRequest([{ address: { address: '0xintent' }, amount: { value: '9999' } }]); + const result = resolveEffectiveTxParams(txRequest, { recipients: [{ address: '0xexplicit', amount: '1' }] }); + result.recipients[0].address.should.equal('0xexplicit'); + }); + + it('maps intent.data field through to the recipient', function () { + const { resolveEffectiveTxParams } = getModule(); + const txRequest = makeTxRequest([ + { address: { address: '0xcontract' }, amount: { value: '0' }, data: '0xabcdef' }, + ]); + const result = resolveEffectiveTxParams(txRequest, undefined); + result.recipients[0].data.should.equal('0xabcdef'); + }); + + it('throws InvalidTransactionError when no recipients and type is not exempted', function () { + const { resolveEffectiveTxParams } = getModule(); + const txRequest = makeTxRequest(); + assert.throws( + () => resolveEffectiveTxParams(txRequest, {}), + /Recipient details are required to verify this transaction before signing/ + ); + }); + + it('throws when txParams is undefined and intent has no recipients', function () { + const { resolveEffectiveTxParams } = getModule(); + const txRequest = makeTxRequest(); + assert.throws( + () => resolveEffectiveTxParams(txRequest, undefined), + /Recipient details are required to verify this transaction before signing/ + ); + }); + + const NO_RECIPIENT_TYPES = [ + 'acceleration', + 'fillNonce', + 'transferToken', + 'tokenApproval', + 'consolidate', + 'bridgeFunds', + 'enableToken', // TSS wallets do not populate recipients for token enablement + 'customTx', // DeFi/WalletConnect smart contract interactions have no traditional recipients + ]; + + NO_RECIPIENT_TYPES.forEach((type) => { + it(`allows empty recipients for no-recipient tx type: ${type}`, function () { + const { resolveEffectiveTxParams } = getModule(); + const txRequest = makeTxRequest(); + const result = resolveEffectiveTxParams(txRequest, { type }); + result.type.should.equal(type); + }); + }); + }); +});