Skip to content

Commit be7f0e8

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 1ea10fd commit be7f0e8

12 files changed

Lines changed: 1020 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,
@@ -75,8 +76,11 @@ import secp256k1 from 'secp256k1';
7576
import { AbstractEthLikeCoin } from './abstractEthLikeCoin';
7677
import { EthLikeToken } from './ethLikeToken';
7778
import {
79+
batchMethodId,
7880
calculateForwarderV1Address,
7981
coinFamiliesWithL1Fees,
82+
decodeBatchTransferData,
83+
decodeNativeTransferData,
8084
decodeTransferData,
8185
ERC1155TransferBuilder,
8286
ERC721TransferBuilder,
@@ -87,6 +91,7 @@ import {
8791
getRawDecoded,
8892
getToken,
8993
KeyPair as KeyPairLib,
94+
sendMultisigMethodId,
9095
TransactionBuilder,
9196
TransferBuilder,
9297
} from './lib';
@@ -1649,6 +1654,152 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin {
16491654
};
16501655
}
16511656

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

31843336
// Helper to throw TxIntentMismatchRecipientError with recipient details
@@ -3213,6 +3365,23 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin {
32133365
throw new Error('tx cannot be both a batch and hop transaction');
32143366
}
32153367

3368+
// TSS batch sends call the batcher contract directly (no sendMultiSig wrapper). Decode the
3369+
// inner batch calldata and compare each (address, amount) pair to user intent. Token batches
3370+
// are not supported through the same pattern, so they keep existing behavior.
3371+
if (!txParams.tokenName && txParams.recipients && txParams.recipients.length > 1) {
3372+
const batcherContractAddress = ethNetwork?.batcherContractAddress;
3373+
if (!batcherContractAddress) {
3374+
await throwRecipientMismatch('batch txPrebuild for tss has no configured batcher contract address', []);
3375+
} else {
3376+
await this.verifyTssBatchInnerRecipients(
3377+
txPrebuild,
3378+
txParams.recipients,
3379+
batcherContractAddress,
3380+
throwRecipientMismatch
3381+
);
3382+
}
3383+
}
3384+
32163385
if (txParams.type && ['transfer'].includes(txParams.type)) {
32173386
if (txParams.recipients && txParams.recipients.length === 1) {
32183387
const recipients = txParams.recipients;
@@ -3439,6 +3608,13 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin {
34393608
{ address: txPrebuild.recipients[0].address, amount: txPrebuild.recipients[0].amount.toString() },
34403609
]);
34413610
}
3611+
3612+
// Decode the inner batch(address[],uint256[]) calldata and verify each (address, amount) pair
3613+
// matches user intent. Without this, a compromised platform could swap inner recipients while
3614+
// preserving the outer total amount and batcher-address checks.
3615+
if (!txParams.tokenName) {
3616+
await this.verifyBatchInnerRecipients(txPrebuild, recipients, throwRecipientMismatch);
3617+
}
34423618
} else {
34433619
// Check recipient address and amount for normal transaction
34443620
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
@@ -149,6 +149,15 @@ export interface NativeTransferData extends TransferData {
149149
data: string;
150150
}
151151

152+
export interface BatchTransferRecipient {
153+
address: string;
154+
amount: string;
155+
}
156+
157+
export interface BatchTransferData {
158+
recipients: BatchTransferRecipient[];
159+
}
160+
152161
export interface WalletInitializationData {
153162
salt?: string;
154163
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,
@@ -66,6 +67,8 @@ import {
6667
flushERC1155ForwarderTokensMethodIdV4,
6768
flushERC1155TokensTypes,
6869
flushERC1155TokensTypesv4,
70+
batchMethodId,
71+
batchMethodTypes,
6972
sendMultisigMethodId,
7073
sendMultisigTokenMethodId,
7174
sendMultiSigTokenTypes,
@@ -469,6 +472,34 @@ export function decodeTransferData(data: string, isFirstSigner?: boolean): Trans
469472
}
470473
}
471474

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

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.86.5",
5353
"@bitgo/sdk-test": "^9.1.56",
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)