fix: cancel V2 arbitrary calls in zetaclient signer (backport v37)#4576
Merged
ws4charlie merged 8 commits intorelease/zetaclient/v37from Apr 28, 2026
Merged
fix: cancel V2 arbitrary calls in zetaclient signer (backport v37)#4576ws4charlie merged 8 commits intorelease/zetaclient/v37from
ws4charlie merged 8 commits intorelease/zetaclient/v37from
Conversation
Backport of #4575 to release/zetaclient/v37. V2 outbounds with CallOptions.IsArbitraryCall=true that route through GatewayEVM.execute are no longer signed. The signer cancels them via a TSS-to-TSS self-transfer, and the observer mirrors the cancellation by voting the outbound failed so the CCTX reverts via the standard V2 flow, refunding the principal to the revert address (or original sender). Only OutboundTypeCall (no-asset call from GatewayZEVM.call) and OutboundTypeGasWithdrawAndCall (gas withdraw + call from GatewayZEVM.withdrawAndCall with the gas ZRC20) are cancelled. Other V2 paths are unchanged: ERC20-`withdrawAndCall` constrains the destination to invoke typed `Callable.onCall`; plain V2 withdraws emit `isArbitraryCall=true` per protocol-contracts semantics ("no authenticated sender") but don't reach signGatewayExecute. The observer predicate is also gated on `ProtocolContractVersion_V2` so it stays symmetric with the signer's V2-only dispatch. Tests: - TestSigner_SignOutboundFromCCTXV2_NoAssetArbitraryCallCancels (the live-attack shape) - TestSigner_SignOutboundFromCCTXV2_GasWithdrawAndArbitraryCallCancels - TestSigner_SignOutboundFromCCTXV2_PlainWithdrawNotCancelled (regression guard for GatewayZEVM.withdraw() emitting isArbitraryCall=true) - Test_isArbitraryCallCancellation predicate table covering all coin-type + call combinations plus a V1 cctx negative case - TestIsGatewayExecuteOutbound enumerates every outbound type - E2E test_eth_withdraw_and_arbitrary_call now expects Reverted, asserts TSS self-transfer (value=0), and verifies ZRC20 refund to RevertAddress - E2E test_zevm_to_evm_arbitrary_call now expects Reverted Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Author
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 80b4d3e. Configure here.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Mirrors the same fix applied to the v38 PR (#4575): the exact-equality form `revertBalanceAfter == revertBalanceBefore + amount` ignored the gas-fee leftover refunded by RefundUnusedGasFee, making the assertion fragile. Assert the robust invariant instead — the revert address receives AT LEAST the principal back. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The new V2 arbitrary-call cancellation subtest asserts continueKeysign is false, matching the cancellation path. Rename to match the assertion (was copy-pasted from the pre-existing compliance subtest's misleading 'return true' wording). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Mirrors the v38 fix: GatewayZEVM.withdrawAndCall charges the user (amount + gasFee + protocolFlatFee) up-front in ETH-ZRC20, and the cancellation refund covers principal + a fractional gas-fee leftover — not the full upfront cost. Capturing the baseline before the withdraw tx made 'actualAfter >= before + amount' fail by the protocol-fee + stability-pool delta. Move the baseline to immediately after MustWaitForTxReceipt so the upfront cost is already deducted and the refund is purely additive. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Addresses PR #4576 review comments: - Cursor: EVMVerifyOutboundTransferAmount(hash, 0) is vacuous on a TSS self-transfer because the helper only iterates ERC20 Transfer logs and a payload-less self-transfer emits none. Replace with a direct fetch via TransactionByHash and assert tx.Value()==0, tx.Data() empty, tx.To()==TSS. This actually catches a regression that turned the cancelled outbound back into a non-zero GatewayEVM.execute. - Greptile: cctx.OutboundParams[0] indexed without a bounds check; add require.NotEmpty before dereferencing so a missing-outbound failure reports cleanly instead of panicking. - Greptile: add a deployment note on isArbitraryCallCancellation documenting that the predicate is metadata-only and the hotfix expects a coordinated zetaclient rollout to avoid split-vote inconsistency with old-signer / new-observer combos. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ws4charlie
added a commit
that referenced
this pull request
Apr 27, 2026
Same fixes applied to v37 (#4576) for parity: - Replace EVMVerifyOutboundTransferAmount(hash, 0) (vacuous on a payload-less TSS self-transfer because the helper only iterates ERC20 Transfer logs) with TransactionByHash + asserts tx.Value()==0, tx.Data() empty, tx.To()==TSS. - Guard cctx.OutboundParams[0] indexing with require.NotEmpty so a missing-outbound failure reports cleanly instead of panicking. - Document that isArbitraryCallCancellation is metadata-only and the hotfix expects a coordinated zetaclient rollout (avoids split-vote inconsistency between old-signer / new-observer combos). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ws4charlie
added a commit
that referenced
this pull request
Apr 27, 2026
* fix: disable arbitrary call in signer Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: cancel arbitrary call CCTX in signer Replace the prior MessageContext error gate with a SignCancel redirect in SignOutboundFromCCTXV2. An arbitrary-call CCTX now produces a TSS-to-TSS zero-value self-transfer that consumes the assigned nonce on the destination chain instead of leaving the outbound perpetually unsigned, which would head-of-line block subsequent outbounds. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: vote outbound failed for cancelled arbitrary call CCTX Mirror the existing compliance-restricted cancellation branch in the EVM observer so that V2 arbitrary call CCTXs (which the signer cancels via SignCancel) bypass Gateway event parsing and vote the outbound as failed. This routes the CCTX through the standard V2 revert flow and produces a clean terminal state of Reverted, freeing the TSS pending nonce. Update the four direct-success arbitrary call e2e tests to expect Reverted and the destination dApp to remain uncalled. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test: cover arbitrary call cancellation bypass in observer Add a unit test for isArbitraryCallCancellation covering nil call options, non-arbitrary, and arbitrary cases, plus an integration subtest in Test_IsOutboundProcessed that mirrors the existing compliance-restricted subtest to verify the observer votes failed and bypasses Gateway event parsing for V2 arbitrary call CCTXs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: narrow arbitrary-call cancel to GatewayEVM.execute path only The unconditional IsArbitraryCall guard cancelled every plain V2 withdraw because GatewayZEVM.withdraw() emits Withdrawn with isArbitraryCall=true even when there is no message — the flag means "no authenticated sender" in V2, not "execute arbitrary calldata." This regressed test_zevm_rpc and would break all V2 plain withdraws to EOAs in production. Restrict the cancel to OutboundTypeCall and OutboundTypeGasWithdrawAndCall — the only outbound types that reach signGatewayExecute. Mirror the same narrowing in the observer's isArbitraryCallCancellation so the bypass only fires for CCTXs the signer actually cancels. ERC20Custody.withdrawAndCall and Connector.withdrawAndCall constrain the destination to invoke typed Callable.onCall and remain enabled. Revert the e2e expectation flips for ERC20- and ZETA-withdraw_and_arbitrary_call; keep ETH- and ZEVM-to-EVM-arbitrary_call as Reverted. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test: assert ZRC20 refund on cancelled arbitrary-call CCTX Strengthens TestETHWithdrawAndArbitraryCall: set an explicit RevertAddress in RevertOptions, capture the address's ETH-ZRC20 balance before/after, and assert the principal-minus-protocol-fees is refunded via getTotalRevertedAmount — the same pattern used by TestEtherWithdrawRestricted, which exercises the existing compliance SignCancel path. Also asserts the cancelled outbound is a TSS self-transfer (value=0). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test: gate observer cancel predicate on V2, add no-asset-call signer test I-1: isArbitraryCallCancellation now short-circuits when ProtocolContractVersion != V2, mirroring the signer's V2-only dispatch in SignOutboundFromCCTXV2. V1 CCTXs don't populate CallOptions today, but the explicit gate prevents any future V1 path from accidentally flipping a successful outbound to failed via this predicate. I-2: TestSigner_SignOutboundFromCCTXV2_NoAssetArbitraryCallCancels covers OutboundTypeCall (CoinType_NoAssetCall, amount=0) — the path the live mainnet drain used. Existing tests covered OutboundTypeGasWithdrawAndCall and the plain-withdraw regression but left this dispatch implicit via the predicate table. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test: relax refund assertion to a lower bound getTotalRevertedAmount over-estimates the actual refund by ~2.35M wei on the arbitrary-call path (likely a rounding/fee discrepancy between the helper and zetacore's RefundUnusedGasFee math), causing CI flake. Assert the more robust invariant instead: the revert address receives AT LEAST the principal back. Zetacore guarantees this — the principal is refunded in full via ProcessRevert and gas-fee leftover is added on top via RefundUnusedGasFee, so balance increase >= amount always holds. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test: fix misleading subtest name (return false, not true) The new V2 arbitrary-call cancellation subtest asserts continueKeysign is false, matching the cancellation path. Rename to match the assertion (was copy-pasted from the pre-existing compliance subtest's misleading 'return true' wording). Greptile flagged this on PR #4575. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test: capture revert-recipient balance after the withdraw tx mines The previous baseline was captured before the withdraw, but GatewayZEVM.withdrawAndCall charges the user (amount + gasFee + protocolFlatFee) up-front in ETH-ZRC20. The cancellation refunds the principal and a fractional gas-fee leftover, but not the protocol flat fee nor 100% of gas — so measured against the pre-withdraw balance, the net change can be negative (CI failure: -392050 wei). Move the baseline to immediately after MustWaitForTxReceipt (matching the pattern in test_eth_withdraw_restricted_address.go). The upfront cost is then already deducted, so the refund is purely additive and the 'actualAfter >= before + amount' lower bound holds cleanly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test: replace vacuous self-transfer check, guard OutboundParams index Same fixes applied to v37 (#4576) for parity: - Replace EVMVerifyOutboundTransferAmount(hash, 0) (vacuous on a payload-less TSS self-transfer because the helper only iterates ERC20 Transfer logs) with TransactionByHash + asserts tx.Value()==0, tx.Data() empty, tx.To()==TSS. - Guard cctx.OutboundParams[0] indexing with require.NotEmpty so a missing-outbound failure reports cleanly instead of panicking. - Document that isArbitraryCallCancellation is metadata-only and the hotfix expects a coordinated zetaclient rollout (avoids split-vote inconsistency between old-signer / new-observer combos). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
morde08
approved these changes
Apr 27, 2026
kingpinXD
reviewed
Apr 28, 2026
kingpinXD
reviewed
Apr 28, 2026
Adds Warn-level log lines on the signer cancel path and the observer bypass path so security-critical cancellations are visible. Each line includes cctx index, nonce/tx, destination, and outbound type. Addresses kingpinXD's review on PR #4576. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The ERC20Custody.withdrawAndCall and ZetaConnector.withdrawAndCall paths
forward MessageContext straight through to GatewayEVM.executeWithERC20,
which has the same sender==0 arbitrary-call branch (_executeArbitraryCall)
as GatewayEVM.execute. Until the contracts patch ships, a CCTX with
IsArbitraryCall=true and CoinType=ERC20/Zeta would be signed normally,
TSS would broadcast ERC20Custody.withdrawAndCall({sender:0x0}, ...) which
calls gateway.executeWithERC20({sender:0x0}, ...), and the gateway would
route to _executeArbitraryCall — same drain primitive as the original
attack via GatewayZEVM.call.
Extend the cancel guard to cover OutboundTypeERC20WithdrawAndCall and
OutboundTypeZetaWithdrawAndCall in addition to the GatewayEVM.execute
paths. Rename IsGatewayExecuteOutbound → IsArbitraryCallCancellable to
reflect the broader scope (the helper now describes which outbound types
are cancelled, not which contract function they hit).
Updated tests:
- TestIsArbitraryCallCancellable enumerates all four cancellable types.
- Test_isArbitraryCallCancellation table covers the two new positive
cases (ERC20 and Zeta withdrawAndCall).
- E2E test_erc20_withdraw_and_arbitrary_call now expects Reverted with
ZRC20 refund to RevertAddress, matching the eth and zevm-to-evm
arbitrary-call e2e patterns.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ws4charlie
changed the title
s2imonovic
approved these changes
Apr 28, 2026
Avoid evaluating the predicate twice in the same block. The function parses ProtocolContractVersion, dereferences outbound params, and dispatches into ParseOutboundTypeFromCCTX, so it does real work each call. Mirrors the same fix applied on v38. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Backport of both #4575 and #4580 to
release/zetaclient/v37.V2 CCTXs with
CallOptions.IsArbitraryCall=truewhose outbound type is one of:OutboundTypeCall/OutboundTypeGasWithdrawAndCall→GatewayEVM.executeOutboundTypeERC20WithdrawAndCall→ERC20Custody.withdrawAndCall→GatewayEVM.executeWithERC20OutboundTypeZetaWithdrawAndCall→ZetaConnector.withdrawAndCall→GatewayEVM.executeWithERC20…are short-circuited to a TSS self-transfer (
SignCancel) in the signer, and the observer treats those receipts as failed outbounds to trigger the standard V2 revert flow rather than parsing gateway events. All four outbound types reach the samesender == address(0)branch (_executeArbitraryCall) on the destination gateway, so they are cancelled as one group.Changes
common/cctx.goIsArbitraryCallCancellablehelper enumerating the four cancellable typessigner/v2_signer.goSignCancelfor arbitrary calls on cancellable outbound types; warn log on cancellationobserver/outbound.goisArbitraryCallCancellationpredicate mirroring the signer; warn log on event-parsing bypasscctx_test.go,outbound_test.go,v2_signer_test.goe2etests/test_eth_withdraw_and_arbitrary_call.goRevertedwith principal refunde2etests/test_erc20_withdraw_and_arbitrary_call.goRevertedwith ZRC20 refund toRevertAddresse2etests/test_zevm_to_evm_arbitrary_call.goReverted, asserts dApp not invokedTest plan
go build ./zetaclient/... ./e2e/...go test ./zetaclient/chains/evm/{common,observer,signer}/...go vet ./zetaclient/chains/evm/... ./e2e/e2etests/...🤖 Generated with Claude Code