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