fix: updating recovery tx building for EIP-155 txs

Ticket: WIN-6260

Author: parasgarg-bitgo

modules/abstract-eth/package.json

@@ -46,6 +46,7 @@
     "@bitgo/statics": "^55.3.0",
     "@ethereumjs/common": "^2.6.5",
     "@ethereumjs/tx": "^3.3.0",
+    "@ethereumjs/rlp": "^4.0.0",
     "@metamask/eth-sig-util": "^5.0.2",
     "bignumber.js": "^9.1.1",
     "bn.js": "^5.2.1",

modules/abstract-eth/src/abstractEthLikeNewCoins.ts

@@ -42,6 +42,7 @@ import { getDerivationPath } from '@bitgo/sdk-lib-mpc';
 import { bip32 } from '@bitgo/secp256k1';
 import {
   BaseCoin as StaticsBaseCoin,
+  CoinFeature,
   CoinMap,
   coins,
   EthereumNetwork as EthLikeNetwork,
@@ -50,12 +51,13 @@ import {
 import type * as EthLikeCommon from '@ethereumjs/common';
 import type * as EthLikeTxLib from '@ethereumjs/tx';
 import { FeeMarketEIP1559Transaction, Transaction as LegacyTransaction } from '@ethereumjs/tx';
+import { RLP } from '@ethereumjs/rlp';
 import { SignTypedDataVersion, TypedDataUtils, TypedMessage } from '@metamask/eth-sig-util';
 import { BigNumber } from 'bignumber.js';
 import BN from 'bn.js';
 import { randomBytes } from 'crypto';
 import debugLib from 'debug';
-import { addHexPrefix, stripHexPrefix } from 'ethereumjs-util';
+import { addHexPrefix, bufArrToArr, stripHexPrefix } from 'ethereumjs-util';
 import Keccak from 'keccak';
 import _ from 'lodash';
 import secp256k1 from 'secp256k1';
@@ -482,8 +484,8 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin {
     replayProtectionOptions?: ReplayProtectionOptions
   ): EthLikeCommon.default {
     // if eip1559 params are specified, default to london hardfork, otherwise,
-    // default to tangerine whistle to avoid replay protection issues
-    const defaultHardfork = !!eip1559 ? 'london' : optionalDeps.EthCommon.Hardfork.TangerineWhistle;
+    // default to petersburg to avoid replay protection issues
+    const defaultHardfork = !!eip1559 ? 'london' : optionalDeps.EthCommon.Hardfork.Petersburg;
     const ethLikeCommon = AbstractEthLikeNewCoins.getCustomChainCommon(replayProtectionOptions?.chain as number);
     ethLikeCommon.setHardfork(replayProtectionOptions?.hardfork ?? defaultHardfork);
     return ethLikeCommon;
@@ -1109,6 +1111,15 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin {
     if (params.recoveryDestination === undefined || !this.isValidAddress(params.recoveryDestination)) {
       throw new Error('invalid recoveryDestination');
     }
+
+    if (!this.staticsCoin?.features.includes(CoinFeature.EIP1559)) {
+      if (params.eip1559) {
+        throw new Error('Invalid fee params. EIP1559 not supported');
+      }
+      if (params.replayProtectionOptions?.hardfork === 'london') {
+        throw new Error('Invalid replayProtection options. Cannot use the hardfork "london" for this chain');
+      }
+    }
   }
 
   /**
@@ -2034,7 +2045,6 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin {
     let lastScanIndex = 0;
 
     for (let i = 0; i < req.length; i++) {
-      const MPC = new Ecdsa();
       const transaction = req[i]?.txRequest?.transactions?.[0]?.unsignedTx as unknown as UnsignedTransactionTss;
       if (!req[i].ovc || !req[i].ovc[0].ecdsaSignature) {
         throw new Error('Missing signature(s)');
@@ -2056,9 +2066,6 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin {
         s: shares[2],
         y: shares[3],
       } as unknown as ECDSAMethodTypes.Signature;
-      const signatureHex = Buffer.from(signature.toString(), 'hex');
-      const txBuilder = this.getTransactionBuilder(getCommon(this.getNetwork() as EthLikeNetwork));
-      txBuilder.from(transaction.serializedTxHex as string);
 
       if (!transaction.coinSpecific?.commonKeyChain) {
         throw new Error(`Missing common keychain for transaction at index ${i}`);
@@ -2071,29 +2078,23 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin {
         throw new Error(`Missing common key chain for transaction at index ${i}`);
       }
 
-      const derivationPath = transaction.derivationPath ?? 'm/0';
-      const derivedCommonKeyChain = MPC.deriveUnhardened(String(commonKeyChain), String(derivationPath));
-      const derivedPublicKey = new KeyPairLib({ pub: derivedCommonKeyChain.slice(0, 66) });
-      txBuilder.addSignature({ pub: derivedPublicKey.getKeys().pub }, signatureHex);
       const ethCommmon = AbstractEthLikeNewCoins.getEthLikeCommon(
         transaction.eip1559,
         transaction.replayProtectionOptions
       );
-      let unsignedTx;
+      let unsignedTx: EthLikeTxLib.FeeMarketEIP1559Transaction | EthLikeTxLib.Transaction;
       if (transaction.eip1559) {
-        unsignedTx = await FeeMarketEIP1559Transaction.fromSerializedTx(
-          Buffer.from(transaction.serializedTxHex, 'hex')
-        );
+        unsignedTx = FeeMarketEIP1559Transaction.fromSerializedTx(Buffer.from(transaction.serializedTxHex, 'hex'));
       } else {
-        unsignedTx = await LegacyTransaction.fromSerializedTx(Buffer.from(transaction.serializedTxHex, 'hex'));
+        unsignedTx = LegacyTransaction.fromSerializedTx(Buffer.from(transaction.serializedTxHex, 'hex'));
       }
       const signedTx = this.getSignedTxFromSignature(ethCommmon, unsignedTx, finalSignature);
       broadcastableTransactions.push({
         serializedTx: addHexPrefix(signedTx.serialize().toString('hex')),
       });
 
-      if (i === req.length - 1 && transaction.coinSpecific!.lastScanIndex) {
-        lastScanIndex = transaction.coinSpecific!.lastScanIndex as number;
+      if (i === req.length - 1 && transaction.coinSpecific?.lastScanIndex) {
+        lastScanIndex = transaction.coinSpecific?.lastScanIndex as number;
       }
     }
 
@@ -2112,11 +2113,7 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin {
     if (!_.isUndefined(params.derivationSeed) && typeof params.derivationSeed !== 'string') {
       throw new Error('invalid derivationSeed');
     }
-    if (
-      _.isUndefined(params.bitgoDestinationAddress) ||
-      typeof params.bitgoDestinationAddress !== 'string' ||
-      !this.isValidAddress(params.bitgoDestinationAddress)
-    ) {
+    if (_.isUndefined(params.recoveryDestination) || !this.isValidAddress(params.recoveryDestination)) {
       throw new Error('missing or invalid destinationAddress');
     }
   }
@@ -2125,17 +2122,19 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin {
    * Helper function for recover()
    * This transforms the unsigned transaction information into a format the BitGo offline vault expects
    * @param {UnformattedTxInfo} txInfo - tx info
-   * @param {EthLikeTxLib.Transaction | EthLikeTxLib.FeeMarketEIP1559Transaction} ethTx - the ethereumjs tx object
+   * @param {LegacyTransaction | FeeMarketEIP1559Transaction} ethTx - the ethereumjs tx object
    * @param {string} derivationPath - the derivationPath
    * @param {number} nonce - the nonce of the backup key address
    * @param {Buffer} gasPrice - gas price for the tx
    * @param {number} gasLimit - gas limit for the tx
    * @param {EIP1559} eip1559 - eip1559 params
+   * @param replayProtectionOptions
+   * @param commonKeyChain
    * @returns {Promise<OfflineVaultTxInfo>}
    */
   private buildTxRequestForOfflineVaultMPCv2(
     txInfo: UnformattedTxInfo,
-    ethTx: EthLikeTxLib.Transaction | EthLikeTxLib.FeeMarketEIP1559Transaction,
+    ethTx: LegacyTransaction | FeeMarketEIP1559Transaction,
     derivationPath: string,
     nonce: number,
     gasPrice: Buffer,
@@ -2154,7 +2153,10 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin {
 
     const unsignedTx: UnsignedTransactionTss = {
       serializedTxHex: ethTx.serialize().toString('hex'),
-      signableHex: ethTx.getMessageToSign(false).toString('hex'),
+      signableHex:
+        ethTx instanceof FeeMarketEIP1559Transaction
+          ? ethTx.getMessageToSign(false).toString('hex')
+          : Buffer.from(RLP.encode(bufArrToArr(ethTx.getMessageToSign(false)))).toString('hex'),
       derivationPath: derivationPath,
       feeInfo: {
         fee: fee,
@@ -2194,8 +2196,8 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin {
   }
 
   private async buildTssRecoveryTxn(baseAddress: string, gasPrice: any, gasLimit: any, params: RecoverOptions) {
-    const nonce = await this.getAddressNonce(baseAddress, params.apiKey);
     const txAmount = await this.validateBalanceAndGetTxAmount(baseAddress, gasPrice, gasLimit, params.apiKey);
+    const nonce = await this.getAddressNonce(baseAddress, params.apiKey);
     const recipients = [
       {
         address: params.recoveryDestination,

modules/bitgo/test/v2/unit/recovery.ts

@@ -1383,14 +1383,19 @@ describe('Recovery:', function () {
           walletPassphrase: ethLikeDKLSKeycard.walletPassphrase,
           eip1559: {
             maxPriorityFeePerGas: 3,
-            maxFeePerGas: 20,
+            maxFeePerGas: 20000000000,
           },
           isTss: true,
           replayProtectionOptions: {
             chain: chain,
           },
         };
 
+        if (coin === 'tbsc') {
+          recoveryParams.eip1559 = undefined;
+          recoveryParams['gasPrice'] = 20000000000;
+        }
+
         const recovery = await basecoin.recover(recoveryParams);
 
         should.exist(recovery);
@@ -1404,7 +1409,7 @@ describe('Recovery:', function () {
         finalTx.common.chainIdBN().toNumber().should.equal(chain);
         baseAddress.should.equal(senderAddress);
         recoveryParams.recoveryDestination.should.equal(finalTx.to?.toString());
-        Number(finalTx.value).should.equal(999999999990000000);
+        Number(finalTx.value).should.equal(990000000000000000);
       }
     });
 

modules/sdk-coin-ethw/test/unit/index.ts

@@ -51,7 +51,6 @@ describe('Ethereum pow', function () {
         recoveryDestination: '0xac05da78464520aa7c9d4c19bd7a440b111b3054',
         replayProtectionOptions: {
           chain: 10001,
-          hardfork: 'london',
         },
       };
     });

modules/sdk-coin-xdc/test/unit/xdc.ts

@@ -8,7 +8,7 @@ import { UnsignedSweepTxMPCv2 } from '@bitgo/abstract-eth';
 import { mockDataUnsignedSweep, mockDataNonBitGoRecovery } from '../resources';
 import nock from 'nock';
 import { common } from '@bitgo/sdk-core';
-import { FeeMarketEIP1559Transaction } from '@ethereumjs/tx';
+import { Transaction } from '@ethereumjs/tx';
 import { stripHexPrefix } from '@ethereumjs/util';
 
 const bitgo: TestBitGoAPI = TestBitGo.decorate(BitGoAPI, { env: 'test' });
@@ -49,9 +49,9 @@ describe('xdc', function () {
 
 describe('Build Unsigned Sweep for Self-Custody Cold Wallets - (MPCv2)', function () {
   const bitgo = TestBitGo.decorate(BitGoAPI, { env: 'test' });
+  bitgo.register('txdc', Txdc.createInstance);
   const explorerUrl = common.Environments[bitgo.getEnv()].xdcExplorerBaseUrl as string;
-  const maxFeePerGasvalue = 20000000000;
-  const maxPriorityFeePerGasValue = 10000000000;
+  const gasPrice = 20000000000;
   const gasLimitValue = 500000;
   const chain_id = 51;
 
@@ -73,11 +73,11 @@ describe('Build Unsigned Sweep for Self-Custody Cold Wallets - (MPCv2)', functio
       walletContractAddress: mockDataUnsignedSweep.walletBaseAddress,
       recoveryDestination: mockDataUnsignedSweep.recoveryDestination,
       isTss: true,
-      eip1559: { maxFeePerGas: maxFeePerGasvalue, maxPriorityFeePerGas: maxPriorityFeePerGasValue },
+      gasPrice: gasPrice,
       gasLimit: gasLimitValue,
       replayProtectionOptions: {
         chain: chain_id,
-        hardfork: 'london',
+        hardfork: 'petersburg',
       },
     })) as UnsignedSweepTxMPCv2;
     should.exist(transaction);
@@ -105,9 +105,9 @@ describe('Build Unsigned Sweep for Self-Custody Cold Wallets - (MPCv2)', functio
 
 describe('Non Bitgo Recovery for Hot Wallets', function () {
   const bitgo = TestBitGo.decorate(BitGoAPI, { env: 'test' });
+  bitgo.register('txdc', Txdc.createInstance);
   const explorerUrl = common.Environments[bitgo.getEnv()].xdcExplorerBaseUrl as string;
-  const maxFeePerGasvalue = 20000000000;
-  const maxPriorityFeePerGasValue = 10000000000;
+  const gasPrice = 20000000000;
   const chain_id = 51;
   const gasLimitvalue = 500000;
 
@@ -130,17 +130,17 @@ describe('Non Bitgo Recovery for Hot Wallets', function () {
       walletPassphrase: mockDataNonBitGoRecovery.walletPassphrase,
       recoveryDestination: mockDataNonBitGoRecovery.recoveryDestination,
       isTss: true,
-      eip1559: { maxFeePerGas: maxFeePerGasvalue, maxPriorityFeePerGas: maxPriorityFeePerGasValue },
+      gasPrice: gasPrice,
       gasLimit: gasLimitvalue,
       replayProtectionOptions: {
         chain: chain_id,
-        hardfork: 'london',
+        hardfork: 'petersburg',
       },
     });
     should.exist(transaction);
     transaction.should.have.property('id');
     transaction.should.have.property('tx');
-    const tx = FeeMarketEIP1559Transaction.fromSerializedTx(Buffer.from(stripHexPrefix(transaction.tx), 'hex'));
+    const tx = Transaction.fromSerializedTx(Buffer.from(stripHexPrefix(transaction.tx), 'hex'));
     tx.getSenderAddress().toString().should.equal(mockDataNonBitGoRecovery.walletRootAddress);
     const jsonTx = tx.toJSON();
     jsonTx.to?.should.equal(mockDataNonBitGoRecovery.recoveryDestination);

yarn.lock

@@ -1764,7 +1764,7 @@
   resolved "https://registry.npmjs.org/@ethereumjs/rlp/-/rlp-5.0.0.tgz#dd81b32b2237bc32fb1b54534f8ff246a6c89d9b"
   integrity sha512-WuS1l7GJmB0n0HsXLozCoEFc9IwYgf3l0gCkKVYgR67puVF1O4OpEaN0hWmm1c+iHUHFCKt1hJrvy5toLg+6ag==
 
-"@ethereumjs/rlp@^4.0.0-beta.2":
+"@ethereumjs/rlp@^4.0.0", "@ethereumjs/rlp@^4.0.0-beta.2":
   version "4.0.1"
   resolved "https://registry.npmjs.org/@ethereumjs/rlp/-/rlp-4.0.1.tgz#626fabfd9081baab3d0a3074b0c7ecaf674aaa41"
   integrity sha512-tqsQiBQDQdmPWE1xkkBq4rlSW5QZpLOUJ5RJh2/9fug+q9tnUhuZoVLk7s0scUIKTOzEtR72DFBXI4WiZcMpvw==