From b53537595f0fa8c0eec28c28889c5b7e5a62723a Mon Sep 17 00:00:00 2001 From: henryyang183 Date: Thu, 11 Sep 2025 10:37:26 -0400 Subject: [PATCH] fix(bitgo): added validation for unsigned txHex from txRequest matches client staking request Ticket: SC-1435 --- .../v2/unit/staking/stakingWalletNonTSS.ts | 15 ++--- .../src/bitgo/staking/stakingWallet.ts | 59 +++++++++---------- 2 files changed, 36 insertions(+), 38 deletions(-) diff --git a/modules/bitgo/test/v2/unit/staking/stakingWalletNonTSS.ts b/modules/bitgo/test/v2/unit/staking/stakingWalletNonTSS.ts index 26eefeb867..bbaf906e81 100644 --- a/modules/bitgo/test/v2/unit/staking/stakingWalletNonTSS.ts +++ b/modules/bitgo/test/v2/unit/staking/stakingWalletNonTSS.ts @@ -400,7 +400,7 @@ describe('non-TSS Staking Wallet', function () { await topethWctStakingWallet.buildAndSign({ walletPassphrase: 'passphrase' }, stakingTransaction); }); - it('should log an error if a transaction is failed to be validated transaction if unsigned transaction does not match the staking transaction', async function () { + it('should throw an error if unsigned transaction does not match the staking transaction', async function () { const unsignedTransaction: PrebuildTransactionResult = { walletId: topethWctStakingWallet.walletId, ...opethFixtures.unsignedStakingTransaction, @@ -409,8 +409,6 @@ describe('non-TSS Staking Wallet', function () { } as PrebuildTransactionResult; const stakingTransaction: StakingTransaction = opethFixtures.updatedStakingRequest; - const consoleStub = sinon.stub(console, 'error'); - nock(microservicesUri) .get( `/api/staking/v1/${topethWctStakingWallet.coin}/wallets/${topethWctStakingWallet.walletId}/requests/${stakingTransaction.stakingRequestId}/transactions/${stakingTransaction.id}` @@ -426,11 +424,14 @@ describe('non-TSS Staking Wallet', function () { .post(`/api/v2/topeth/wallet/${topethWctStakingWallet.walletId}/tx/build`) .reply(200, unsignedTransaction); - await topethWctStakingWallet.buildAndSign({ walletPassphrase: 'passphrase' }, stakingTransaction); + const expectedErrorMessage = + 'Staking transaction validation failed before signing: ' + + 'Unexpected recipient address found in built transaction: 0x86bb6dca2cd6f9a0189c478bbb8f7ee2fef07c89; ' + + 'Expected recipient address not found in built transaction: 0x75bb6dca2cd6f9a0189c478bbb8f7ee2fef07c78'; - consoleStub.calledWith( - 'Invalid recipient address: 0x86bb6dca2cd6f9a0189c478bbb8f7ee2fef07c89, Missing recipient address(es): 0x75bb6dca2cd6f9a0189c478bbb8f7ee2fef07c78' - ); + await topethWctStakingWallet + .buildAndSign({ walletPassphrase: 'passphrase' }, stakingTransaction) + .should.be.rejectedWith(expectedErrorMessage); }); }); }); diff --git a/modules/sdk-core/src/bitgo/staking/stakingWallet.ts b/modules/sdk-core/src/bitgo/staking/stakingWallet.ts index 7ee2f13c07..fcfa647f57 100644 --- a/modules/sdk-core/src/bitgo/staking/stakingWallet.ts +++ b/modules/sdk-core/src/bitgo/staking/stakingWallet.ts @@ -271,7 +271,7 @@ export class StakingWallet implements IStakingWallet { // default to verifying a transaction unless explicitly skipped const skipVerification = signOptions.transactionVerificationOptions?.skipTransactionVerification ?? false; if (!isStakingTxRequestPrebuildResult(builtTx.result) && !skipVerification) { - await this.validateBuiltStakingTransaction(transaction, builtTx); + await this.validateBuiltStakingTransaction(builtTx.transaction, builtTx); } return await this.sign(signOptions, builtTx); } @@ -372,7 +372,7 @@ export class StakingWallet implements IStakingWallet { const explainedTransaction = await coin.explainTransaction(result); - if (buildParams?.recipients) { + if (buildParams?.recipients && buildParams.recipients.length > 0) { const userRecipientMap = new Map( buildParams.recipients.map((recipient) => [recipient.address.toLowerCase(), recipient]) ); @@ -382,41 +382,38 @@ export class StakingWallet implements IStakingWallet { const mismatchErrors: string[] = []; - for (const [recipientAddress, recipientInfo] of platformRecipientMap) { - if (userRecipientMap.has(recipientAddress)) { - const userRecipient = userRecipientMap.get(recipientAddress); - if (!userRecipient) { - console.error('Unable to determine recipient address'); - return; - } - const matchResult = transactionRecipientsMatch(userRecipient, recipientInfo); - if (!matchResult.exactMatch) { - if (!matchResult.tokenMatch) { - mismatchErrors.push( - `Invalid token ${recipientInfo.tokenName} transfer with amount ${recipientInfo.amount} to ${recipientInfo.address} found in built transaction, specified ${userRecipient.tokenName}` - ); - } - if (!matchResult.amountMatch) { - mismatchErrors.push( - `Invalid recipient amount for ${recipientInfo.address}, specified ${userRecipient.amount} got ${recipientInfo.amount}` - ); - } - } - } else { - mismatchErrors.push(`Invalid recipient address: ${recipientAddress}`); + for (const [address] of platformRecipientMap) { + if (!userRecipientMap.has(address)) { + mismatchErrors.push(`Unexpected recipient address found in built transaction: ${address}`); } } - const missingRecipientAddresses = Array.from(userRecipientMap.keys()).filter( - (address) => !platformRecipientMap.has(address) - ); + for (const [address, userRecipient] of userRecipientMap) { + const platformRecipient = platformRecipientMap.get(address); + if (!platformRecipient) { + mismatchErrors.push(`Expected recipient address not found in built transaction: ${address}`); + continue; + } + + const matchResult = transactionRecipientsMatch(userRecipient, platformRecipient); - if (missingRecipientAddresses.length > 0) { - mismatchErrors.push(`Missing recipient address(es): ${missingRecipientAddresses.join(', ')}`); + if (!matchResult.amountMatch) { + mismatchErrors.push( + `Recipient ${address} amount mismatch. Expected: ${userRecipient.amount}, Got: ${platformRecipient.amount}` + ); + } + if (!matchResult.tokenMatch) { + mismatchErrors.push( + `Recipient ${address} token mismatch. Expected: ${userRecipient.tokenName ?? 'native coin'}, Got: ${ + platformRecipient.tokenName ?? 'native coin' + }` + ); + } } if (mismatchErrors.length > 0) { - console.error(mismatchErrors.join(', ')); - return; + const errorMessage = `Staking transaction validation failed before signing: ${mismatchErrors.join('; ')}`; + debug(errorMessage); + throw new Error(errorMessage); } } else { debug(`Cannot validate staking transaction ${transaction.stakingRequestId} without specified build params`);