fix(observer): skip forged outbound logs during v2 outbound receipt parsing#4572
fix(observer): skip forged outbound logs during v2 outbound receipt parsing#4572bit2swaz wants to merge 1 commit intozeta-chain:mainfrom
Conversation
📝 WalkthroughWalkthroughSecurity fix that adds address filtering to six v2 outbound event parsing functions to prevent forged contract logs from aborting the parser early. Includes comprehensive test suite validating that parsers correctly ignore logs from non-Gateway contract addresses. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
Pull request overview
This PR fixes a v2 outbound receipt-parsing bug where forged same-signature logs from non-protocol contracts could cause the observer to abort early, preventing it from finding the real protocol log later in the receipt.
Changes:
- Updated six v2 outbound
parseAndCheck*helpers to skip logs whose emitter address does not match the expected protocol contract before validating the log. - Added a table-driven regression test that ensures forged logs are skipped and valid protocol logs are still parsed successfully.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
zetaclient/chains/evm/observer/v2_outbound.go |
Skips wrong-emitter logs before ValidateEvmTxLog so parsing continues to later (valid) protocol logs. |
zetaclient/chains/evm/observer/v2_outbound_test.go |
Adds regression coverage for forged same-signature logs preceding valid protocol logs across all six affected parsers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for _, vLog := range receipt.Logs { | ||
| executed, err := gateway.GatewayEVMFilterer.ParseExecuted(*vLog) | ||
| if err != nil { | ||
| continue | ||
| } | ||
| if vLog.Address != gatewayAddr { | ||
| continue |
There was a problem hiding this comment.
Address filtering happens after ABI parsing (ParseExecuted). Since vLog.Address is available up front, consider checking the emitter address before attempting ABI parsing to avoid unnecessary unpacking work (and potential large allocations) for forged logs that intentionally reuse the same event signature.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
zetaclient/chains/evm/observer/v2_outbound.go (2)
150-150: Pre-existing typo — safe to fix opportunistically.The error message reads
"failed to validate gateway reverte event"(missing trailingd). Not introduced by this PR, but trivially addressable while this file is open.✏️ Proposed fix
- "failed to validate gateway reverte event", + "failed to validate gateway reverted event",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@zetaclient/chains/evm/observer/v2_outbound.go` at line 150, Typo in the error message string "failed to validate gateway reverte event" should be corrected to "failed to validate gateway reverted event"; locate the string literal in zetaclient/chains/evm/observer/v2_outbound.go (the error return/log call that uses that message) and update the message text accordingly so the logged/returned error reads "failed to validate gateway reverted event".
82-94: Optional: the residualvLog.Addresscheck insideValidateEvmTxLogis now unreachable.With the new
if vLog.Address != gatewayAddr { continue }guard immediately preceding everyValidateEvmTxLog(vLog, <addr>, …)invocation, the address branch incommon.ValidateEvmTxLog(seezetaclient/chains/evm/common/validation.goline 20) can never fire for these six parsers. The call is still valuable for theRemoved,TxHash, and topic-count assertions, so removing it is not warranted — but if you would like to eliminate the double-check without losing coverage, an equivalent in-line guard over the remaining fields reads cleanly:♻️ Illustrative simplification (applies to all six parsers)
- if vLog.Address != gatewayAddr { - continue - } - // basic event check - if err := common.ValidateEvmTxLog( - vLog, - gatewayAddr, - receipt.TxHash.Hex(), - common.TopicsGatewayExecuted, - ); err != nil { - return big.NewInt( - 0, - ), chains.ReceiveStatus_failed, errors.Wrap( - err, - "failed to validate gateway executed event", - ) - } + // skip forged logs from non-protocol emitters; validate the remaining invariants + if vLog.Address != gatewayAddr { + continue + } + if vLog.Removed { + return big.NewInt(0), chains.ReceiveStatus_failed, + errors.New("log is removed, it might be related to a chain reorganization") + }Happy to leave this as-is given the PR's deliberately minimal scope; flagging only for future cleanup.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@zetaclient/chains/evm/observer/v2_outbound.go` around lines 82 - 94, The address equality branch in common.ValidateEvmTxLog is now redundant because each parser in v2_outbound.go already does if vLog.Address != gatewayAddr { continue }, so remove the address-specific check from ValidateEvmTxLog (validation.go) and keep the other assertions (Removed, TxHash, topics count) intact; alternatively, if you prefer to keep ValidateEvmTxLog untouched, replace calls like ValidateEvmTxLog(vLog, gatewayAddr, ...) in v2_outbound.go with an inline call that only asserts Removed, TxHash, and topic-count so you avoid the double address-check — refer to ValidateEvmTxLog and the parsers in v2_outbound.go when making the change.zetaclient/chains/evm/observer/v2_outbound_test.go (1)
292-317: Minor:ethclient.Client{}placeholder is unnecessary for parse-only operations.The test helpers create contract instances solely to access their
*Filterermethods, which perform pure log parsing using only the providedethtypes.Logand do not dereference the backend. Anilbackend passed through the typedbind.ContractBackendinterface would work equally well and more explicitly signal that no backend interaction is needed. Consider this substitution if you add defensive nil-checks to the generated bindings in the future.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@zetaclient/chains/evm/observer/v2_outbound_test.go` around lines 292 - 317, The helpers mustNewGatewayEVM, mustNewERC20Custody, and mustNewZetaConnectorNative currently pass a dummy ðclient.Client{} as the backend; change these to pass a nil backend (e.g. bind.ContractBackend(nil)) since the tests only use the generated *Filterer methods for pure log parsing and do not require a real client—update the call sites in those three functions to pass a nil backend and keep the require.NoError checks as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@zetaclient/chains/evm/observer/v2_outbound_test.go`:
- Around line 292-317: The helpers mustNewGatewayEVM, mustNewERC20Custody, and
mustNewZetaConnectorNative currently pass a dummy ðclient.Client{} as the
backend; change these to pass a nil backend (e.g. bind.ContractBackend(nil))
since the tests only use the generated *Filterer methods for pure log parsing
and do not require a real client—update the call sites in those three functions
to pass a nil backend and keep the require.NoError checks as-is.
In `@zetaclient/chains/evm/observer/v2_outbound.go`:
- Line 150: Typo in the error message string "failed to validate gateway reverte
event" should be corrected to "failed to validate gateway reverted event";
locate the string literal in zetaclient/chains/evm/observer/v2_outbound.go (the
error return/log call that uses that message) and update the message text
accordingly so the logged/returned error reads "failed to validate gateway
reverted event".
- Around line 82-94: The address equality branch in common.ValidateEvmTxLog is
now redundant because each parser in v2_outbound.go already does if vLog.Address
!= gatewayAddr { continue }, so remove the address-specific check from
ValidateEvmTxLog (validation.go) and keep the other assertions (Removed, TxHash,
topics count) intact; alternatively, if you prefer to keep ValidateEvmTxLog
untouched, replace calls like ValidateEvmTxLog(vLog, gatewayAddr, ...) in
v2_outbound.go with an inline call that only asserts Removed, TxHash, and
topic-count so you avoid the double address-check — refer to ValidateEvmTxLog
and the parsers in v2_outbound.go when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9018adf4-57fe-42a6-97d6-f0a2abb9ca18
📒 Files selected for processing (2)
zetaclient/chains/evm/observer/v2_outbound.gozetaclient/chains/evm/observer/v2_outbound_test.go
closes #4567
Root cause:
The v2 outbound parseAndCheck helpers iterate receipt logs and first try to ABI-parse each log. A forged log from a non-protocol contract can still parse successfully if it reuses the same event signature and ABI layout. The old code then called ValidateEvmTxLog and returned immediately on the emitter-address mismatch, which aborted parsing before the real protocol log later in the receipt could be examined.
Fix:
For the six affected v2 outbound parsers, skip logs whose emitter address does not match the expected protocol contract before calling ValidateEvmTxLog. Keep ValidateEvmTxLog in place for candidate protocol logs so removed-log, tx-hash, and topic-count validation behavior remains unchanged.
Affected functions:
Tests:
Notes:
Note
Medium Risk
Moderate risk because it changes how outbound receipts are interpreted; incorrect filtering could cause valid protocol events to be missed or misclassified, but the change is narrow and covered by a regression test.
Overview
Fixes v2 outbound receipt parsing to skip ABI-decodable but non-protocol (forged) logs by checking
vLog.Addressmatches the expected contract before callingValidateEvmTxLog, preventing early failure when a valid protocol log appears later in the receipt.Adds a table-driven regression test (
v2_outbound_test.go) that builds receipts containing a forged same-signature log followed by a valid one, and asserts all six affected parsers returnReceiveStatus_successwith the expected amount.Reviewed by Cursor Bugbot for commit e608191. Configure here.
Greptile Summary
This PR fixes a log-parsing bug in all six v2 outbound receipt parsers where a forged EVM log (same event signature, wrong emitter address) would cause
ValidateEvmTxLogto return an address-mismatch error that aborted the loop early, preventing the real protocol log from being processed. The fix adds avLog.Address != <protocolAddr>guard immediately after ABI parsing succeeds so forged logs are silently skipped, andValidateEvmTxLog(which still validates tx-hash and topic-count) is only reached for logs from the expected contract. A comprehensive table-driven regression test covers all six parsers.Confidence Score: 5/5
Safe to merge — the fix is minimal, correct, and well-tested; no behavioral regressions for the happy path.
All six parsers receive consistent, correctly-placed address guards. The address check comes after ABI-parsing succeeds and before ValidateEvmTxLog, preserving tx-hash and topic-count validation for legitimate protocol logs. The regression test directly reproduces the reported failure condition.
No files require special attention; the analogous v1 parsers are intentionally out of scope and should be addressed in a follow-up.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Start: iterate receipt.Logs] --> B[ABI Parse log\ne.g. ParseExecuted] B -->|parse error| C[continue to next log] B -->|parse ok| D{NEW: vLog.Address\n== protocolAddr?} D -->|no - forged emitter| C D -->|yes| E[ValidateEvmTxLog\ncheck txHash + topic count] E -->|fail| F[return ReceiveStatus_failed] E -->|pass| G[Validate fields\nreceiver / amount / data] G -->|mismatch| F G -->|match| H[return ReceiveStatus_success] C --> A A -->|no more logs| I[return event not found error]Reviews (1): Last reviewed commit: "fix: skip forged non-protocol v2 outboun..." | Re-trigger Greptile
Summary by CodeRabbit