Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
176 changes: 176 additions & 0 deletions modules/abstract-eth/src/abstractEthLikeNewCoins.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import {
MPCTx,
MPCTxs,
ParsedTransaction,
ITransactionRecipient,
ParseTransactionOptions,
PrebuildTransactionResult,
PresignTransactionOptions as BasePresignTransactionOptions,
Expand Down Expand Up @@ -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,
Expand All @@ -83,6 +87,7 @@ import {
getRawDecoded,
getToken,
KeyPair as KeyPairLib,
sendMultisigMethodId,
TransactionBuilder,
TransferBuilder,
} from './lib';
Expand Down Expand Up @@ -1633,6 +1638,152 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin {
};
}

/**
* Verify that the inner batch(address[],uint256[]) calldata embedded in txPrebuild.txHex matches
* the user-supplied recipients. Used by the multi-sig (sendMultiSig) batch path. Throws via
* throwRecipientMismatch if any pair differs or if the calldata cannot be decoded. 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<never>
): Promise<void> {
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;
}

await this.compareBatchCalldataAgainstRecipients(innerBatchData, recipients, throwRecipientMismatch);
}

/**
* Verify that the batch(address[],uint256[]) calldata embedded directly in the outer TSS
* transaction matches the user-supplied recipients. TSS wallets are EOAs controlled by MPC keys
* and call the batcher contract directly, so the outer tx.data IS the batch calldata (no
* sendMultiSig wrapper). Verifies the outer to == batcherContractAddress and the outer value
* matches the total amount, then decodes and compares each inner (address, amount) pair.
*/
private async verifyTssBatchInnerRecipients(
txPrebuild: TransactionPrebuild,
recipients: ITransactionRecipient[],
batcherContractAddress: string,
throwRecipientMismatch: (message: string, mismatchedRecipients: Recipient[]) => Promise<never>
): Promise<void> {
if (!txPrebuild.txHex) {
await throwRecipientMismatch('batch txPrebuild missing txHex required for inner calldata verification', []);
return;
}

let outerTo: string;
let outerValue: string;
let outerCalldata: string;
try {
const txBuffer = optionalDeps.ethUtil.toBuffer(txPrebuild.txHex);
const decodedTx = optionalDeps.EthTx.TransactionFactory.fromSerializedData(txBuffer);
outerTo = decodedTx.to ? decodedTx.to.toString() : '';
outerValue = decodedTx.value.toString();
outerCalldata = optionalDeps.ethUtil.bufferToHex(decodedTx.data);
} catch (e) {
await throwRecipientMismatch(`failed to parse batch txHex: ${e instanceof Error ? e.message : String(e)}`, []);
return;
}

if (!outerTo || outerTo.toLowerCase() !== batcherContractAddress.toLowerCase()) {
await throwRecipientMismatch('batch txPrebuild outer to does not match batcher contract address', [
{ address: outerTo, amount: outerValue },
]);
return;
}

const expectedTotal = recipients
.reduce((sum, r) => sum.plus(new BigNumber(r.amount as string | number)), new BigNumber(0))
.toFixed();
if (!new BigNumber(outerValue).isEqualTo(expectedTotal)) {
await throwRecipientMismatch(
`batch txPrebuild outer value (${outerValue}) does not match sum of txParams recipients (${expectedTotal})`,
[{ address: outerTo, amount: outerValue }]
);
return;
}

await this.compareBatchCalldataAgainstRecipients(outerCalldata, recipients, throwRecipientMismatch);
}

/**
* Shared comparator: verify that the given batch calldata starts with the batch selector,
* decode it, and compare each inner (address, amount) pair to the user-supplied recipients.
*/
private async compareBatchCalldataAgainstRecipients(
batchCalldata: string,
recipients: ITransactionRecipient[],
throwRecipientMismatch: (message: string, mismatchedRecipients: Recipient[]) => Promise<never>
): Promise<void> {
if (!batchCalldata || !batchCalldata.toLowerCase().startsWith(batchMethodId)) {
await throwRecipientMismatch('batch txPrebuild inner method selector is not batch(address[],uint256[])', []);
return;
}

let decoded;
try {
decoded = decodeBatchTransferData(batchCalldata);
} 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
Expand Down Expand Up @@ -3088,6 +3239,7 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin {
* @throws {TxIntentMismatchRecipientError} if transaction recipients don't match user intent
*/
async verifyTssTransaction(params: VerifyEthTransactionOptions): Promise<boolean> {
const ethNetwork = this.getNetwork();
const { txParams, txPrebuild, wallet } = params;

// Helper to throw TxIntentMismatchRecipientError with recipient details
Expand Down Expand Up @@ -3123,6 +3275,23 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin {
throw new Error('tx cannot be both a batch and hop transaction');
}

// TSS batch sends call the batcher contract directly (no sendMultiSig wrapper). Decode the
// inner batch calldata and compare each (address, amount) pair to user intent. Token batches
// are not supported through the same pattern, so they keep existing behavior.
if (!txParams.tokenName && txParams.recipients && txParams.recipients.length > 1) {
const batcherContractAddress = ethNetwork?.batcherContractAddress;
if (!batcherContractAddress) {
await throwRecipientMismatch('batch txPrebuild for tss has no configured batcher contract address', []);
} else {
await this.verifyTssBatchInnerRecipients(
txPrebuild,
txParams.recipients,
batcherContractAddress,
throwRecipientMismatch
);
}
}

if (txParams.type && ['transfer'].includes(txParams.type)) {
if (txParams.recipients && txParams.recipients.length === 1) {
const recipients = txParams.recipients;
Expand Down Expand Up @@ -3325,6 +3494,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);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't we do the same for verifyTssTransaction ? method

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — verifyTssTransaction previously had no batch verification at all (only single-recipient transfer + consolidation paths). I just pushed a commit that adds equivalent inner-recipient verification for the TSS path.

The TSS shape is slightly different from multi-sig:

  • Multi-sig: outer sendMultiSig(batcher, total, batchData, ...) with batch calldata nested as the data field
  • TSS: TSS wallets are EOAs, so the outer tx calls the batcher contract directly — tx.to = batcherContractAddress, tx.value = total, tx.data = batch(addr[],amt[]) (no sendMultiSig wrapper)

I extracted the inner decode/compare into a shared compareBatchCalldataAgainstRecipients helper used by both paths. New TSS-specific tests cover: missing txHex, wrong outer to, wrong outer value, mismatched inner address, and wrong inner selector.

}
} else {
// Check recipient address and amount for normal transaction
if (recipients.length !== 1) {
Expand Down
9 changes: 9 additions & 0 deletions modules/abstract-eth/src/lib/iface.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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[];
Expand Down
31 changes: 31 additions & 0 deletions modules/abstract-eth/src/lib/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import {
} from '@bitgo/sdk-core';

import {
BatchTransferData,
ERC1155TransferData,
ERC721TransferData,
FlushTokensData,
Expand Down Expand Up @@ -65,6 +66,8 @@ import {
flushERC1155ForwarderTokensMethodIdV4,
flushERC1155TokensTypes,
flushERC1155TokensTypesv4,
batchMethodId,
batchMethodTypes,
sendMultisigMethodId,
sendMultisigTokenMethodId,
sendMultiSigTokenTypes,
Expand Down Expand Up @@ -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
*
Expand Down
5 changes: 5 additions & 0 deletions modules/abstract-eth/src/lib/walletUtil.ts
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -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'];

Expand Down
48 changes: 47 additions & 1 deletion modules/abstract-eth/test/unit/utils.ts
Original file line number Diff line number Diff line change
@@ -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', () => {
Expand Down Expand Up @@ -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/);
});
});
});
2 changes: 2 additions & 0 deletions modules/sdk-coin-arbeth/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@
"devDependencies": {
"@bitgo/sdk-api": "^1.79.2",
"@bitgo/sdk-test": "^9.1.42",
"@ethereumjs/tx": "^3.3.0",
"bignumber.js": "^9.1.1",
"secp256k1": "5.0.1"
},
"gitHead": "18e460ddf02de2dbf13c2aa243478188fb539f0c",
Expand Down
Loading
Loading