fix: skip forged non-protocol logs in v1 outbound parsers#4574
fix: skip forged non-protocol logs in v1 outbound parsers#4574bit2swaz wants to merge 3 commits into
Conversation
📝 WalkthroughWalkthroughThe PR adds contract address filtering to outbound event parsers in both v1 and v2 paths. Candidate logs are now validated against expected contract addresses before field extraction, allowing parsers to skip forged logs with matching signatures and continue parsing subsequent logs in the same receipt. ChangesOutbound Parser Address Filtering
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
Fixes legacy (v1) outbound receipt parsing so forged same-signature logs from non-protocol contracts don’t cause early aborts, allowing the parser to continue scanning and pick up the later valid protocol log in the same receipt. Also applies the same “skip non-protocol emitter before validation” pattern to v2 outbound parsers and adds regression coverage.
Changes:
- Skip logs whose
Addressdoesn’t match the expected protocol contract before callingValidateEvmTxLogin v1 and v2 outbound parsers. - Update v1 outbound tests to reflect new “no event found” behavior on wrong expected contract address and add forged-first regression cases.
- Add a v2 outbound regression test covering forged-first ordering across the relevant v2 parsers.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
zetaclient/chains/evm/observer/outbound.go |
Updates v1 parseAndCheckZetaEvent and parseAndCheckWithdrawnEvent to skip parsed events from unexpected emitter addresses. |
zetaclient/chains/evm/observer/v2_outbound.go |
Applies the same emitter-address skip behavior to v2 outbound parse-and-check functions. |
zetaclient/chains/evm/observer/outbound_test.go |
Adjusts mismatch expectations and adds forged-first regression tests plus helper utilities for forging logs. |
zetaclient/chains/evm/observer/v2_outbound_test.go |
Adds regression test ensuring v2 outbound parsers skip forged logs and successfully parse the later valid protocol log. |
💡 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.
The new emitter-address guard still ABI-parses the log before checking vLog.Address. Since receipts are untrusted input, consider checking vLog.Address != <expected> at the start of the loop (before Parse*) to skip non-protocol logs without doing any ABI unpacking. This reduces unnecessary work and limits potential CPU amplification if a receipt contains many same-signature forged logs. Apply the same ordering to the other parseAndCheck* loops in this file that follow this pattern.
| for _, vLog := range receipt.Logs { | ||
| // try parsing ZetaReceived event | ||
| received, err := connector.ZetaConnectorNonEthFilterer.ParseZetaReceived(*vLog) | ||
| if err == nil { | ||
| if vLog.Address != connectorAddr { | ||
| continue | ||
| } | ||
| err = common.ValidateEvmTxLog(vLog, connectorAddr, receipt.TxHash.Hex(), common.TopicsZetaReceived) | ||
| if err != nil { |
There was a problem hiding this comment.
The emitter-address check is currently performed only after a successful ABI parse (ParseZetaReceived / ParseZetaReverted / ParseWithdrawn). Consider moving the vLog.Address != <expected> guard to the top of the receipt-log loop (before attempting any Parse* call) so forged logs from other contracts are skipped without doing ABI decoding. This keeps the new skip-and-continue semantics while avoiding unnecessary work on attacker-controlled receipts.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
zetaclient/chains/evm/observer/v2_outbound_test.go (1)
145-145: 💤 Low valueVerify the hardcoded issue reference.
Line 145 references
"issue-4567"in the CCTX sample, but the actual PR number is 4574 and the linked issue is 4573. Confirm whether this identifier should match the actual issue numbers or if it's an intentional test-data identifier unrelated to the PR.🔍 Suggested alignment
If this should reference the actual issue:
- cctx := sample.CrossChainTxV2(t, "issue-4567") + cctx := sample.CrossChainTxV2(t, "issue-4573")Otherwise, consider adding a comment clarifying that this is synthetic test data.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@zetaclient/chains/evm/observer/v2_outbound_test.go` at line 145, The test uses sample.CrossChainTxV2(t, "issue-4567") which hardcodes an issue id that appears inconsistent with PR `#4574` and linked issue `#4573`; update the identifier in the sample call to the correct value (e.g., "issue-4573" or "pr-4574") if it should reflect real issue/PR numbers, or leave it as-is but add a clarifying comment next to the sample.CrossChainTxV2(...) call explaining that "issue-4567" is synthetic test data and not intended to match real issues—make the change near the cctx variable to keep intent clear.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@zetaclient/chains/evm/observer/v2_outbound_test.go`:
- Line 145: The test uses sample.CrossChainTxV2(t, "issue-4567") which hardcodes
an issue id that appears inconsistent with PR `#4574` and linked issue `#4573`;
update the identifier in the sample call to the correct value (e.g.,
"issue-4573" or "pr-4574") if it should reflect real issue/PR numbers, or leave
it as-is but add a clarifying comment next to the sample.CrossChainTxV2(...)
call explaining that "issue-4567" is synthetic test data and not intended to
match real issues—make the change near the cctx variable to keep intent clear.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a84f09ca-6965-42b3-a3cc-cc8390b06d31
📒 Files selected for processing (4)
zetaclient/chains/evm/observer/outbound.gozetaclient/chains/evm/observer/outbound_test.gozetaclient/chains/evm/observer/v2_outbound.gozetaclient/chains/evm/observer/v2_outbound_test.go
Follow-up to #4567
Closes #4573
Root cause:
The legacy v1 outbound parsers iterate receipt logs, attempt ABI parsing, and then validate the parsed log with
ValidateEvmTxLog. A forged log from a non-protocol contract can still parse successfully if it reuses the same event signature and ABI shape. The old code then returned immediately on emitter-address mismatch, which prevented the parser from reaching the later valid protocol log in the same receipt.Fix:
For the affected v1 outbound parsers, skip logs whose emitter address does not match the expected protocol contract before calling
ValidateEvmTxLog. KeepValidateEvmTxLogfor candidate protocol logs so removed-log, tx-hash, and topic-count validation behavior remains unchanged.Affected paths:
parseAndCheckZetaEventparseAndCheckWithdrawnEventTests:
ZetaReceivedZetaRevertedWithdrawnzetaclient/chains/evm/observerpackage passes under Go 1.23.9.Notes:
Note
Medium Risk
Touches outbound receipt parsing used to determine cross-chain receive status/value; while the change is small, it affects core confirmation/voting behavior and could cause missed events if address checks are wrong.
Overview
Hardens outbound receipt parsing to ignore forged/non-protocol logs that reuse protocol event signatures. The v1 parsers (
parseAndCheckZetaEvent,parseAndCheckWithdrawnEvent) and v2 outbound parsers now skip logs whoseAddressis not the expected contract before runningValidateEvmTxLog, allowing the parser to continue scanning and find the later legitimate protocol log.Updates and expands tests to match the new semantics (wrong contract address now yields “no event found”), and adds regression coverage that prepends a forged log to real receipts/events to ensure parsing still succeeds.
Reviewed by Cursor Bugbot for commit bd2e6d3. Configure here.
Greptile Summary
This PR fixes a forged-log bypass in the EVM outbound parsers (v1 and v2) where a non-protocol contract emitting an event with the same ABI signature could cause an early validation failure that prevented the parser from reaching the real protocol log later in the receipt. The fix consistently adds an emitter-address check (
vLog.Address != protocolAddr → continue) immediately after a successful ABI parse and beforeValidateEvmTxLog, converting the old abort-on-mismatch into a skip-and-continue across all nine affected parsers.Confidence Score: 5/5
Safe to merge; fix is minimal, correctly applied to all affected parsers, and well-covered by new regression tests.
The patch is a narrow, consistent change (one 3-line guard per parser) with no logic regressions. All nine parsers (v1 and v2) receive the same treatment, existing tests are updated to reflect the new semantics, and targeted forged-first tests are added for all affected paths. No P0/P1 findings were identified.
No files require special attention.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Start: iterate receipt.Logs] --> B[Try ABI parse] B -- parse error --> A B -- parse OK --> C{vLog.Address == protocolAddr?} C -- No --> A C -- Yes --> D[ValidateEvmTxLog\ntx-hash, topic-count, address] D -- validation error --> E[Return error] D -- OK --> F[Check receiver / amount / index] F -- mismatch --> G[Return error] F -- match --> H[Return event ✓] A -- no logs left --> I[Return 'no event found' error]Reviews (1): Last reviewed commit: "fix: skip forged non-protocol v1 outboun..." | Re-trigger Greptile
Summary by CodeRabbit
Bug Fixes
Tests