From 843b46332d277fa1a46737c186126e7572bbef00 Mon Sep 17 00:00:00 2001 From: Kamlesh Mugdiya Date: Thu, 14 May 2026 13:09:04 +0530 Subject: [PATCH] fix(abstract-eth): verify inner batch calldata recipients Decode the inner batch(address[],uint256[]) calldata embedded in txPrebuild.txHex and compare each (address, amount) pair to the user-supplied recipients. Without this check, the verifier validated only the outer batcher contract address and the total amount, so a compromised platform could swap inner recipients while preserving the outer wrapper checks and redirect batched payouts. Fails closed when txHex is missing, the outer selector is not sendMultiSig, or the inner selector is not batch(address[],uint256[]). Ticket: CGD-1319 --- .../src/abstractEthLikeNewCoins.ts | 93 +++++++ modules/abstract-eth/src/lib/iface.ts | 9 + modules/abstract-eth/src/lib/utils.ts | 31 +++ modules/abstract-eth/src/lib/walletUtil.ts | 5 + modules/abstract-eth/test/unit/utils.ts | 48 +++- modules/sdk-coin-eth/test/unit/eth.ts | 242 +++++++++++++++++- 6 files changed, 420 insertions(+), 8 deletions(-) diff --git a/modules/abstract-eth/src/abstractEthLikeNewCoins.ts b/modules/abstract-eth/src/abstractEthLikeNewCoins.ts index 3effa6a05a..1844519158 100644 --- a/modules/abstract-eth/src/abstractEthLikeNewCoins.ts +++ b/modules/abstract-eth/src/abstractEthLikeNewCoins.ts @@ -23,6 +23,7 @@ import { MPCTx, MPCTxs, ParsedTransaction, + ITransactionRecipient, ParseTransactionOptions, PrebuildTransactionResult, PresignTransactionOptions as BasePresignTransactionOptions, @@ -71,8 +72,11 @@ import secp256k1 from 'secp256k1'; import { AbstractEthLikeCoin } from './abstractEthLikeCoin'; import { EthLikeToken } from './ethLikeToken'; import { + batchMethodId, calculateForwarderV1Address, coinFamiliesWithL1Fees, + decodeBatchTransferData, + decodeNativeTransferData, decodeTransferData, ERC1155TransferBuilder, ERC721TransferBuilder, @@ -83,6 +87,7 @@ import { getRawDecoded, getToken, KeyPair as KeyPairLib, + sendMultisigMethodId, TransactionBuilder, TransferBuilder, } from './lib'; @@ -1633,6 +1638,87 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin { }; } + /** + * Verify that the inner batch(address[],uint256[]) calldata embedded in txPrebuild.txHex matches + * the user-supplied recipients. Throws via throwRecipientMismatch if any pair differs or if the + * calldata cannot be decoded. The verifier fails closed: missing txHex, an unexpected outer + * selector, or an unexpected inner selector all reject. + */ + private async verifyBatchInnerRecipients( + txPrebuild: TransactionPrebuild, + recipients: ITransactionRecipient[], + throwRecipientMismatch: (message: string, mismatchedRecipients: Recipient[]) => Promise + ): Promise { + if (!txPrebuild.txHex) { + await throwRecipientMismatch('batch txPrebuild missing txHex required for inner calldata verification', []); + return; + } + + let outerCalldata: string; + try { + const txBuffer = optionalDeps.ethUtil.toBuffer(txPrebuild.txHex); + const decodedTx = optionalDeps.EthTx.TransactionFactory.fromSerializedData(txBuffer); + outerCalldata = optionalDeps.ethUtil.bufferToHex(decodedTx.data); + } catch (e) { + await throwRecipientMismatch(`failed to parse batch txHex: ${e instanceof Error ? e.message : String(e)}`, []); + return; + } + + if (!outerCalldata.toLowerCase().startsWith(sendMultisigMethodId)) { + await throwRecipientMismatch('batch txPrebuild outer call is not sendMultiSig', []); + return; + } + + let innerBatchData: string; + try { + innerBatchData = decodeNativeTransferData(outerCalldata).data; + } catch (e) { + await throwRecipientMismatch( + `failed to decode outer sendMultiSig wrapper: ${e instanceof Error ? e.message : String(e)}`, + [] + ); + return; + } + + if (!innerBatchData || !innerBatchData.toLowerCase().startsWith(batchMethodId)) { + await throwRecipientMismatch('batch txPrebuild inner method selector is not batch(address[],uint256[])', []); + return; + } + + let decoded; + try { + decoded = decodeBatchTransferData(innerBatchData); + } catch (e) { + await throwRecipientMismatch( + `failed to decode inner batch calldata: ${e instanceof Error ? e.message : String(e)}`, + [] + ); + return; + } + + if (decoded.recipients.length !== recipients.length) { + await throwRecipientMismatch( + `batch txPrebuild inner recipient count (${decoded.recipients.length}) does not match txParams (${recipients.length})`, + decoded.recipients + ); + return; + } + + for (let i = 0; i < recipients.length; i++) { + const expected = recipients[i]; + const actual = decoded.recipients[i]; + // Skip address comparison for non-hex inputs (e.g. unresolved ENS); mirrors normal-tx path. + if (this.isETHAddress(expected.address) && expected.address.toLowerCase() !== actual.address.toLowerCase()) { + await throwRecipientMismatch('batch txPrebuild inner recipient address does not match txParams', [actual]); + return; + } + if (!new BigNumber(expected.amount).isEqualTo(actual.amount)) { + await throwRecipientMismatch('batch txPrebuild inner recipient amount does not match txParams', [actual]); + return; + } + } + } + /** * Extract recipients from transaction hex * @param txHex - The transaction hex string @@ -3325,6 +3411,13 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin { { address: txPrebuild.recipients[0].address, amount: txPrebuild.recipients[0].amount.toString() }, ]); } + + // Decode the inner batch(address[],uint256[]) calldata and verify each (address, amount) pair + // matches user intent. Without this, a compromised platform could swap inner recipients while + // preserving the outer total amount and batcher-address checks. + if (!txParams.tokenName) { + await this.verifyBatchInnerRecipients(txPrebuild, recipients, throwRecipientMismatch); + } } else { // Check recipient address and amount for normal transaction if (recipients.length !== 1) { diff --git a/modules/abstract-eth/src/lib/iface.ts b/modules/abstract-eth/src/lib/iface.ts index a97a67eb0f..3ceea19ada 100644 --- a/modules/abstract-eth/src/lib/iface.ts +++ b/modules/abstract-eth/src/lib/iface.ts @@ -144,6 +144,15 @@ export interface NativeTransferData extends TransferData { data: string; } +export interface BatchTransferRecipient { + address: string; + amount: string; +} + +export interface BatchTransferData { + recipients: BatchTransferRecipient[]; +} + export interface WalletInitializationData { salt?: string; owners: string[]; diff --git a/modules/abstract-eth/src/lib/utils.ts b/modules/abstract-eth/src/lib/utils.ts index 05dcdadc51..0e1016d928 100644 --- a/modules/abstract-eth/src/lib/utils.ts +++ b/modules/abstract-eth/src/lib/utils.ts @@ -31,6 +31,7 @@ import { } from '@bitgo/sdk-core'; import { + BatchTransferData, ERC1155TransferData, ERC721TransferData, FlushTokensData, @@ -65,6 +66,8 @@ import { flushERC1155ForwarderTokensMethodIdV4, flushERC1155TokensTypes, flushERC1155TokensTypesv4, + batchMethodId, + batchMethodTypes, sendMultisigMethodId, sendMultisigTokenMethodId, sendMultiSigTokenTypes, @@ -459,6 +462,34 @@ export function decodeTransferData(data: string, isFirstSigner?: boolean): Trans } } +/** + * Decode the inner batch(address[],uint256[]) calldata produced for batcher contract sends. + * The data is the inner payload nested inside a sendMultiSig wrapper, not a full transaction. + * + * @param data Hex string starting with the batch method selector + * @returns Decoded recipients and amounts in the order they appear in the calldata + */ +export function decodeBatchTransferData(data: string): BatchTransferData { + if (!data.toLowerCase().startsWith(batchMethodId)) { + throw new BuildTransactionError(`Invalid batch transfer bytecode: ${data}`); + } + const [addresses, amounts] = getRawDecoded(batchMethodTypes, getBufferedByteCode(batchMethodId, data)); + if (!Array.isArray(addresses) || !Array.isArray(amounts)) { + throw new BuildTransactionError(`Invalid batch transfer bytecode: ${data}`); + } + if (addresses.length !== amounts.length) { + throw new BuildTransactionError( + `Mismatched batch address/amount array lengths: ${addresses.length} vs ${amounts.length}` + ); + } + return { + recipients: addresses.map((addr, i) => ({ + address: addHexPrefix(addr as string), + amount: new BigNumber(bufferToHex(amounts[i] as Buffer)).toFixed(), + })), + }; +} + /** * Decode the given ABI-encoded transfer data for the sendMultisigToken function and return parsed fields * diff --git a/modules/abstract-eth/src/lib/walletUtil.ts b/modules/abstract-eth/src/lib/walletUtil.ts index add5c680f6..e10d367c7f 100644 --- a/modules/abstract-eth/src/lib/walletUtil.ts +++ b/modules/abstract-eth/src/lib/walletUtil.ts @@ -1,5 +1,7 @@ export const sendMultisigMethodId = '0x39125215'; export const sendMultisigTokenMethodId = '0x0dcd7a6c'; +// Selector for batch(address[],uint256[]) used by batcher contract sends. +export const batchMethodId = '0xc00c4e9e'; export const v1CreateForwarderMethodId = '0xfb90b320'; export const v4CreateForwarderMethodId = '0x13b2f75c'; export const v1WalletInitializationFirstBytes = '0x60806040'; @@ -38,6 +40,9 @@ export const sendMultiSigTypesFirstSigner = ['string', 'address', 'uint', 'bytes export const sendMultiSigTokenTypes = ['address', 'uint', 'address', 'uint', 'uint', 'bytes']; export const sendMultiSigTokenTypesFirstSigner = ['string', 'address', 'uint', 'address', 'uint', 'uint']; +export const batchMethodName = 'batch'; +export const batchMethodTypes = ['address[]', 'uint256[]']; + export const ERC721SafeTransferTypes = ['address', 'address', 'uint256', 'bytes']; export const ERC721TransferFromTypes = ['address', 'address', 'uint256']; diff --git a/modules/abstract-eth/test/unit/utils.ts b/modules/abstract-eth/test/unit/utils.ts index 68cd2e47fd..d45f72d98a 100644 --- a/modules/abstract-eth/test/unit/utils.ts +++ b/modules/abstract-eth/test/unit/utils.ts @@ -1,12 +1,20 @@ import should from 'should'; +import EthereumAbi from 'ethereumjs-abi'; import { flushERC721TokensData, flushERC1155TokensData, decodeFlushERC721TokensData, decodeFlushERC1155TokensData, + decodeBatchTransferData, } from '../../src/lib/utils'; import { ERC721TransferBuilder } from '../../src/lib/transferBuilders/transferBuilderERC721'; -import { ERC721TransferFromMethodId, ERC721SafeTransferTypeMethodId } from '../../src/lib/walletUtil'; +import { + ERC721TransferFromMethodId, + ERC721SafeTransferTypeMethodId, + batchMethodId, + batchMethodName, + batchMethodTypes, +} from '../../src/lib/walletUtil'; describe('Abstract ETH Utils', () => { describe('ERC721 Flush Functions', () => { @@ -268,4 +276,42 @@ describe('Abstract ETH Utils', () => { decoded1155.tokenAddress.toLowerCase().should.equal(tokenAddressChecksum.toLowerCase()); }); }); + + describe('decodeBatchTransferData', () => { + const address1 = '0x1111111111111111111111111111111111111111'; + const address2 = '0x2222222222222222222222222222222222222222'; + const encodeBatch = (addresses: string[], amounts: string[]): string => { + const selector = EthereumAbi.methodID(batchMethodName, batchMethodTypes); + const args = EthereumAbi.rawEncode(batchMethodTypes, [addresses, amounts]); + return '0x' + Buffer.concat([selector, args]).toString('hex'); + }; + + it('hardcoded batchMethodId matches the runtime-computed selector', () => { + const computed = '0x' + EthereumAbi.methodID(batchMethodName, batchMethodTypes).toString('hex'); + computed.should.equal(batchMethodId); + }); + + it('round-trips encode/decode for multiple recipients', () => { + const data = encodeBatch([address1, address2], ['1000', '2500']); + const decoded = decodeBatchTransferData(data); + + decoded.recipients.length.should.equal(2); + decoded.recipients[0].address.toLowerCase().should.equal(address1); + decoded.recipients[0].amount.should.equal('1000'); + decoded.recipients[1].address.toLowerCase().should.equal(address2); + decoded.recipients[1].amount.should.equal('2500'); + }); + + it('throws on wrong method selector', () => { + should.throws(() => decodeBatchTransferData('0xdeadbeef00000000'), /Invalid batch transfer bytecode/); + }); + + it('throws when the encoded address[] and uint256[] arrays have different lengths', () => { + // Encode the batch payload directly with mismatched array lengths. + const payload = EthereumAbi.rawEncode(batchMethodTypes, [[address1, address2], ['1000']]); + const tampered = + '0x' + Buffer.concat([EthereumAbi.methodID(batchMethodName, batchMethodTypes), payload]).toString('hex'); + should.throws(() => decodeBatchTransferData(tampered), /Mismatched batch address\/amount array lengths/); + }); + }); }); diff --git a/modules/sdk-coin-eth/test/unit/eth.ts b/modules/sdk-coin-eth/test/unit/eth.ts index d3aee49106..cfde8115a7 100644 --- a/modules/sdk-coin-eth/test/unit/eth.ts +++ b/modules/sdk-coin-eth/test/unit/eth.ts @@ -5,6 +5,10 @@ import sinon from 'sinon'; import { bip32 } from '@bitgo/secp256k1'; import * as secp256k1 from 'secp256k1'; import request from 'superagent'; +import { Transaction as LegacyTransaction } from '@ethereumjs/tx'; +import EthereumAbi from 'ethereumjs-abi'; +import { addHexPrefix } from 'ethereumjs-util'; +import BigNumber from 'bignumber.js'; import { common, generateRandomPassword, @@ -39,6 +43,53 @@ import { ethTssBackupKey } from './fixtures/ethTssBackupKey'; nock.enableNetConnect(); +/** + * Build a valid serialized batch transaction hex. The outer call is sendMultiSig to the batcher + * contract; its inner `data` field is the batch(address[],uint256[]) calldata. Mirrors the + * production encoding at abstractEthLikeNewCoins.ts:1820 so verifyTransaction can decode it. + */ +function buildBatchTxHex( + recipients: { address: string; amount: string }[], + batcherAddress: string, + opts: { + walletContractAddress?: string; + sequenceId?: number; + expireTime?: number; + innerMethodSignature?: { name: string; types: string[] }; + overrideInnerAddresses?: string[]; + overrideInnerAmounts?: string[]; + } = {} +): string { + const walletContractAddress = opts.walletContractAddress || '0x' + '11'.repeat(20); + const sequenceId = opts.sequenceId ?? 1; + const expireTime = opts.expireTime ?? 1700000000; + const innerSig = opts.innerMethodSignature || { name: 'batch', types: ['address[]', 'uint256[]'] }; + const innerAddresses = opts.overrideInnerAddresses ?? recipients.map((r) => r.address); + const innerAmounts = opts.overrideInnerAmounts ?? recipients.map((r) => r.amount); + + const innerData = Buffer.concat([ + EthereumAbi.methodID(innerSig.name, innerSig.types), + EthereumAbi.rawEncode(innerSig.types, [innerAddresses, innerAmounts]), + ]); + const total = recipients.reduce((s, r) => s.plus(r.amount), new BigNumber(0)).toFixed(); + const sendMultisigData = Buffer.concat([ + EthereumAbi.methodID('sendMultiSig', ['address', 'uint', 'bytes', 'uint', 'uint', 'bytes']), + EthereumAbi.rawEncode( + ['address', 'uint', 'bytes', 'uint', 'uint', 'bytes'], + [batcherAddress, total, innerData, expireTime, sequenceId, Buffer.alloc(0)] + ), + ]); + const tx = LegacyTransaction.fromTxData({ + to: walletContractAddress, + data: addHexPrefix(sendMultisigData.toString('hex')), + nonce: 0, + gasPrice: 20000000000, + gasLimit: 500000, + value: 0, + }); + return addHexPrefix(tx.serialize().toString('hex')); +} + describe('ETH:', function () { let bitgo: TestBitGoAPI; let hopTxBitgoSignature; @@ -216,20 +267,21 @@ describe('ETH:', function () { it('should verify a batch txPrebuild from the bitgo server that matches the client txParams', async function () { const coin = bitgo.coin('teth') as Teth; const wallet = new Wallet(bitgo, coin, {}); + const batcherAddress = (coin?.staticsCoin?.network as EthereumNetwork).batcherContractAddress as string; + const batchRecipients = [ + { amount: '1000000000000', address: address1 }, + { amount: '2500000000000', address: address2 }, + ]; const txParams = { - recipients: [ - { amount: '1000000000000', address: address1 }, - { amount: '2500000000000', address: address2 }, - ], + recipients: batchRecipients, wallet: wallet, walletPassphrase: 'fakeWalletPassphrase', }; const txPrebuild = { - recipients: [ - { amount: '3500000000000', address: (coin?.staticsCoin?.network as EthereumNetwork).batcherContractAddress }, - ], + recipients: [{ amount: '3500000000000', address: batcherAddress }], + txHex: buildBatchTxHex(batchRecipients, batcherAddress), nextContractSequenceId: 0, gasPrice: 20000000000, gasLimit: 500000, @@ -541,6 +593,182 @@ describe('ETH:', function () { .should.be.rejectedWith('recipient address of txPrebuild does not match batcher address'); }); + it('should reject a batch txPrebuild that omits txHex', async function () { + const coin = bitgo.coin('teth') as Teth; + const wallet = new Wallet(bitgo, coin, {}); + const batcherAddress = (coin?.staticsCoin?.network as EthereumNetwork).batcherContractAddress as string; + + const txParams = { + recipients: [ + { amount: '1000000000000', address: address1 }, + { amount: '2500000000000', address: address2 }, + ], + wallet: wallet, + walletPassphrase: 'fakeWalletPassphrase', + }; + + const txPrebuild = { + recipients: [{ amount: '3500000000000', address: batcherAddress }], + // no txHex + nextContractSequenceId: 0, + gasPrice: 20000000000, + gasLimit: 500000, + isBatch: true, + coin: 'teth', + walletId: 'fakeWalletId', + walletContractAddress: 'fakeWalletContractAddress', + }; + + await coin + .verifyTransaction({ txParams, txPrebuild: txPrebuild as any, wallet, verification: {} }) + .should.be.rejectedWith(/missing txHex required for inner calldata verification/); + }); + + it('should reject a batch txPrebuild whose inner recipient address differs from txParams', async function () { + const coin = bitgo.coin('teth') as Teth; + const wallet = new Wallet(bitgo, coin, {}); + const batcherAddress = (coin?.staticsCoin?.network as EthereumNetwork).batcherContractAddress as string; + + const userRecipients = [ + { amount: '1000000000000', address: address1 }, + { amount: '2500000000000', address: address2 }, + ]; + // Server-supplied txHex routes the second payment to an attacker address instead of address2. + const attackerAddress = '0xdeaddeaddeaddeaddeaddeaddeaddeaddeaddead'; + + const txParams = { + recipients: userRecipients, + wallet: wallet, + walletPassphrase: 'fakeWalletPassphrase', + }; + + const txPrebuild = { + recipients: [{ amount: '3500000000000', address: batcherAddress }], + txHex: buildBatchTxHex(userRecipients, batcherAddress, { + overrideInnerAddresses: [address1, attackerAddress], + }), + nextContractSequenceId: 0, + gasPrice: 20000000000, + gasLimit: 500000, + isBatch: true, + coin: 'teth', + walletId: 'fakeWalletId', + walletContractAddress: 'fakeWalletContractAddress', + }; + + await coin + .verifyTransaction({ txParams, txPrebuild: txPrebuild as any, wallet, verification: {} }) + .should.be.rejectedWith('batch txPrebuild inner recipient address does not match txParams'); + }); + + it('should reject a batch txPrebuild whose inner recipient amount differs from txParams', async function () { + const coin = bitgo.coin('teth') as Teth; + const wallet = new Wallet(bitgo, coin, {}); + const batcherAddress = (coin?.staticsCoin?.network as EthereumNetwork).batcherContractAddress as string; + + const userRecipients = [ + { amount: '1000000000000', address: address1 }, + { amount: '2500000000000', address: address2 }, + ]; + + const txParams = { + recipients: userRecipients, + wallet: wallet, + walletPassphrase: 'fakeWalletPassphrase', + }; + + // Total is unchanged (so outer check passes) but the per-recipient split is tampered. + const txPrebuild = { + recipients: [{ amount: '3500000000000', address: batcherAddress }], + txHex: buildBatchTxHex(userRecipients, batcherAddress, { + overrideInnerAmounts: ['500000000000', '3000000000000'], + }), + nextContractSequenceId: 0, + gasPrice: 20000000000, + gasLimit: 500000, + isBatch: true, + coin: 'teth', + walletId: 'fakeWalletId', + walletContractAddress: 'fakeWalletContractAddress', + }; + + await coin + .verifyTransaction({ txParams, txPrebuild: txPrebuild as any, wallet, verification: {} }) + .should.be.rejectedWith('batch txPrebuild inner recipient amount does not match txParams'); + }); + + it('should reject a batch txPrebuild whose inner method selector is not batch(address[],uint256[])', async function () { + const coin = bitgo.coin('teth') as Teth; + const wallet = new Wallet(bitgo, coin, {}); + const batcherAddress = (coin?.staticsCoin?.network as EthereumNetwork).batcherContractAddress as string; + + const userRecipients = [ + { amount: '1000000000000', address: address1 }, + { amount: '2500000000000', address: address2 }, + ]; + + const txParams = { + recipients: userRecipients, + wallet: wallet, + walletPassphrase: 'fakeWalletPassphrase', + }; + + const txPrebuild = { + recipients: [{ amount: '3500000000000', address: batcherAddress }], + txHex: buildBatchTxHex(userRecipients, batcherAddress, { + innerMethodSignature: { name: 'notBatch', types: ['address[]', 'uint256[]'] }, + }), + nextContractSequenceId: 0, + gasPrice: 20000000000, + gasLimit: 500000, + isBatch: true, + coin: 'teth', + walletId: 'fakeWalletId', + walletContractAddress: 'fakeWalletContractAddress', + }; + + await coin + .verifyTransaction({ txParams, txPrebuild: txPrebuild as any, wallet, verification: {} }) + .should.be.rejectedWith(/inner method selector is not batch/); + }); + + it('should reject a batch txPrebuild whose inner recipient count differs from txParams', async function () { + const coin = bitgo.coin('teth') as Teth; + const wallet = new Wallet(bitgo, coin, {}); + const batcherAddress = (coin?.staticsCoin?.network as EthereumNetwork).batcherContractAddress as string; + + const userRecipients = [ + { amount: '1000000000000', address: address1 }, + { amount: '2500000000000', address: address2 }, + ]; + + const txParams = { + recipients: userRecipients, + wallet: wallet, + walletPassphrase: 'fakeWalletPassphrase', + }; + + const txPrebuild = { + recipients: [{ amount: '3500000000000', address: batcherAddress }], + // Inner calldata sweeps the full total to a single attacker recipient. + txHex: buildBatchTxHex(userRecipients, batcherAddress, { + overrideInnerAddresses: ['0xabababababababababababababababababababab'], + overrideInnerAmounts: ['3500000000000'], + }), + nextContractSequenceId: 0, + gasPrice: 20000000000, + gasLimit: 500000, + isBatch: true, + coin: 'teth', + walletId: 'fakeWalletId', + walletContractAddress: 'fakeWalletContractAddress', + }; + + await coin + .verifyTransaction({ txParams, txPrebuild: txPrebuild as any, wallet, verification: {} }) + .should.be.rejectedWith(/inner recipient count \(1\) does not match txParams \(2\)/); + }); + it('should reject a normal txPrebuild from the bitgo server with the wrong amount', async function () { const coin = bitgo.coin('teth') as Teth; const wallet = new Wallet(bitgo, coin, {});