Merge pull request #6957 from BitGo/validate-Staking-txrequest

fix(SC-1435): add validation for unsigned txHex matches client staking request

Author: henryyang183

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);
     });
   });
 });

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`);