polygon/sync: ignore empty NewBlockHashes to prevent observer panic#21560
Merged
Conversation
An empty NewBlockHashes packet (RLP 0xc0) decodes to a zero-length slice without error, so the surviving execution/p2p inbound path delivers it to observers unpenalized. The tip-events observer then indexed blockHashes[0] with no length check; since observers run in a bare go observer(event) with no recover, that panic aborted the whole process — a remote, unauthenticated crash DoS on polygon/Bor nodes. Guard the observer: ignore announcements with no entries before any blockHashes[0] access. Closes erigontech/security#72 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a crash-DoS in Polygon/Bor tip-events processing by ignoring empty NewBlockHashes announcements before indexing into the slice.
Changes:
- Add a length guard in the
NewBlockHashesobserver to avoidblockHashes[0]panics on empty packets. - Add tests to ensure empty announcements do not panic and non-empty announcements still emit
NewBlockHashesevents.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
polygon/sync/tip_events.go |
Adds len(blockHashes)==0 early-return to prevent slice indexing panic. |
polygon/sync/tip_events_empty_newblockhashes_test.go |
Adds regression tests for empty and non-empty NewBlockHashes observer behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
… year Address PR review feedback: the test cleanup now asserts te.Run exits with context.Canceled instead of discarding the error (matching tip_events_test.go), and the new file uses the current copyright year. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Giulio2002
approved these changes
Jun 1, 2026
Giulio2002
left a comment
Contributor
There was a problem hiding this comment.
LGTM — obvious guard against empty NewBlockHashes packets, with focused regression coverage for the panic path.
Giulio2002
approved these changes
Jun 1, 2026
Giulio2002
left a comment
Contributor
There was a problem hiding this comment.
LGTM — obvious guard against empty NewBlockHashes packets, with a focused regression test.
taratorio
approved these changes
Jun 2, 2026
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.
Summary
An empty NewBlockHashes packet (RLP
0xc0) decodes to a zero-length slice without error, so the survivingexecution/p2pinbound path delivers it to observers unpenalized. Thepolygon/synctip-events observer then indexedblockHashes[0]with no length check and panicked; observers run in a barego observer(event)with norecover(), so the panic aborted the whole process — a remote, unauthenticated crash DoS on polygon/Bor nodes.Fix: ignore announcements with no entries before any
blockHashes[0]access.Closes erigontech/security#72
Scope
Only the polygon/Bor astrid path (
polygon/synctip-events) is affected — it is the live consumer of inbound NewBlockHashes. On post-Merge Ethereum the eth multi-client drops these messages (#21505), so this is polygon-specific.peer_tracker's observer ranges over the slice and is already safe on empty;NewBlockdecodes into a struct, so an empty packet is rejected at decode rather than reaching an observer.Test plan
TestTipEventsEmptyNewBlockHashesDoesNotPanic— empty packet no longer panics the observer (red→green; the red failure wasindex out of range [0] with length 0attip_events.go:213)TestTipEventsNewBlockHashesEmitsEvent— non-empty announcements still emit aNewBlockHasheseventgo test -race ./polygon/sync/(new tests,-count=5)make lintclean;make erigon integrationNote
A
recover()incommon/eventObservers.Notifywould be complementary defense-in-depth — it would catch any future observer panic, not just this one — but per the issue it should not replace this targeted length guard.🤖 Generated with Claude Code