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 2 commits intozeta-chain:mainfrom
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 35 minutes and 13 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✨ 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.
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