Skip to content

revert(abstract-eth): remove signableHex consistency check (WCI-1398) #9601

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
lokesh-bitgo wants to merge 1 commit into from
Closed

revert(abstract-eth): remove signableHex consistency check (WCI-1398) #9601

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 2 additions & 18 deletions modules/abstract-eth/src/abstractEthLikeNewCoins.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ import {
HalfSignedTransaction,
InvalidAddressError,
InvalidAddressVerificationObjectPropertyError,
InvalidTransactionError,
IWallet,
KeyPair,
MPCSweepRecoveryOptions,
Expand Down Expand Up @@ -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';
Expand All @@ -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';
Expand Down Expand Up @@ -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';

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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');
}
Expand Down
5 changes: 1 addition & 4 deletions modules/sdk-coin-bsc/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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": [
Expand Down
42 changes: 0 additions & 42 deletions modules/sdk-coin-bsc/test/unit/bsc.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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<string> {
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'
);
});
});
});
});
4 changes: 1 addition & 3 deletions modules/sdk-coin-xdc/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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": [
Expand Down
41 changes: 0 additions & 41 deletions modules/sdk-coin-xdc/test/unit/xdc.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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<string> {
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'
);
});
});
});
});

Expand Down
8 changes: 0 additions & 8 deletions modules/sdk-core/src/bitgo/utils/tss/ecdsa/ecdsaMPCv2.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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 },
Expand Down
1 change: 0 additions & 1 deletion modules/sdk-core/src/bitgo/utils/tss/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,4 +18,3 @@ export * from './baseTypes';
export * from './addressVerification';
export * from './preHashedSignable';
export * from './recipientUtils';
export * from './signableConsistency';
18 changes: 0 additions & 18 deletions modules/sdk-core/src/bitgo/utils/tss/signableConsistency.ts
View file
Open in desktop

This file was deleted.

Loading