fix(zetaclient/v38): also cancel arbitrary calls reaching executeWithERC20#4580
Conversation
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. Add warn logs on both signer-side cancellation
and observer-side bypass so cancellations are visible in production logs.
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.
- E2E test_zeta_withdraw_and_arbitrary_call's V2-enabled branch flipped
to Reverted for forward compatibility (V2 ZETA flows are currently
disabled at the contract level on mainnet).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
bugbot run |
|
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 |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 38856c9. Configure here.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
- outbound.go: extract isArbitraryCallCancellation(cctx) to a local variable to avoid double evaluation (the predicate parses ProtocolContractVersion, dereferences outbound params, and dispatches into ParseOutboundTypeFromCCTX). - test_zeta_withdraw_and_arbitrary_call.go: add a revert-refund balance check on the V2-enabled branch, mirroring the ERC20 test. Uses TestDAppV2ZEVMAddr as the revert recipient so the assertion isn't perturbed by gas (revert tx is signed by TSS). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
Extends the V2 arbitrary-call cancel guard (merged in #4575) to cover the ERC20 and ZETA
withdrawAndCallpaths, which forward toGatewayEVM.executeWithERC20and reach the samesender==0arbitrary-call branch asGatewayEVM.execute. Also adds warn logs around the cancellation so it's visible in production.Why this matters
Before this change, only
GatewayEVM.executearbitrary calls were cancelled.ERC20Custody.withdrawAndCallandZetaConnector.withdrawAndCallwere still signed normally — but those paths forwardMessageContextstraight intoGatewayEVM.executeWithERC20, which has the same_executeArbitraryCallbranch thatexecutedoes. Without this extension, an attacker who triggers anIsArbitraryCall=trueCCTX withCoinType=ERC20(orZeta) could still reach the arbitrary-call branch on the destination gateway.Changes
common/cctx.goIsGatewayExecuteOutbound→IsArbitraryCallCancellable; expand to coverOutboundTypeERC20WithdrawAndCallandOutboundTypeZetaWithdrawAndCallsigner/v2_signer.goobserver/outbound.gocctx_test.goTestIsArbitraryCallCancellableenumerates all four cancellable typesoutbound_test.gowithdrawAndCallcases totruee2etests/test_erc20_withdraw_and_arbitrary_call.goRevertedwith ZRC20 refund toRevertAddresse2etests/test_zeta_withdraw_and_arbitrary_call.goReverted(forward compat — V2 ZETA flows are disabled at the contract level on mainnet today)Test plan
go build ./zetaclient/... ./e2e/...go test ./zetaclient/chains/evm/{common,observer,signer}/...go vet ./zetaclient/chains/evm/... ./e2e/e2etests/...🤖 Generated with Claude Code
Note
High Risk
Changes signer and observer behavior for V2 cross-chain outbounds by cancelling additional arbitrary-call routes, which is security- and funds-flow critical and could affect outbound processing if misclassified.
Overview
Extends the V2 arbitrary-call cancellation guard so CCTXs with
CallOptions.IsArbitraryCall=trueare also signer-cancelled forERC20andZETAwithdrawAndCalloutbounds that reachGatewayEVM.executeWithERC20(in addition to the existingGatewayEVM.executepaths).Renames/expands the outbound-type predicate to
IsArbitraryCallCancellable, wires it into both the V2 signer and observer (including new warn logs when voting failed due to signer-cancel), and updates unit/e2e tests to expectRevertedCCTXs with refund to the configuredRevertAddressand no destination dApp invocation.Reviewed by Cursor Bugbot for commit 38856c9. Configure here.
Greptile Summary
Extends the V2 arbitrary-call cancel guard (from #4575) to cover
ERC20Custody.withdrawAndCallandZetaConnector.withdrawAndCall, which both forward intoGatewayEVM.executeWithERC20's_executeArbitraryCallbranch withMessageContext.Sender == address(0)— the same drain shape asGatewayEVM.execute. The changes renameIsGatewayExecuteOutbound→IsArbitraryCallCancellable, wire the updated predicate into both the V2 signer and observer, add warn logs on cancellation, and update unit + e2e tests to expectRevertedCCTXs with refund to the configuredRevertAddress.Confidence Score: 4/5
Safe to merge; all findings are P2 style/test-coverage gaps with no correctness impact on the core cancellation logic.
The predicate expansion, signer guard, observer handler, and unit tests are all consistent and correct. Both P2 findings (double function call in observer, missing refund assertion in Zeta e2e test) are quality issues rather than logic defects. Deployment note about coordinated rollout is important and already documented.
e2e/e2etests/test_zeta_withdraw_and_arbitrary_call.go — missing principal-refund balance assertion parallel to the ERC20 test.
Important Files Changed
IsGatewayExecuteOutbound→IsArbitraryCallCancellableand expands it to includeOutboundTypeERC20WithdrawAndCallandOutboundTypeZetaWithdrawAndCall; logic and docs are correct and exhaustive.IsArbitraryCallCancellablepredicate and adds a warn log; the early-return guard before the switch is correctly positioned so ERC20/Zeta non-arbitrary calls still fall through to normal signing paths.IsArbitraryCallCancellable; adds warn log but callsisArbitraryCallCancellationtwice in the same block — minor style issue, no correctness impact.Revertedand dApp not called, but omits the revert-address balance check present in the ERC20 counterpart.falsetotrue; table is consistent with the new predicate.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[CCTX PendingOutbound\nIsArbitraryCall=true] --> B{ParseOutboundTypeFromCCTX} B --> C[OutboundTypeCall\nOutboundTypeGasWithdrawAndCall] B --> D[OutboundTypeERC20WithdrawAndCall\n🆕 now cancellable] B --> E[OutboundTypeZetaWithdrawAndCall\n🆕 now cancellable] B --> F[Plain withdraw types\nIsArbitraryCall=true on wire\nbut NOT cancellable] C --> G{IsArbitraryCallCancellable?} D --> G E --> G F --> H[Normal signing path\nEvent parsing in observer] G -- true --> I[Signer: SignCancel\nTSS self-transfer\nWarn log emitted] I --> J[Observer: vote failed\nWarn log emitted] J --> K[ZetaCore: CCTX → Reverted\nPrincipal refunded to RevertAddress] G -- false --> HPrompt To Fix All With AI
Reviews (1): Last reviewed commit: "fix: also cancel arbitrary calls reachin..." | Re-trigger Greptile