From 1af07cf727ef92dcab8b08a92505afba021ed5a2 Mon Sep 17 00:00:00 2001 From: Lokesh Chandra Date: Mon, 31 Aug 2026 13:04:42 +0530 Subject: [PATCH] revert(abstract-eth): remove signableHex/serializedTxHex consistency check This reverts merge commit 220142c7fa (PR #9526). The check treats EIP-155 unsigned txs with empty v as Ethereum chainId 1, so BSC/XDC sendMany and token withdrawals fail with a false tampering error. TICKET: WCI-1398 Co-authored-by: Cursor --- .../src/abstractEthLikeNewCoins.ts | 20 +-------- modules/sdk-coin-bsc/package.json | 5 +-- modules/sdk-coin-bsc/test/unit/bsc.ts | 42 ------------------- modules/sdk-coin-xdc/package.json | 4 +- modules/sdk-coin-xdc/test/unit/xdc.ts | 41 ------------------ .../src/bitgo/utils/tss/ecdsa/ecdsaMPCv2.ts | 8 ---- modules/sdk-core/src/bitgo/utils/tss/index.ts | 1 - .../bitgo/utils/tss/signableConsistency.ts | 18 -------- 8 files changed, 4 insertions(+), 135 deletions(-) delete mode 100644 modules/sdk-core/src/bitgo/utils/tss/signableConsistency.ts diff --git a/modules/abstract-eth/src/abstractEthLikeNewCoins.ts b/modules/abstract-eth/src/abstractEthLikeNewCoins.ts index 75110f12d4..5fdd787790 100644 --- a/modules/abstract-eth/src/abstractEthLikeNewCoins.ts +++ b/modules/abstract-eth/src/abstractEthLikeNewCoins.ts @@ -17,7 +17,6 @@ import { HalfSignedTransaction, InvalidAddressError, InvalidAddressVerificationObjectPropertyError, - InvalidTransactionError, IWallet, KeyPair, MPCSweepRecoveryOptions, @@ -49,7 +48,6 @@ import { DeriveAddressOptions, DeriveAddressResult, NO_RECIPIENT_TX_TYPES, - CoinWithSignableConsistency, } from '@bitgo/sdk-core'; import { getDerivationPath } from '@bitgo/sdk-lib-mpc'; import { bip32 } from '@bitgo/secp256k1'; @@ -63,7 +61,7 @@ import { } from '@bitgo/statics'; import type * as EthLikeCommon from '@ethereumjs/common'; import type * as EthLikeTxLib from '@ethereumjs/tx'; -import { FeeMarketEIP1559Transaction, Transaction as LegacyTransaction, TransactionFactory } 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'; @@ -513,7 +511,7 @@ export const optionalDeps = { }, }; -export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin implements CoinWithSignableConsistency { +export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin { static hopTransactionSalt = 'bitgoHopAddressRequestSalt'; protected readonly sendMethodName: 'sendMultiSig' | 'sendMultiSigToken'; @@ -3177,19 +3175,6 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin implem /** * Verify if a tss transaction is valid * - /** @inheritdoc CoinWithSignableConsistency */ - assertSignableConsistency(serializedTxHex: string, signableHex: string): void { - const ethTx = TransactionFactory.fromSerializedData(toBuffer(addHexPrefix(serializedTxHex))); - const derivedSignableHex = - ethTx instanceof FeeMarketEIP1559Transaction - ? ethTx.getMessageToSign(false).toString('hex') - : Buffer.from(RLP.encode(bufArrToArr((ethTx as LegacyTransaction).getMessageToSign(false)))).toString('hex'); - if (derivedSignableHex !== signableHex) { - throw new InvalidTransactionError('signableHex is inconsistent with serializedTxHex: possible server tampering'); - } - } - - /** * @param {VerifyEthTransactionOptions} params * @param {TransactionParams} params.txParams - params object passed to send * @param {TransactionPrebuild} params.txPrebuild - prebuild object returned by server @@ -3228,7 +3213,6 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin implem if (!wallet || !txPrebuild) { throw new Error('missing params'); } - if (txParams.hop && txParams.recipients && txParams.recipients.length > 1) { throw new Error('tx cannot be both a batch and hop transaction'); } diff --git a/modules/sdk-coin-bsc/package.json b/modules/sdk-coin-bsc/package.json index ad901c6ae6..4c19a4a9e3 100644 --- a/modules/sdk-coin-bsc/package.json +++ b/modules/sdk-coin-bsc/package.json @@ -48,10 +48,7 @@ }, "devDependencies": { "@bitgo/sdk-api": "^2.4.2", - "@bitgo/sdk-test": "^9.1.71", - "@ethereumjs/rlp": "^4.0.0", - "@ethereumjs/tx": "^3.3.0", - "ethereumjs-util": "7.1.5" + "@bitgo/sdk-test": "^9.1.71" }, "gitHead": "18e460ddf02de2dbf13c2aa243478188fb539f0c", "files": [ diff --git a/modules/sdk-coin-bsc/test/unit/bsc.ts b/modules/sdk-coin-bsc/test/unit/bsc.ts index 29ed2d2ae8..debb7ffd42 100644 --- a/modules/sdk-coin-bsc/test/unit/bsc.ts +++ b/modules/sdk-coin-bsc/test/unit/bsc.ts @@ -3,9 +3,6 @@ import 'should'; import { TestBitGo, TestBitGoAPI } from '@bitgo/sdk-test'; import { BitGoAPI } from '@bitgo/sdk-api'; import { TransactionType, Wallet } from '@bitgo/sdk-core'; -import { Transaction as LegacyTransaction } from '@ethereumjs/tx'; -import { RLP } from '@ethereumjs/rlp'; -import { bufArrToArr } from 'ethereumjs-util'; import { Bsc, Tbsc } from '../../src/index'; import { TransactionBuilder } from '../../src/lib'; @@ -225,44 +222,5 @@ describe('Native BNB', function () { }) .should.be.rejectedWith('destination address does not match with the recipient address'); }); - - describe('assertSignableConsistency (Gap 2 / WCI-1398)', function () { - function stripHex(hex: string): string { - return hex.startsWith('0x') ? hex.slice(2) : hex; - } - - async function buildSerializedTxHex(address: string): Promise { - const txBuilder = getBuilder('tbsc') as TransactionBuilder; - txBuilder.type(TransactionType.SingleSigSend); - txBuilder.fee({ fee: '10', gasLimit: '21000' }); - txBuilder.counter(1); - txBuilder.contract(address); - txBuilder.value(transferAmount); - const tx = await txBuilder.build(); - return stripHex(tx.toBroadcastFormat()); - } - - function deriveSignableHex(serializedTxHex: string): string { - const legacyTx = LegacyTransaction.fromSerializedTx(Buffer.from(serializedTxHex, 'hex')); - return Buffer.from(RLP.encode(bufArrToArr(legacyTx.getMessageToSign(false)))).toString('hex'); - } - - it('should pass when signableHex is consistent with serializedTxHex', async function () { - const coin = bitgo.coin('tbsc') as Tbsc; - const serializedTxHex = await buildSerializedTxHex(recipientAddress); - const signableHex = deriveSignableHex(serializedTxHex); - coin.assertSignableConsistency(serializedTxHex, signableHex); - }); - - it('should throw when signableHex does not match serializedTxHex (Gap 2 attack)', async function () { - const coin = bitgo.coin('tbsc') as Tbsc; - const benignSerializedTxHex = await buildSerializedTxHex(recipientAddress); - const maliciousSerializedTxHex = await buildSerializedTxHex(wrongAddress); - const tamperedSignableHex = deriveSignableHex(maliciousSerializedTxHex); - (() => coin.assertSignableConsistency(benignSerializedTxHex, tamperedSignableHex)).should.throw( - 'signableHex is inconsistent with serializedTxHex: possible server tampering' - ); - }); - }); }); }); diff --git a/modules/sdk-coin-xdc/package.json b/modules/sdk-coin-xdc/package.json index 3543d57cdc..65710f1f78 100644 --- a/modules/sdk-coin-xdc/package.json +++ b/modules/sdk-coin-xdc/package.json @@ -48,9 +48,7 @@ }, "devDependencies": { "@bitgo/sdk-api": "^2.4.2", - "@bitgo/sdk-test": "^9.1.71", - "@ethereumjs/rlp": "^4.0.0", - "ethereumjs-util": "7.1.5" + "@bitgo/sdk-test": "^9.1.71" }, "gitHead": "18e460ddf02de2dbf13c2aa243478188fb539f0c", "files": [ diff --git a/modules/sdk-coin-xdc/test/unit/xdc.ts b/modules/sdk-coin-xdc/test/unit/xdc.ts index 2e30248e6d..98cd23ce75 100644 --- a/modules/sdk-coin-xdc/test/unit/xdc.ts +++ b/modules/sdk-coin-xdc/test/unit/xdc.ts @@ -9,9 +9,7 @@ import { mockDataUnsignedSweep, mockDataNonBitGoRecovery } from '../resources'; import nock from 'nock'; import { common, TransactionType, Wallet } from '@bitgo/sdk-core'; import { Transaction } from '@ethereumjs/tx'; -import { RLP } from '@ethereumjs/rlp'; import { stripHexPrefix } from '@ethereumjs/util'; -import { bufArrToArr } from 'ethereumjs-util'; import { TransactionBuilder } from '../../src/lib'; import { getBuilder } from './getBuilder'; @@ -169,45 +167,6 @@ describe('xdc', function () { }) .should.be.rejectedWith('destination address does not match with the recipient address'); }); - - describe('assertSignableConsistency (Gap 2 / WCI-1398)', function () { - function stripHex(hex: string): string { - return hex.startsWith('0x') ? hex.slice(2) : hex; - } - - async function buildSerializedTxHex(address: string): Promise { - const txBuilder = getBuilder('txdc') as TransactionBuilder; - txBuilder.type(TransactionType.SingleSigSend); - txBuilder.fee({ fee: '10', gasLimit: '21000' }); - txBuilder.counter(1); - txBuilder.contract(address); - txBuilder.value(transferAmount); - const tx = await txBuilder.build(); - return stripHex(tx.toBroadcastFormat()); - } - - function deriveSignableHex(serializedTxHex: string): string { - const legacyTx = Transaction.fromSerializedTx(Buffer.from(serializedTxHex, 'hex')); - return Buffer.from(RLP.encode(bufArrToArr(legacyTx.getMessageToSign(false)))).toString('hex'); - } - - it('should pass when signableHex is consistent with serializedTxHex', async function () { - const coin = bitgo.coin('txdc') as Txdc; - const serializedTxHex = await buildSerializedTxHex(recipientAddress); - const signableHex = deriveSignableHex(serializedTxHex); - coin.assertSignableConsistency(serializedTxHex, signableHex); - }); - - it('should throw when signableHex does not match serializedTxHex (Gap 2 attack)', async function () { - const coin = bitgo.coin('txdc') as Txdc; - const benignSerializedTxHex = await buildSerializedTxHex(recipientAddress); - const maliciousSerializedTxHex = await buildSerializedTxHex(wrongAddress); - const tamperedSignableHex = deriveSignableHex(maliciousSerializedTxHex); - (() => coin.assertSignableConsistency(benignSerializedTxHex, tamperedSignableHex)).should.throw( - 'signableHex is inconsistent with serializedTxHex: possible server tampering' - ); - }); - }); }); }); 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 7cb9b2fb24..478107d3ef 100644 --- a/modules/sdk-core/src/bitgo/utils/tss/ecdsa/ecdsaMPCv2.ts +++ b/modules/sdk-core/src/bitgo/utils/tss/ecdsa/ecdsaMPCv2.ts @@ -52,7 +52,6 @@ import { } from '../baseTypes'; import { shouldUsePreHashedSignable } from '../preHashedSignable'; import { shouldVerifyWithSerializedTxHex } from '../serializedTxHexVerify'; -import { isCoinWithSignableConsistency } from '../signableConsistency'; import { BaseEcdsaUtils } from './base'; import { EcdsaMPCv2KeyGenSendFn, KeyGenSenderForEnterprise } from './ecdsaMPCv2KeyGenSender'; import { envRequiresBitgoPubGpgKeyConfig, isBitgoMpcPubKey } from '../../../tss/bitgoPubKeys'; @@ -959,13 +958,6 @@ export class EcdsaMPCv2Utils extends BaseEcdsaUtils { wallet: this.wallet, walletType: this.wallet.multisigType(), }); - // Gap 2 fix (WCI-1398): verifyTransaction sees serializedTxHex but signing uses - // signableHex — both are server-supplied independently. For coins that implement - // the consistency check, derive signableHex from serializedTxHex and confirm they - // match before signing. - if (shouldVerifyWithSerializedTxHex(this.baseCoin) && isCoinWithSignableConsistency(this.baseCoin)) { - this.baseCoin.assertSignableConsistency(unsignedTx.serializedTxHex, unsignedTx.signableHex); - } } else { await this.baseCoin.verifyTransaction({ txPrebuild: { txHex: unsignedTx.signableHex }, diff --git a/modules/sdk-core/src/bitgo/utils/tss/index.ts b/modules/sdk-core/src/bitgo/utils/tss/index.ts index ec728cc68f..3480ade77e 100644 --- a/modules/sdk-core/src/bitgo/utils/tss/index.ts +++ b/modules/sdk-core/src/bitgo/utils/tss/index.ts @@ -18,4 +18,3 @@ export * from './baseTypes'; export * from './addressVerification'; export * from './preHashedSignable'; export * from './recipientUtils'; -export * from './signableConsistency'; diff --git a/modules/sdk-core/src/bitgo/utils/tss/signableConsistency.ts b/modules/sdk-core/src/bitgo/utils/tss/signableConsistency.ts deleted file mode 100644 index d55908fedc..0000000000 --- a/modules/sdk-core/src/bitgo/utils/tss/signableConsistency.ts +++ /dev/null @@ -1,18 +0,0 @@ -export interface CoinWithSignableConsistency { - /** - * Verify that the server-supplied signableHex is consistent with serializedTxHex. - * For TSS_VERIFY_USE_SERIALIZED_TX_HEX coins (BSC, XDC), the MPC signing flow - * verifies serializedTxHex but signs signableHex — both are server-supplied - * independently. This method derives the expected signableHex from the - * serializedTxHex locally and throws InvalidTransactionError if they diverge. - */ - assertSignableConsistency(serializedTxHex: string, signableHex: string): void; -} - -export function isCoinWithSignableConsistency(coin: unknown): coin is CoinWithSignableConsistency { - return ( - coin !== null && - typeof coin === 'object' && - typeof (coin as CoinWithSignableConsistency).assertSignableConsistency === 'function' - ); -}