From 8173889576e657eea3c8e53c0b0e1506ea0c5e07 Mon Sep 17 00:00:00 2001 From: neha-kri Date: Fri, 30 Oct 2026 20:27:42 +0530 Subject: [PATCH] fix(sdk-core): skip auto-sign for message-sign approvals postApprove() unconditionally ran the TSS recreate-and-sign flow for TRANSACTION_REQUEST_FULL approvals, which reads transactions[0].unsignedTx. Message-only txRequests have an empty transactions array, so this crashed. postApprove() now returns early when the approval's intent is a message-sign intent (signMessage / signTypedStructuredData), leaving the message to be signed separately via signAndSendMessageTxRequest. Also hardens the signTxRequest assertions in all TSS signer variants (ecdsa, ecdsaMPCv2, eddsa, eddsaMPCv2) to check length > 0 instead of truthiness (an empty array is truthy), and types the transactionRequestFull info field on PendingApprovalInfo. Refs: DEFI-1257 TICKET: DEFI-1257 --- modules/bitgo/test/v2/unit/pendingApproval.ts | 50 +++++++++++++++++++ .../bitgo/pendingApproval/iPendingApproval.ts | 16 ++++++ .../bitgo/pendingApproval/pendingApproval.ts | 17 ++++++- .../src/bitgo/utils/tss/ecdsa/ecdsa.ts | 18 +++++-- .../src/bitgo/utils/tss/ecdsa/ecdsaMPCv2.ts | 7 ++- .../src/bitgo/utils/tss/eddsa/eddsa.ts | 18 +++++-- .../src/bitgo/utils/tss/eddsa/eddsaMPCv2.ts | 7 ++- .../unit/bitgo/utils/tss/ecdsa/ecdsaMPCv2.ts | 29 +++++++++++ 8 files changed, 153 insertions(+), 9 deletions(-) diff --git a/modules/bitgo/test/v2/unit/pendingApproval.ts b/modules/bitgo/test/v2/unit/pendingApproval.ts index 3a71b58d1d..77a3b6fd11 100644 --- a/modules/bitgo/test/v2/unit/pendingApproval.ts +++ b/modules/bitgo/test/v2/unit/pendingApproval.ts @@ -706,4 +706,54 @@ describe('Pending Approvals:', () => { }); }); }); + + describe('message-sign approvals (transactionRequestFull)', () => { + const buildMessageSignPendingApprovalData = (intentType: string): PendingApprovalData => ({ + id: 'pa-msg', + info: { + type: Type.TRANSACTION_REQUEST_FULL, + transactionRequestFull: { + txRequestId: 'txreq-msg-1', + intent: { intentType }, + }, + } as PendingApprovalInfo, + state: State.PENDING, + creator: 'test', + txRequestId: 'txreq-msg-1', + }); + + ['signMessage', 'signTypedStructuredData'].forEach((intentType) => { + it(`should approve without the post-approve auto-sign for a ${intentType} approval`, async () => { + const ethBaseCoin = bitgo.coin('teth'); + const paData = buildMessageSignPendingApprovalData(intentType); + const pendingApproval = new PendingApproval(bitgo, ethBaseCoin, paData, wallet); + + const paScope = nock(bgUrl) + .put(`/api/v2/teth/pendingapprovals/pa-msg`, { state: 'approved', otp: undefined }) + .reply(200, { ...paData, state: 'approved' }); + const recreateTransactionTssStub = sandbox.stub(PendingApproval.prototype, 'recreateAndSignTSSTransaction'); + + await pendingApproval.approve({ walletPassphrase: 'pass' }); + + recreateTransactionTssStub.notCalled.should.be.true(); + paScope.isDone().should.be.true(); + }); + }); + + it('should keep the post-approve auto-sign for transaction approvals', async () => { + const ethBaseCoin = bitgo.coin('teth'); + const paData = buildMessageSignPendingApprovalData('payment'); + const pendingApproval = new PendingApproval(bitgo, ethBaseCoin, paData, wallet); + + const paScope = nock(bgUrl) + .put(`/api/v2/teth/pendingapprovals/pa-msg`, { state: 'approved', otp: undefined }) + .reply(200, { ...paData, state: 'approved' }); + const recreateTransactionTssStub = sandbox.stub(PendingApproval.prototype, 'recreateAndSignTSSTransaction'); + + await pendingApproval.approve({ walletPassphrase: 'pass' }); + + recreateTransactionTssStub.calledOnce.should.be.true(); + paScope.isDone().should.be.true(); + }); + }); }); diff --git a/modules/sdk-core/src/bitgo/pendingApproval/iPendingApproval.ts b/modules/sdk-core/src/bitgo/pendingApproval/iPendingApproval.ts index 5b33a47e22..6376164a4e 100644 --- a/modules/sdk-core/src/bitgo/pendingApproval/iPendingApproval.ts +++ b/modules/sdk-core/src/bitgo/pendingApproval/iPendingApproval.ts @@ -48,8 +48,24 @@ export interface PendingApprovalInfo { }; sourceWallet?: string; }; + /** + * Present on transactionRequestFull pending approvals. intent.intentType is + * MESSAGE_SIGNING_INTENT_TYPES for message-sign approvals, which have no + * unsigned transaction. + */ + transactionRequestFull?: { + txRequestId?: string; + intent?: { + intentType?: string; + [index: string]: unknown; + }; + [index: string]: unknown; + }; } +/** Intent types whose txRequest carries a message to sign rather than a transaction. */ +export const MESSAGE_SIGNING_INTENT_TYPES: ReadonlySet = new Set(['signMessage', 'signTypedStructuredData']); + export interface PendingApprovalData { id: string; wallet?: string; diff --git a/modules/sdk-core/src/bitgo/pendingApproval/pendingApproval.ts b/modules/sdk-core/src/bitgo/pendingApproval/pendingApproval.ts index 4f18b59d69..6eb61f3eb3 100644 --- a/modules/sdk-core/src/bitgo/pendingApproval/pendingApproval.ts +++ b/modules/sdk-core/src/bitgo/pendingApproval/pendingApproval.ts @@ -4,6 +4,7 @@ import * as _ from 'lodash'; import * as utxolib from '@bitgo/utxo-lib'; import type { AttestationPayload } from '@bitgo/public-types'; +import { MESSAGE_SIGNING_INTENT_TYPES } from './iPendingApproval'; import { IBaseCoin } from '../baseCoin'; import { BitGoBase } from '../bitgoBase'; import { @@ -172,7 +173,10 @@ export class PendingApproval implements IPendingApproval { } /** - * Sets this PendingApproval to an approved state + * Sets this PendingApproval to an approved state. + * For transaction approvals on TSS wallets, this also attempts to recreate + * and sign the transaction. For message-sign approvals, signing is deferred: + * the message must be signed separately via signAndSendMessageTxRequest. */ async approve(params: ApproveOptions = {}): Promise { params.previewPendingTxs = true; @@ -489,12 +493,23 @@ export class PendingApproval implements IPendingApproval { reqId ); } else if (this.canRecreateTransaction(params) && this.baseCoin.supportsTss()) { + if (this.isMessageSigningRequest()) { + return; + } await this.recreateAndSignTSSTransaction(params, reqId); } } } } + /** + * True when the approval's intent is a message-sign intent. + */ + private isMessageSigningRequest(): boolean { + const intentType = this.info()?.transactionRequestFull?.intent?.intentType; + return Boolean(intentType && MESSAGE_SIGNING_INTENT_TYPES.has(intentType)); + } + /** * Helper function to ensure that self.wallet is set */ 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 3ef50fe19e..fece8b88d0 100644 --- a/modules/sdk-core/src/bitgo/utils/tss/ecdsa/ecdsa.ts +++ b/modules/sdk-core/src/bitgo/utils/tss/ecdsa/ecdsa.ts @@ -590,11 +590,13 @@ export class EcdsaUtils extends BaseEcdsaUtils { if (requestType === RequestType.tx) { assert( - txRequest.transactions || (txRequest as TxRequest).unsignedTxs, + (txRequest.transactions && txRequest.transactions.length > 0) || + (txRequest.unsignedTxs && txRequest.unsignedTxs.length > 0), 'Unable to find transactions in txRequest' ); const unsignedTx = txRequest.apiVersion === 'full' ? txRequest.transactions![0].unsignedTx : txRequest.unsignedTxs[0]; + assert(unsignedTx, 'Missing unsignedTx in txRequest'); derivationPath = unsignedTx.derivationPath; } else if (requestType === RequestType.message) { // TODO BG-67299 Message signing with derivation path @@ -641,9 +643,14 @@ export class EcdsaUtils extends BaseEcdsaUtils { const txRequest: TxRequest = tssParams.txRequest; let signablePayload; if (requestType === RequestType.tx) { - assert(txRequest.transactions || txRequest.unsignedTxs, 'Unable to find transactions in txRequest'); + assert( + (txRequest.transactions && txRequest.transactions.length > 0) || + (txRequest.unsignedTxs && txRequest.unsignedTxs.length > 0), + 'Unable to find transactions in txRequest' + ); const unsignedTx = txRequest.apiVersion === 'full' ? txRequest.transactions![0].unsignedTx : txRequest.unsignedTxs[0]; + assert(unsignedTx, 'Missing unsignedTx in txRequest'); signablePayload = Buffer.from(unsignedTx.signableHex, 'hex'); } else if (requestType === RequestType.message) { signablePayload = (params.tssParams as TSSParamsForMessage).bufferToSign; @@ -799,9 +806,14 @@ export class EcdsaUtils extends BaseEcdsaUtils { let derivationPath = ''; if (requestType === RequestType.tx) { - assert(txRequest.transactions || txRequest.unsignedTxs, 'Unable to find transactions in txRequest'); + assert( + (txRequest.transactions && txRequest.transactions.length > 0) || + (txRequest.unsignedTxs && txRequest.unsignedTxs.length > 0), + 'Unable to find transactions in txRequest' + ); const unsignedTx = txRequest.apiVersion === 'full' ? txRequest.transactions![0].unsignedTx : txRequest.unsignedTxs[0]; + assert(unsignedTx, 'Missing unsignedTx in txRequest'); if ( 'recipientSource' in params && 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 448dc632c5..b942656b94 100644 --- a/modules/sdk-core/src/bitgo/utils/tss/ecdsa/ecdsaMPCv2.ts +++ b/modules/sdk-core/src/bitgo/utils/tss/ecdsa/ecdsaMPCv2.ts @@ -962,9 +962,14 @@ export class EcdsaMPCv2Utils extends BaseEcdsaUtils { } if (requestType === RequestType.tx) { - assert(txRequest.transactions || txRequest.unsignedTxs, 'Unable to find transactions in txRequest'); + assert( + (txRequest.transactions && txRequest.transactions.length > 0) || + (txRequest.unsignedTxs && txRequest.unsignedTxs.length > 0), + 'Unable to find transactions in txRequest' + ); const unsignedTx = txRequest.apiVersion === 'full' ? txRequest.transactions![0].unsignedTx : txRequest.unsignedTxs[0]; + assert(unsignedTx, 'Missing unsignedTx in txRequest'); if ( 'recipientSource' in params && diff --git a/modules/sdk-core/src/bitgo/utils/tss/eddsa/eddsa.ts b/modules/sdk-core/src/bitgo/utils/tss/eddsa/eddsa.ts index 46a22f0080..2a678a1d0a 100644 --- a/modules/sdk-core/src/bitgo/utils/tss/eddsa/eddsa.ts +++ b/modules/sdk-core/src/bitgo/utils/tss/eddsa/eddsa.ts @@ -546,11 +546,16 @@ export class EddsaUtils extends baseTSSUtils { throw new Error('Invalid user key - missing backupYShare'); } - assert(txRequestResolved.transactions || txRequestResolved.unsignedTxs, 'Unable to find transactions in txRequest'); + assert( + (txRequestResolved.transactions && txRequestResolved.transactions.length > 0) || + (txRequestResolved.unsignedTxs && txRequestResolved.unsignedTxs.length > 0), + 'Unable to find transactions in txRequest' + ); const unsignedTx = txRequestResolved.apiVersion === 'full' ? txRequestResolved.transactions![0].unsignedTx : txRequestResolved.unsignedTxs[0]; + assert(unsignedTx, 'Missing unsignedTx in txRequest'); const signingKey = MPC.keyDerive( userSigningMaterial.uShare, @@ -634,11 +639,16 @@ export class EddsaUtils extends baseTSSUtils { throw new Error('Invalid user key - missing backupYShare'); } - assert(txRequestResolved.transactions || txRequestResolved.unsignedTxs, 'Unable to find transactions in txRequest'); + assert( + (txRequestResolved.transactions && txRequestResolved.transactions.length > 0) || + (txRequestResolved.unsignedTxs && txRequestResolved.unsignedTxs.length > 0), + 'Unable to find transactions in txRequest' + ); const unsignedTx = txRequestResolved.apiVersion === 'full' ? txRequestResolved.transactions![0].unsignedTx : txRequestResolved.unsignedTxs[0]; + assert(unsignedTx, 'Missing unsignedTx in txRequest'); const signablePayload = Buffer.from(unsignedTx.signableHex, 'hex'); @@ -765,11 +775,13 @@ export class EddsaUtils extends baseTSSUtils { let unsignedTx: UnsignedTransactionTss; if (requestType === RequestType.tx) { assert( - txRequestResolved.transactions || txRequestResolved.unsignedTxs, + (txRequestResolved.transactions && txRequestResolved.transactions.length > 0) || + (txRequestResolved.unsignedTxs && txRequestResolved.unsignedTxs.length > 0), 'Unable to find transactions in txRequest' ); unsignedTx = apiVersion === 'full' ? txRequestResolved.transactions![0].unsignedTx : txRequestResolved.unsignedTxs[0]; + assert(unsignedTx, 'Missing unsignedTx in txRequest'); } else if (requestType === RequestType.message) { assert(txRequestResolved.messages?.length, 'Unable to find messages in txRequest for message signing'); const message = txRequestResolved.messages[0]; diff --git a/modules/sdk-core/src/bitgo/utils/tss/eddsa/eddsaMPCv2.ts b/modules/sdk-core/src/bitgo/utils/tss/eddsa/eddsaMPCv2.ts index d6b73e963b..0114ef0b86 100644 --- a/modules/sdk-core/src/bitgo/utils/tss/eddsa/eddsaMPCv2.ts +++ b/modules/sdk-core/src/bitgo/utils/tss/eddsa/eddsaMPCv2.ts @@ -587,9 +587,14 @@ export class EddsaMPCv2Utils extends BaseEddsaUtils { assert(bitgoGpgPubKey, 'Missing BitGo GPG key for MPCv2'); if (requestType === RequestType.tx) { - assert(txRequest.transactions || txRequest.unsignedTxs, 'Unable to find transactions in txRequest'); + assert( + (txRequest.transactions && txRequest.transactions.length > 0) || + (txRequest.unsignedTxs && txRequest.unsignedTxs.length > 0), + 'Unable to find transactions in txRequest' + ); const unsignedTx = txRequest.apiVersion === 'full' ? txRequest.transactions![0].unsignedTx : txRequest.unsignedTxs![0]; + assert(unsignedTx, 'Missing unsignedTx in txRequest'); txOrMessageToSign = unsignedTx.signableHex; assert(txOrMessageToSign, 'Missing signableHex in unsignedTx'); derivationPath = unsignedTx.derivationPath; diff --git a/modules/sdk-core/test/unit/bitgo/utils/tss/ecdsa/ecdsaMPCv2.ts b/modules/sdk-core/test/unit/bitgo/utils/tss/ecdsa/ecdsaMPCv2.ts index 0e8217f61d..a72732aeeb 100644 --- a/modules/sdk-core/test/unit/bitgo/utils/tss/ecdsa/ecdsaMPCv2.ts +++ b/modules/sdk-core/test/unit/bitgo/utils/tss/ecdsa/ecdsaMPCv2.ts @@ -1489,3 +1489,32 @@ describe('EcdsaMPCv2Utils.signEcdsaMPCv2TssUsingExternalSigner attestation', () } }); }); + +describe('EcdsaMPCv2Utils.signTxRequest assert', () => { + it('should throw a clear error when transactions is empty', async () => { + const mockBg = {} as BitGoBase; + mockBg.getEnv = sinon.stub().returns('test'); + mockBg.setRequestTracer = sinon.stub(); + const mockCoin = {} as IBaseCoin; + mockCoin.getHashFunction = sinon.stub().callsFake(() => createKeccakHash('keccak256') as Hash); + + const utils = new EcdsaMPCv2Utils(mockBg, mockCoin); + sinon.stub(utils as any, 'pickBitgoPubGpgKeyForSigning').resolves(bitgoGpgKey.public); + + const emptyTxRequest = { + txRequestId: 'assert-test', + apiVersion: 'full', + transactions: [], + messages: [], + } as unknown as TxRequest; + + await assert.rejects( + utils.signTxRequest({ + txRequest: emptyTxRequest, + prv: 'fake-prv', + reqId: { inc: sinon.stub(), toString: sinon.stub().returns('test-req') } as any, + } as any), + /Unable to find transactions in txRequest/ + ); + }); +});