bug: add length guard to decodeMasternodesFromHeaderExtra (both engines) to match verifyHeader

[kajaaz]

Summary

decodeMasternodesFromHeaderExtra computes a slice length from header.Extra with no length precondition. When len(header.Extra) <= 77, the size expression is negative and make([]common.Address, negative) panics with runtime: makeslice: len out of range. The v1 sibling verifyHeader already validates this length; the helper does not. Today the helper is only reached on locally-stored, already-verified headers, so this is a latent defect worth fixing defensively rather than an exploitable crash.

Robustness / defense-in-depth. Not a security/DoS report : see Reachability below.

Affected code

consensus/XDPoS/engines/engine_v1/utils.go and consensus/XDPoS/engines/engine_v2/utils.go:

func decodeMasternodesFromHeaderExtra(checkpointHeader *types.Header) []common.Address {
	masternodes := make([]common.Address,
		(len(checkpointHeader.Extra)-utils.ExtraVanity-utils.ExtraSeal)/common.AddressLength)
	for i := 0; i < len(masternodes); i++ {
		copy(masternodes[i][:], checkpointHeader.Extra[utils.ExtraVanity+i*common.AddressLength:])
	}
	return masternodes
}

With ExtraVanity = 32, ExtraSeal = 65, AddressLength = 20, the length is (len(Extra) - 97) / 20, which is negative for len(Extra) <= 77, causing make to panic. (sigHash in the same files already notes // Yes, this will panic if extra is too short.)

Reachability (why this is not flagged as a DoS)

The engine dispatch in XDPoSConfig.BlockConsensusVersion (params/config.go:578) selects the v2 engine only for header.Number > V2.SwitchBlock (strictly greater), while the unguarded v2 decode in getExtraFields fires only at header.Number == V2.SwitchBlock. Those two conditions are mutually exclusive, so a peer header at the switch height is handled by the v1 engine instead.

The v1 verify path is guarded: engine_v1/engine.go:187-192 rejects len(Extra) < ExtraVanity + ExtraSeal with ErrMissingSignature before any decode, so a malformed short header never enters the database.

All live callers of the helper read from the local chain store via chain.GetHeaderByNumber or chain.GetHeaderByHash, operating on headers that have already passed verification: engine_v1/engine.go:669, the exported GetMasternodesFromCheckpointHeader called from internal/ethapi/api.go, eth/hooks/engine_v2_hooks.go, and contracts/utils.go, plus the v2 getExtraFields callers in verifyHeader.go:65 (which only reach the DecodeBytesExtraFields path for num > SwitchBlock, never the masternode-decode branch).

The panic therefore only fires on already-verified, locally-sourced data. The motivation for the fix is defense-in-depth: the guard is genuinely missing here whereas its v1 sibling has one, the exported API is callable by any future caller passing an unverified header, and a dispatch change that makes the switch block itself v2 would make the v2 branch live.

Reproduction (verbatim logic)

package main

import "fmt"

const (ExtraVanity = 32; ExtraSeal = 65; AddressLength = 20)
type Address [AddressLength]byte

func decode(extra []byte) []Address {
	mn := make([]Address, (len(extra)-ExtraVanity-ExtraSeal)/AddressLength)
	for i := 0; i < len(mn); i++ {
		copy(mn[i][:], extra[ExtraVanity+i*AddressLength:])
	}
	return mn
}

func main() {
	for _, n := range []int{0, 65, 77, 78, 200} {
		func() {
			defer func() {
				if r := recover(); r != nil {
					fmt.Printf("len=%-3d => PANIC: %v\n", n, r)
				}
			}()
			fmt.Printf("len=%-3d => %d masternodes (ok)\n", n, len(decode(make([]byte, n))))
		}()
	}
}

len=0   => PANIC: runtime error: makeslice: len out of range
len=65  => PANIC: runtime error: makeslice: len out of range
len=77  => PANIC: runtime error: makeslice: len out of range
len=78  => 0 masternodes (ok)
len=200 => 5 masternodes (ok)

Suggested fix

func decodeMasternodesFromHeaderExtra(checkpointHeader *types.Header) []common.Address {
	extra := checkpointHeader.Extra
	if len(extra) < utils.ExtraVanity+utils.ExtraSeal {
		return nil
	}
	n := (len(extra) - utils.ExtraVanity - utils.ExtraSeal) / common.AddressLength
	masternodes := make([]common.Address, n)
	for i := 0; i < n; i++ {
		copy(masternodes[i][:], extra[utils.ExtraVanity+i*common.AddressLength:])
	}
	return masternodes
}

Apply to both engine_v1/utils.go and engine_v2/utils.go.

Detection method

This bug was found using Zorya, a concolic execution engine for Go binaries. Zorya symbolized the Extra length argument and used Z3 to find concrete inputs that trigger the panic. The first satisfiable state was produced in 93 seconds.

The exact command used:

zorya targets/_third_party/xdpos/src/cmd/zorya_masternodes/zorya_masternodes_real --lang go --compiler gc --mode function 0x4b7560 --thread-scheduling main-only --arg "c800000000000000" --negate-path-exploration

0x4b7560 is the address of main.exerciseDecodeMasternodes, a thin wrapper that calls the verbatim upstream function with a symbolized length. The concrete seed c800000000000000 is n=200 in little-endian int64, a safe input that produces 5 masternodes and establishes the normal execution path before Zorya explores negated branches.

It produced the following satisfiable states within 116 seconds:

Witness n	Opcode	Address	Meaning	Elapsed
MinInt64	INT_SUB	0x4b758a	signed subtraction len(Extra) - 97 underflows	93s
73	INT_MULT	0x4b7306	make([]Address, -1) because (73-97)/20 = -1	108s
0	LOAD	0x478872	make([]Address, -4) because (0-97)/20 = -4	116s

All three witnesses were confirmed by running the standalone reproducer above.