Skip to content

fix(abstract-eth): verify inner batch calldata recipients [CGD-1319]#8772

Open
kamleshmugdiya wants to merge 1 commit into
masterfrom
kamleshmugdiya/CGD-1319-batch-verifier-inner-recipients
Open

fix(abstract-eth): verify inner batch calldata recipients [CGD-1319]#8772
kamleshmugdiya wants to merge 1 commit into
masterfrom
kamleshmugdiya/CGD-1319-batch-verifier-inner-recipients

Conversation

@kamleshmugdiya
Copy link
Copy Markdown
Contributor

Replaces #8771 — that PR auto-closed when its head branch was renamed; same commit 843b46332d.

Description

verifyTransaction for ETH batcher-contract sends only checked (a) the sum of txParams.recipients against txPrebuild.recipients[0].amount and (b) that the outer recipient was the network's batcherContractAddress. It never decoded the inner batch(address[],uint256[]) calldata embedded in txPrebuild.txHex, and signTransaction then loaded that txHex verbatim.

A compromised BitGo platform could preserve both outer checks while swapping the inner (address, amount) pairs to attacker-controlled addresses, redirecting batched payouts. This PR decodes the inner calldata during verification and compares each pair to user intent.

Issue Number

CGD-1319

Type of change

  • Bug fix (non-breaking change which fixes an issue)

What changed

  • modules/abstract-eth/src/lib/walletUtil.ts — exports batchMethodId (0xc00c4e9e), batchMethodName, batchMethodTypes.
  • modules/abstract-eth/src/lib/iface.ts — adds BatchTransferData / BatchTransferRecipient.
  • modules/abstract-eth/src/lib/utils.ts — adds decodeBatchTransferData(), modeled after the existing decode*TransferData helpers and reusing getRawDecoded / getBufferedByteCode.
  • modules/abstract-eth/src/abstractEthLikeNewCoins.ts — adds private verifyBatchInnerRecipients which parses txPrebuild.txHex, decodes the outer sendMultiSig, decodes the inner batch(address[],uint256[]), and compares each (address, amount) pair to txParams.recipients. Wired into the existing batch path in verifyTransaction for native batches.

The verifier fails closed when:

  • txPrebuild.txHex is missing
  • the outer selector is not sendMultiSig
  • the inner selector is not batch(address[],uint256[])
  • inner recipient count, address, or amount differ from txParams.recipients

Token-batch (txParams.tokenName) and TSS paths are out of scope per the issue; the codebase has no sendMultisigBatch-style token wrapper, and the existing token branch already expects txPrebuild.recipients[0].amount === 0. signTransaction is unchanged — it now safely loads txHex because the verifier proves inner calldata matches user intent first.

ENS / non-hex txParams addresses skip the address comparison but still enforce the amount check, mirroring the normal-tx path at abstractEthLikeNewCoins.ts:3340.

How Has This Been Tested?

  • modules/abstract-eth/test/unit/utils.ts — 4 tests for decodeBatchTransferData: hardcoded selector matches runtime-computed value, round-trip encode/decode, wrong-selector rejection, mismatched-array-length rejection.
  • modules/sdk-coin-eth/test/unit/eth.ts — adds a buildBatchTxHex helper and 5 new verifyTransaction tests: missing txHex, tampered inner address, tampered inner amount (with outer total preserved), wrong inner selector, mismatched inner recipient count. Updates the existing happy-path batch test to include a valid txHex.

Local runs:

  • yarn unit-test in modules/abstract-eth77 passing
  • yarn unit-test in modules/sdk-coin-eth237 passing
  • yarn lint in both modules — clean

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • My code compiles correctly for both Node and Browser environments
  • I have commented my code, particularly in hard-to-understand areas
  • My commits follow Conventional Commits and I have properly described any BREAKING CHANGES
  • The ticket or github issue was included in the commit message as a reference
  • I have made corresponding changes to the documentation and on any new/updated functions and/or methods - jsdoc
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@kamleshmugdiya kamleshmugdiya requested a review from a team as a code owner May 14, 2026 07:43
@linear-code
Copy link
Copy Markdown

linear-code Bot commented May 14, 2026

CGD-1319

@kamleshmugdiya kamleshmugdiya self-assigned this May 14, 2026
@kamleshmugdiya
Copy link
Copy Markdown
Contributor Author

@claude

// 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.

.should.be.rejectedWith('recipient address of txPrebuild does not match batcher address');
});

it('should reject a batch txPrebuild that omits txHex', async function () {
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.

also add tests for other evm coins since its common 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.

Added happy-path + one negative case in sdk-coin-arbeth, sdk-coin-opeth, and sdk-coin-polygon to confirm the abstract verifier picks up each coin's batcher contract address correctly. Each module now has a should verify a batch txPrebuild whose inner calldata matches txParams.recipients test plus a targeted rejection (mismatched address / amount / count) so coverage isn't only on Ethereum.

The verifier itself lives in abstract-eth and is shared by all EVM coins, so the comprehensive test matrix stays in sdk-coin-eth; the per-coin tests are smoke tests for the network-config wiring.

kamleshmugdiya added a commit that referenced this pull request May 14, 2026
Add the same batch(address[],uint256[]) inner-recipient decoding to
verifyTssTransaction, which previously had no batch verification at
all. TSS wallets are EOAs and call the batcher contract directly, so
the outer transaction has tx.to = batcherContractAddress and tx.data
= batch calldata (no sendMultiSig wrapper). The new verifier checks
the outer to and value, then decodes and compares each inner pair.

Extracts the comparator into compareBatchCalldataAgainstRecipients so
the multi-sig and TSS paths share the inner decode/compare logic.

Also adds happy-path verifyTransaction batch tests in sdk-coin-arbeth,
sdk-coin-opeth, sdk-coin-polygon — the verifier lives in abstract-eth
so it applies to all EVM coins, but each per-coin test confirms the
batcher contract address is picked up correctly from network config.

Addresses review comments on PR #8772:
- mullapudipruthvik: shouldn't we do the same for verifyTssTransaction?
- mullapudipruthvik: also add tests for other evm coins since its common method

Ticket: CGD-1319
@kamleshmugdiya kamleshmugdiya force-pushed the kamleshmugdiya/CGD-1319-batch-verifier-inner-recipients branch 2 times, most recently from 8312a05 to 747c372 Compare May 14, 2026 09:35
@kamleshmugdiya kamleshmugdiya requested a review from a team as a code owner May 14, 2026 09:35
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
@kamleshmugdiya kamleshmugdiya force-pushed the kamleshmugdiya/CGD-1319-batch-verifier-inner-recipients branch from 747c372 to fccd267 Compare May 14, 2026 09:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants