Skip to content

Commit fccd267

Browse files
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. Covers both transaction signing paths: - Multi-sig: outer sendMultiSig(batcher, total, batchData, ...) wraps the batch calldata as the inner `data` field; verifyTransaction decodes the wrapper then the inner batch. - TSS: TSS wallets are EOAs and call the batcher contract directly, so the outer tx has `to = batcher`, `value = total`, and `data = batch(addr[],amt[])` with no wrapper. verifyTssTransaction now decodes the outer tx and compares inner pairs. Both paths share `compareBatchCalldataAgainstRecipients`, which fails closed on missing txHex, wrong outer selector / target / value, unexpected inner selector, or mismatched recipient count / address / amount. Tests added in: - abstract-eth/test/unit/utils.ts: decodeBatchTransferData unit tests - sdk-coin-eth/test/unit/eth.ts: multi-sig and TSS batch verification - sdk-coin-arbeth, sdk-coin-opeth, sdk-coin-polygon: per-coin batch verify smoke tests so each chain's batcher contract address is exercised through the abstract verifier. Ticket: CGD-1319
1 parent 4863c85 commit fccd267

12 files changed

Lines changed: 1021 additions & 8 deletions

File tree

modules/abstract-eth/src/abstractEthLikeNewCoins.ts

Lines changed: 176 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ import {
2323
MPCTx,
2424
MPCTxs,
2525
ParsedTransaction,
26+
ITransactionRecipient,
2627
ParseTransactionOptions,
2728
PrebuildTransactionResult,
2829
PresignTransactionOptions as BasePresignTransactionOptions,
@@ -71,8 +72,11 @@ import secp256k1 from 'secp256k1';
7172
import { AbstractEthLikeCoin } from './abstractEthLikeCoin';
7273
import { EthLikeToken } from './ethLikeToken';
7374
import {
75+
batchMethodId,
7476
calculateForwarderV1Address,
7577
coinFamiliesWithL1Fees,
78+
decodeBatchTransferData,
79+
decodeNativeTransferData,
7680
decodeTransferData,
7781
ERC1155TransferBuilder,
7882
ERC721TransferBuilder,
@@ -83,6 +87,7 @@ import {
8387
getRawDecoded,
8488
getToken,
8589
KeyPair as KeyPairLib,
90+
sendMultisigMethodId,
8691
TransactionBuilder,
8792
TransferBuilder,
8893
} from './lib';
@@ -1633,6 +1638,152 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin {
16331638
};
16341639
}
16351640

1641+
/**
1642+
* Verify that the inner batch(address[],uint256[]) calldata embedded in txPrebuild.txHex matches
1643+
* the user-supplied recipients. Used by the multi-sig (sendMultiSig) batch path. Throws via
1644+
* throwRecipientMismatch if any pair differs or if the calldata cannot be decoded. Fails closed:
1645+
* missing txHex, an unexpected outer selector, or an unexpected inner selector all reject.
1646+
*/
1647+
private async verifyBatchInnerRecipients(
1648+
txPrebuild: TransactionPrebuild,
1649+
recipients: ITransactionRecipient[],
1650+
throwRecipientMismatch: (message: string, mismatchedRecipients: Recipient[]) => Promise<never>
1651+
): Promise<void> {
1652+
if (!txPrebuild.txHex) {
1653+
await throwRecipientMismatch('batch txPrebuild missing txHex required for inner calldata verification', []);
1654+
return;
1655+
}
1656+
1657+
let outerCalldata: string;
1658+
try {
1659+
const txBuffer = optionalDeps.ethUtil.toBuffer(txPrebuild.txHex);
1660+
const decodedTx = optionalDeps.EthTx.TransactionFactory.fromSerializedData(txBuffer);
1661+
outerCalldata = optionalDeps.ethUtil.bufferToHex(decodedTx.data);
1662+
} catch (e) {
1663+
await throwRecipientMismatch(`failed to parse batch txHex: ${e instanceof Error ? e.message : String(e)}`, []);
1664+
return;
1665+
}
1666+
1667+
if (!outerCalldata.toLowerCase().startsWith(sendMultisigMethodId)) {
1668+
await throwRecipientMismatch('batch txPrebuild outer call is not sendMultiSig', []);
1669+
return;
1670+
}
1671+
1672+
let innerBatchData: string;
1673+
try {
1674+
innerBatchData = decodeNativeTransferData(outerCalldata).data;
1675+
} catch (e) {
1676+
await throwRecipientMismatch(
1677+
`failed to decode outer sendMultiSig wrapper: ${e instanceof Error ? e.message : String(e)}`,
1678+
[]
1679+
);
1680+
return;
1681+
}
1682+
1683+
await this.compareBatchCalldataAgainstRecipients(innerBatchData, recipients, throwRecipientMismatch);
1684+
}
1685+
1686+
/**
1687+
* Verify that the batch(address[],uint256[]) calldata embedded directly in the outer TSS
1688+
* transaction matches the user-supplied recipients. TSS wallets are EOAs controlled by MPC keys
1689+
* and call the batcher contract directly, so the outer tx.data IS the batch calldata (no
1690+
* sendMultiSig wrapper). Verifies the outer to == batcherContractAddress and the outer value
1691+
* matches the total amount, then decodes and compares each inner (address, amount) pair.
1692+
*/
1693+
private async verifyTssBatchInnerRecipients(
1694+
txPrebuild: TransactionPrebuild,
1695+
recipients: ITransactionRecipient[],
1696+
batcherContractAddress: string,
1697+
throwRecipientMismatch: (message: string, mismatchedRecipients: Recipient[]) => Promise<never>
1698+
): Promise<void> {
1699+
if (!txPrebuild.txHex) {
1700+
await throwRecipientMismatch('batch txPrebuild missing txHex required for inner calldata verification', []);
1701+
return;
1702+
}
1703+
1704+
let outerTo: string;
1705+
let outerValue: string;
1706+
let outerCalldata: string;
1707+
try {
1708+
const txBuffer = optionalDeps.ethUtil.toBuffer(txPrebuild.txHex);
1709+
const decodedTx = optionalDeps.EthTx.TransactionFactory.fromSerializedData(txBuffer);
1710+
outerTo = decodedTx.to ? decodedTx.to.toString() : '';
1711+
outerValue = decodedTx.value.toString();
1712+
outerCalldata = optionalDeps.ethUtil.bufferToHex(decodedTx.data);
1713+
} catch (e) {
1714+
await throwRecipientMismatch(`failed to parse batch txHex: ${e instanceof Error ? e.message : String(e)}`, []);
1715+
return;
1716+
}
1717+
1718+
if (!outerTo || outerTo.toLowerCase() !== batcherContractAddress.toLowerCase()) {
1719+
await throwRecipientMismatch('batch txPrebuild outer to does not match batcher contract address', [
1720+
{ address: outerTo, amount: outerValue },
1721+
]);
1722+
return;
1723+
}
1724+
1725+
const expectedTotal = recipients
1726+
.reduce((sum, r) => sum.plus(new BigNumber(r.amount as string | number)), new BigNumber(0))
1727+
.toFixed();
1728+
if (!new BigNumber(outerValue).isEqualTo(expectedTotal)) {
1729+
await throwRecipientMismatch(
1730+
`batch txPrebuild outer value (${outerValue}) does not match sum of txParams recipients (${expectedTotal})`,
1731+
[{ address: outerTo, amount: outerValue }]
1732+
);
1733+
return;
1734+
}
1735+
1736+
await this.compareBatchCalldataAgainstRecipients(outerCalldata, recipients, throwRecipientMismatch);
1737+
}
1738+
1739+
/**
1740+
* Shared comparator: verify that the given batch calldata starts with the batch selector,
1741+
* decode it, and compare each inner (address, amount) pair to the user-supplied recipients.
1742+
*/
1743+
private async compareBatchCalldataAgainstRecipients(
1744+
batchCalldata: string,
1745+
recipients: ITransactionRecipient[],
1746+
throwRecipientMismatch: (message: string, mismatchedRecipients: Recipient[]) => Promise<never>
1747+
): Promise<void> {
1748+
if (!batchCalldata || !batchCalldata.toLowerCase().startsWith(batchMethodId)) {
1749+
await throwRecipientMismatch('batch txPrebuild inner method selector is not batch(address[],uint256[])', []);
1750+
return;
1751+
}
1752+
1753+
let decoded;
1754+
try {
1755+
decoded = decodeBatchTransferData(batchCalldata);
1756+
} catch (e) {
1757+
await throwRecipientMismatch(
1758+
`failed to decode inner batch calldata: ${e instanceof Error ? e.message : String(e)}`,
1759+
[]
1760+
);
1761+
return;
1762+
}
1763+
1764+
if (decoded.recipients.length !== recipients.length) {
1765+
await throwRecipientMismatch(
1766+
`batch txPrebuild inner recipient count (${decoded.recipients.length}) does not match txParams (${recipients.length})`,
1767+
decoded.recipients
1768+
);
1769+
return;
1770+
}
1771+
1772+
for (let i = 0; i < recipients.length; i++) {
1773+
const expected = recipients[i];
1774+
const actual = decoded.recipients[i];
1775+
// Skip address comparison for non-hex inputs (e.g. unresolved ENS); mirrors normal-tx path.
1776+
if (this.isETHAddress(expected.address) && expected.address.toLowerCase() !== actual.address.toLowerCase()) {
1777+
await throwRecipientMismatch('batch txPrebuild inner recipient address does not match txParams', [actual]);
1778+
return;
1779+
}
1780+
if (!new BigNumber(expected.amount).isEqualTo(actual.amount)) {
1781+
await throwRecipientMismatch('batch txPrebuild inner recipient amount does not match txParams', [actual]);
1782+
return;
1783+
}
1784+
}
1785+
}
1786+
16361787
/**
16371788
* Extract recipients from transaction hex
16381789
* @param txHex - The transaction hex string
@@ -3088,6 +3239,7 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin {
30883239
* @throws {TxIntentMismatchRecipientError} if transaction recipients don't match user intent
30893240
*/
30903241
async verifyTssTransaction(params: VerifyEthTransactionOptions): Promise<boolean> {
3242+
const ethNetwork = this.getNetwork();
30913243
const { txParams, txPrebuild, wallet } = params;
30923244

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

3278+
// TSS batch sends call the batcher contract directly (no sendMultiSig wrapper). Decode the
3279+
// inner batch calldata and compare each (address, amount) pair to user intent. Token batches
3280+
// are not supported through the same pattern, so they keep existing behavior.
3281+
if (!txParams.tokenName && txParams.recipients && txParams.recipients.length > 1) {
3282+
const batcherContractAddress = ethNetwork?.batcherContractAddress;
3283+
if (!batcherContractAddress) {
3284+
await throwRecipientMismatch('batch txPrebuild for tss has no configured batcher contract address', []);
3285+
} else {
3286+
await this.verifyTssBatchInnerRecipients(
3287+
txPrebuild,
3288+
txParams.recipients,
3289+
batcherContractAddress,
3290+
throwRecipientMismatch
3291+
);
3292+
}
3293+
}
3294+
31263295
if (txParams.type && ['transfer'].includes(txParams.type)) {
31273296
if (txParams.recipients && txParams.recipients.length === 1) {
31283297
const recipients = txParams.recipients;
@@ -3325,6 +3494,13 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin {
33253494
{ address: txPrebuild.recipients[0].address, amount: txPrebuild.recipients[0].amount.toString() },
33263495
]);
33273496
}
3497+
3498+
// Decode the inner batch(address[],uint256[]) calldata and verify each (address, amount) pair
3499+
// matches user intent. Without this, a compromised platform could swap inner recipients while
3500+
// preserving the outer total amount and batcher-address checks.
3501+
if (!txParams.tokenName) {
3502+
await this.verifyBatchInnerRecipients(txPrebuild, recipients, throwRecipientMismatch);
3503+
}
33283504
} else {
33293505
// Check recipient address and amount for normal transaction
33303506
if (recipients.length !== 1) {

modules/abstract-eth/src/lib/iface.ts

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -144,6 +144,15 @@ export interface NativeTransferData extends TransferData {
144144
data: string;
145145
}
146146

147+
export interface BatchTransferRecipient {
148+
address: string;
149+
amount: string;
150+
}
151+
152+
export interface BatchTransferData {
153+
recipients: BatchTransferRecipient[];
154+
}
155+
147156
export interface WalletInitializationData {
148157
salt?: string;
149158
owners: string[];

modules/abstract-eth/src/lib/utils.ts

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ import {
3131
} from '@bitgo/sdk-core';
3232

3333
import {
34+
BatchTransferData,
3435
ERC1155TransferData,
3536
ERC721TransferData,
3637
FlushTokensData,
@@ -65,6 +66,8 @@ import {
6566
flushERC1155ForwarderTokensMethodIdV4,
6667
flushERC1155TokensTypes,
6768
flushERC1155TokensTypesv4,
69+
batchMethodId,
70+
batchMethodTypes,
6871
sendMultisigMethodId,
6972
sendMultisigTokenMethodId,
7073
sendMultiSigTokenTypes,
@@ -459,6 +462,34 @@ export function decodeTransferData(data: string, isFirstSigner?: boolean): Trans
459462
}
460463
}
461464

465+
/**
466+
* Decode the inner batch(address[],uint256[]) calldata produced for batcher contract sends.
467+
* The data is the inner payload nested inside a sendMultiSig wrapper, not a full transaction.
468+
*
469+
* @param data Hex string starting with the batch method selector
470+
* @returns Decoded recipients and amounts in the order they appear in the calldata
471+
*/
472+
export function decodeBatchTransferData(data: string): BatchTransferData {
473+
if (!data.toLowerCase().startsWith(batchMethodId)) {
474+
throw new BuildTransactionError(`Invalid batch transfer bytecode: ${data}`);
475+
}
476+
const [addresses, amounts] = getRawDecoded(batchMethodTypes, getBufferedByteCode(batchMethodId, data));
477+
if (!Array.isArray(addresses) || !Array.isArray(amounts)) {
478+
throw new BuildTransactionError(`Invalid batch transfer bytecode: ${data}`);
479+
}
480+
if (addresses.length !== amounts.length) {
481+
throw new BuildTransactionError(
482+
`Mismatched batch address/amount array lengths: ${addresses.length} vs ${amounts.length}`
483+
);
484+
}
485+
return {
486+
recipients: addresses.map((addr, i) => ({
487+
address: addHexPrefix(addr as string),
488+
amount: new BigNumber(bufferToHex(amounts[i] as Buffer)).toFixed(),
489+
})),
490+
};
491+
}
492+
462493
/**
463494
* Decode the given ABI-encoded transfer data for the sendMultisigToken function and return parsed fields
464495
*

modules/abstract-eth/src/lib/walletUtil.ts

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
export const sendMultisigMethodId = '0x39125215';
22
export const sendMultisigTokenMethodId = '0x0dcd7a6c';
3+
// Selector for batch(address[],uint256[]) used by batcher contract sends.
4+
export const batchMethodId = '0xc00c4e9e';
35
export const v1CreateForwarderMethodId = '0xfb90b320';
46
export const v4CreateForwarderMethodId = '0x13b2f75c';
57
export const v1WalletInitializationFirstBytes = '0x60806040';
@@ -38,6 +40,9 @@ export const sendMultiSigTypesFirstSigner = ['string', 'address', 'uint', 'bytes
3840
export const sendMultiSigTokenTypes = ['address', 'uint', 'address', 'uint', 'uint', 'bytes'];
3941
export const sendMultiSigTokenTypesFirstSigner = ['string', 'address', 'uint', 'address', 'uint', 'uint'];
4042

43+
export const batchMethodName = 'batch';
44+
export const batchMethodTypes = ['address[]', 'uint256[]'];
45+
4146
export const ERC721SafeTransferTypes = ['address', 'address', 'uint256', 'bytes'];
4247
export const ERC721TransferFromTypes = ['address', 'address', 'uint256'];
4348

modules/abstract-eth/test/unit/utils.ts

Lines changed: 47 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,20 @@
11
import should from 'should';
2+
import EthereumAbi from 'ethereumjs-abi';
23
import {
34
flushERC721TokensData,
45
flushERC1155TokensData,
56
decodeFlushERC721TokensData,
67
decodeFlushERC1155TokensData,
8+
decodeBatchTransferData,
79
} from '../../src/lib/utils';
810
import { ERC721TransferBuilder } from '../../src/lib/transferBuilders/transferBuilderERC721';
9-
import { ERC721TransferFromMethodId, ERC721SafeTransferTypeMethodId } from '../../src/lib/walletUtil';
11+
import {
12+
ERC721TransferFromMethodId,
13+
ERC721SafeTransferTypeMethodId,
14+
batchMethodId,
15+
batchMethodName,
16+
batchMethodTypes,
17+
} from '../../src/lib/walletUtil';
1018

1119
describe('Abstract ETH Utils', () => {
1220
describe('ERC721 Flush Functions', () => {
@@ -268,4 +276,42 @@ describe('Abstract ETH Utils', () => {
268276
decoded1155.tokenAddress.toLowerCase().should.equal(tokenAddressChecksum.toLowerCase());
269277
});
270278
});
279+
280+
describe('decodeBatchTransferData', () => {
281+
const address1 = '0x1111111111111111111111111111111111111111';
282+
const address2 = '0x2222222222222222222222222222222222222222';
283+
const encodeBatch = (addresses: string[], amounts: string[]): string => {
284+
const selector = EthereumAbi.methodID(batchMethodName, batchMethodTypes);
285+
const args = EthereumAbi.rawEncode(batchMethodTypes, [addresses, amounts]);
286+
return '0x' + Buffer.concat([selector, args]).toString('hex');
287+
};
288+
289+
it('hardcoded batchMethodId matches the runtime-computed selector', () => {
290+
const computed = '0x' + EthereumAbi.methodID(batchMethodName, batchMethodTypes).toString('hex');
291+
computed.should.equal(batchMethodId);
292+
});
293+
294+
it('round-trips encode/decode for multiple recipients', () => {
295+
const data = encodeBatch([address1, address2], ['1000', '2500']);
296+
const decoded = decodeBatchTransferData(data);
297+
298+
decoded.recipients.length.should.equal(2);
299+
decoded.recipients[0].address.toLowerCase().should.equal(address1);
300+
decoded.recipients[0].amount.should.equal('1000');
301+
decoded.recipients[1].address.toLowerCase().should.equal(address2);
302+
decoded.recipients[1].amount.should.equal('2500');
303+
});
304+
305+
it('throws on wrong method selector', () => {
306+
should.throws(() => decodeBatchTransferData('0xdeadbeef00000000'), /Invalid batch transfer bytecode/);
307+
});
308+
309+
it('throws when the encoded address[] and uint256[] arrays have different lengths', () => {
310+
// Encode the batch payload directly with mismatched array lengths.
311+
const payload = EthereumAbi.rawEncode(batchMethodTypes, [[address1, address2], ['1000']]);
312+
const tampered =
313+
'0x' + Buffer.concat([EthereumAbi.methodID(batchMethodName, batchMethodTypes), payload]).toString('hex');
314+
should.throws(() => decodeBatchTransferData(tampered), /Mismatched batch address\/amount array lengths/);
315+
});
316+
});
271317
});

modules/sdk-coin-arbeth/package.json

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,8 @@
5151
"devDependencies": {
5252
"@bitgo/sdk-api": "^1.79.2",
5353
"@bitgo/sdk-test": "^9.1.42",
54+
"@ethereumjs/tx": "^3.3.0",
55+
"bignumber.js": "^9.1.1",
5456
"secp256k1": "5.0.1"
5557
},
5658
"gitHead": "18e460ddf02de2dbf13c2aa243478188fb539f0c",

0 commit comments

Comments
 (0)