test(l2): pin defensive misuse of topics.contains in get_block_l1_messages#6539
Draft
avilagaston9 wants to merge 1 commit intomainfrom
Draft
test(l2): pin defensive misuse of topics.contains in get_block_l1_messages#6539avilagaston9 wants to merge 1 commit intomainfrom
avilagaston9 wants to merge 1 commit intomainfrom
Conversation
`get_block_l1_messages` in `crates/l2/common/src/messages.rs` filters logs with `log.topics.contains(&L1MESSAGE_EVENT_SELECTOR)`, which returns true if the selector appears anywhere in the topic list — not specifically at index 0. After the filter passes, the parser unconditionally reads `topics[1]` as `from`, `topics[2]` as `data_hash`, and `topics[3]` as `message_id`. So any log with at least four topics that happens to contain the L1Message selector somewhere past index 0 is parsed as a fake `L1Message`. The companion `get_block_l2_out_messages` already uses the correct `topics.first() == Some(&L2MESSAGE_EVENT_SELECTOR)`, so this is an inconsistency between the two filters. The bug isn't currently reachable through the real EVM emission path: the only contract whose address matches `MESSENGER_ADDRESS` is `Messenger.sol`, which only emits `L1Message` (selector at topics[0]) and `L2Message` (only 2 topics, so the parser bails out via `topics.get(2) == None`). But the function is a public API in `ethrex_l2_common::messages` consumed by at least three callers (committer, RPC, state-reconstruct) that get fed receipts from many sources (storage, peers, replay). A future event added to the messenger with three or more indexed parameters whose values can land on `L1MESSAGE_EVENT_SELECTOR` would silently start producing fake `L1Message`s — which would then become withdrawal- claim leaves committed to L1 by the committer. The new test `get_block_l1_messages_misparses_log_with_selector_off_topic_zero` in `test/tests/l2/l1_messages_filter.rs` feeds a hand-crafted log into `get_block_l1_messages` and pins the current (buggy) behaviour: the function returns a fabricated `L1Message` whose `from` is the bottom 20 bytes of `L1MESSAGE_EVENT_SELECTOR`. After the fix (`topics.first() == Some(&L1MESSAGE_EVENT_SELECTOR)`), the filter rejects the crafted log and the assertion has to be updated to `messages.len() == 0`, prompting an explicit review of the fix.
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
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.
Motivation
get_block_l1_messagesincrates/l2/common/src/messages.rsfilters logs as
topics.contains(&SELECTOR)returns true if the selector appearsanywhere in the topic list, not specifically at index 0. After
the filter passes, the parser unconditionally reads
topics[1]as
from,topics[2]asdata_hash, andtopics[3]asmessage_id— so any log with at least four topics that happensto contain
L1MESSAGE_EVENT_SELECTORsomewhere past index 0 isparsed as a fake
L1Message.The companion
get_block_l2_out_messagesalready uses thecorrect
so this is an inconsistency between the two filters in the same
file.
Current reachability
The bug isn't currently reachable through the real EVM emission
path: the only contract whose
addressmatchesMESSENGER_ADDRESSisMessenger.sol, which only emitsL1Message(selector attopics[0]) andL2Message(only 2topics, so the parser bails out via
topics.get(2) == None).EIP-3541 prevents anyone from deploying new code at
MESSENGER_ADDRESSwhose runtime starts with0xef, but itdoesn't constrain the topic values an event can carry — and
the messenger is upgradeable.
So this is a defensive / latent bug: the day a new event with
three or more indexed parameters is added to the messenger and
one of them carries the L1Message selector value, fake
L1Messages would silently start being parsed. Those would feedinto the committer's
l1_out_message_hashes, which is committedon-chain as the batch's withdrawal merkle root, and the
withdrawal hash for a fabricated message could collide with a
real claim leaf — meaning withdrawals could in principle be
forged via a future event addition. The class is
"a tx that has the wrong leading topic but contains the
selector somewhere" — a known gotcha across multiple Ethereum
clients (e.g. erigon's
ContainsSpecificTopics
discussion), so worth fixing now rather than later.
Description
Adds a regression test
get_block_l1_messages_misparses_log_with_selector_off_topic_zeroin
test/tests/l2/l1_messages_filter.rsthat feeds a hand-crafted
Log(address =MESSENGER_ADDRESS, topics =[unrelated, L1MESSAGE_EVENT_SELECTOR, fake_data_hash, fake_message_id]) intoget_block_l1_messagesand pins thecurrent (buggy) outcome: the function returns a fabricated
L1Messagewhosefromis the bottom 20 bytes ofL1MESSAGE_EVENT_SELECTOR,data_hashis the third topic,and
message_idis the fourth.After the fix
(
log.topics.first() == Some(&*L1MESSAGE_EVENT_SELECTOR)), thefilter rejects the crafted log and the assertion has to be
updated to
messages.len() == 0, prompting an explicit reviewof the fix.
The fix itself is intentionally not in this PR — surfacing the
gap and its reproducer first lets the maintainers decide
whether to apply the same
first()pattern asget_block_l2_out_messagesor take a different approach (e.g.also asserting
topics.len() >= 4defensively).Reproduction
The test passes on
main(the buggy filter accepts the craftedlog).
Checklist
STORE_SCHEMA_VERSION(crates/storage/lib.rs) if the PR includes breaking changes to theStorerequiring a re-sync.