core/types: guard (*Header).GetValidatorBytes against short Extra

[kajaaz]

Description

(*Header).GetValidatorBytes in core/types/block.go panics with runtime error: slice bounds out of range when called on a header whose Extra field is shorter than ExtraVanityLength + ExtraSealLength (< 97 bytes) and the chain configuration is pre-Cancun. The pre-Cancun branch performs the slice h.Extra[ExtraVanityLength : len(h.Extra)-ExtraSealLength] with no length guard, while the post-Cancun branch and the three companion helpers (GetBaseFeeParams, DecodeBlockExtraData, (*Block).GetTxDependency) all already guard this path. The comment at consensus/bor/bor.go:670 acknowledges the function relies on upstream validation, but because the function is exported, any out-of-tree consumer (indexer, light client, block explorer, MEV tooling) that calls it on unvalidated headers will panic on attacker-controllable input.

This PR hoists the existing length check to the top of GetValidatorBytes so it covers both the pre-Cancun and post-Cancun branches uniformly. Behaviour for valid headers is unchanged.

Discovered with Zorya, a concolic-execution engine for Go binaries. Zorya produced a Z3 witness extra.len = 1 after 65.7 s; confirmed to reproduce natively as slice bounds out of range [:-64]. See issue #2221 for the full write-up, SAT witness, and reproduction steps.

Fixes #2221

Changes

• Bugfix (non-breaking change that solves an issue)

Breaking changes

None. The only behavioural change is that GetValidatorBytes now returns nil instead of panicking when len(h.Extra) < ExtraVanityLength + ExtraSealLength on a pre-Cancun config. All in-tree call sites already guard against this condition via validateHeaderExtraField, so they are unaffected.

Nodes audience

All nodes that import core/types. No flag or configuration change required.

Checklist

• I have added at least 2 reviewer or the whole pos-v1 team
• I have added sufficient documentation in code
• I will be resolving comments - if any - by pushing each fix in a separate commit and linking the commit hash in the comment reply
• Created a task in Jira and informed the team for implementation in Erigon client (if applicable)
• Includes RPC methods changes, and the Notion documentation has been updated

Cross repository changes

• This PR requires changes to heimdall
• This PR requires changes to matic-cli

Testing

• I have added unit tests
• I have added tests to CI
• I have tested this code manually on local environment

Manual tests

1. Checked out develop, appended TestGetValidatorBytesShortExtraVerbose to core/types/block_test.go (pre-fix). All 7 short-Extra lengths panicked as expected (slice bounds out of range [:-65] through [32:31]). Test passed, confirming the bug is present on unpatched code.
2. Applied this PR's patch to block.go. Re-ran go test -v -run TestGetValidatorBytesShortExtra ./core/types/. Test passed: no panics, all short-Extra cases returned nil, happy-path case returned correct 40 validator bytes.
3. Ran full go test ./core/types/, all existing tests pass.

[claude]

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

[Copilot]

Pull request overview

Hardens the exported (*Header).GetValidatorBytes helper in core/types to avoid a slice-bounds panic on malformed headers (short Extra) in the pre-Cancun path, and adds a regression test to ensure short inputs return nil instead of panicking.

Changes:

• Hoists the len(Extra) >= ExtraVanityLength+ExtraSealLength guard to cover both pre- and post-Cancun branches in GetValidatorBytes.
• Documents the new nil return behavior for short Extra on this API.
• Adds a regression test covering multiple short Extra lengths across both Cancun/non-Cancun configs plus a happy-path assertion.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
core/types/block.go	Moves the Extra length guard ahead of the Cancun fork to prevent pre-Cancun slice panics and return nil safely.
core/types/block_test.go	Adds a regression test ensuring short Extra never panics and returns nil, plus a pre-Cancun happy-path check.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

core/types/block_test.go:966

• Rather than hard-coding a specific set of short lengths, consider deriving them relative to ExtraVanityLength+ExtraSealLength (e.g., 0, 1, threshold-1) so the regression test remains valid if the constants ever change.

	shortLens := []int{0, 1, 32, 64, 65, 80, 96}

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

[pratikspatil024]

@claude review once.

[claude]

Claude finished @pratikspatil024's task in 1m 30s —— View job

---

Review of PR #2222

• Read changed files and gather context
• Review core/types/block.go changes
• Review core/types/block_test.go changes
• Check existing guard patterns in companion helpers
• Post final review

---

Verdict: Approve — the fix is correct and minimal.

core/types/block.go

The change hoists the len(h.Extra) < ExtraVanityLength+ExtraSealLength guard to the top of GetValidatorBytes, before the Cancun fork check. This is the right approach:

• The pre-Cancun branch previously sliced without any guard, which panics on short Extra.
• The post-Cancun branch already had this guard — now both branches share it.
• The companion helpers (GetBaseFeeParams at line 535, DecodeBlockExtraData at line 554) use the same pattern (return nil silently), so this is consistent.
• All in-tree call sites go through validateHeaderExtraField (consensus/bor/bor.go:464→549) which rejects short Extra with an error before GetValidatorBytes is ever called, so the behavioral change is only visible to out-of-tree consumers.

One minor note: The log.Error that was previously on the post-Cancun path has been removed. This is actually an improvement — GetBaseFeeParams and DecodeBlockExtraData both return nil silently in this case, and since the function is exported and may be called on unvalidated headers, an error-level log is noise that could be weaponized for log spam. The Copilot reviewer flagged this removal; I think it's the right call.

core/types/block_test.go

The regression test is solid:

• Tests both pre-Cancun and post-Cancun configs across 7 short Extra lengths.
• Uses recover() to catch panics and turn them into test failures — good pattern for regression testing.
• Includes a pre-Cancun happy path that verifies valid input still produces correct output.

Minor suggestions (non-blocking):

1. Missing post-Cancun happy path — The test only has a pre-Cancun happy path. Adding a post-Cancun case with valid RLP-encoded BlockExtraData would fully cover both branches. This is a nice-to-have, not a blocker.

2. Hard-coded boundary values — The shortLens list hard-codes {0, 1, 32, 64, 65, 80, 96}. Deriving the threshold from ExtraVanityLength+ExtraSealLength (currently 97) would make the test resilient to constant changes. Again, cosmetic — the constants haven't changed in years.

Security Assessment

Per the blockchain security rules: this is a HIGH severity fix (panic from out-of-range slice on exported function), escalated to HIGH since malicious peers could craft headers with short Extra that reach out-of-tree consumers. The in-tree consensus path is already protected by validateHeaderExtraField, so the in-tree risk is low.

The fix follows the established pattern exactly (compare with GetBaseFeeParams, DecodeBlockExtraData, and GetTxDependency), introduces no new logic, and doesn't affect consensus behavior for valid headers.

---

[claude]

LGTM — small defensive guard that matches the pattern of the companion helpers, with a regression test.

Extended reasoning...

Overview

The PR hoists an existing length check to the top of (*Header).GetValidatorBytes in core/types/block.go so that both the pre-Cancun and post-Cancun branches return nil instead of panicking when len(h.Extra) < ExtraVanityLength + ExtraSealLength. A regression test (TestGetValidatorBytesShortExtra) covers a range of short Extra lengths for both branches plus a pre-Cancun happy-path assertion. Two files changed; the diff is ~15 LOC of production code.

Security risks

None introduced. The change is strictly safer: it converts a previously panicking code path into a silent nil return, eliminating a DoS vector for any out-of-tree consumer that might call this exported helper on unvalidated headers. All in-tree callers (consensus/bor/bor.go:472, bor.go:640, bor.go:660, consensus/bor/snapshot.go:156) either feed the result to valset.ParseValidators (handles nil) or take len() of it — none rely on the prior panic behavior. The removed log.Error on the post-Cancun short-Extra path now matches GetBaseFeeParams / DecodeBlockExtraData behavior, which is consistent.

Level of scrutiny

Low. This is a defensive bugfix that follows an existing established pattern within the same file. The semantics for valid headers are unchanged, the behavior change for invalid headers is provably safer, and the function's callers all tolerate nil output. The regression test directly demonstrates that the prior panic is gone.

Other factors

The Copilot reviewer raised four cosmetic suggestions (log noise, test comment drift, hardcoded < 97 threshold, post-Cancun happy-path coverage) — all have been marked resolved on the PR. SonarCloud quality gate passed. The bug hunting system found no bugs. The author (pratikspatil024) explicitly requested a review.