NODES response decode aborts the whole batch when one record fails to decode

[MysticRyuujin]

Summary

Decoding a NODES response aborts the entire response if any single record fails to decode, discarding all sibling records in that message. A record whose port key (tcp/udp/tcp6/udp6) does not fit in a u16 is one way to trigger this — such a value is legal at the RLP layer and can be self-signed into an ENR, so it is attacker-reachable.

Where

src/rpc.rs, Message::decode, the NODES arm (4 => { ... }):

let enr_rlp = Enr::<CombinedKey>::decode(
    &mut &payload[..node_header.length_with_payload()],
)?;   // <- one bad record fails the whole message
payload.advance(enr_rlp.size());
enr_list_rlp.append(&mut vec![enr_rlp]);

The ? propagates a single record's decode error out of Message::decode, so the whole NODES message (up to the per-response record limit) is dropped.

The record-level failure itself comes from the enr crate, which decodes the port keys strictly as u16 (enr::Enr as Decodable, the TCP_ENR_KEY | TCP6_ENR_KEY | UDP_ENR_KEY | UDP6_ENR_KEY => { let port = u16::decode(payload)?; ... } arm). An out-of-range port therefore fails the entire record decode, which then fails the entire message here.

Why it matters

NODES records are relayed from a peer's own routing table, so a single malformed-but-signed ENR that lands in many peers' tables degrades the NODES responses those honest peers serve — not just responses from a malicious peer. Downstream, this is a peer-acquisition availability surface for every consumer of the crate (e.g. Lighthouse, Grandine, and reth all depend on discv5 + enr with discovery enabled by default).

Spec position

This is not a spec violation — it is an unspecified implementation choice. The devp2p discv5 wire/theory specs are silent on per-record fault tolerance inside a NODES response, and EIP-778 defines ports only as "big endian integer" (no u16/65535 constraint; only the 300-byte whole-record limit). Isolating per-record is equally spec-compliant and more robust — some other implementations already skip bad records rather than dropping the batch. Filing this to decide the intended behavior rather than asserting a bug.

Possible direction

Skip an undecodable record and continue, advancing by the already-bounds-checked node_header.length_with_payload():

let node_len = node_header.length_with_payload();
match Enr::<CombinedKey>::decode(&mut &payload[..node_len]) {
    Ok(enr_rlp) => {
        payload.advance(enr_rlp.size());
        enr_list_rlp.push(enr_rlp);
    }
    Err(e) => {
        debug!(error = ?e, "skipping undecodable ENR in NODES response");
        payload.advance(node_len);
    }
}

(Illustrative — happy to open a PR if the maintainers agree on skip-vs-reject. There is also a related decision on whether enr should reject or clamp out-of-range ports at all; see sigp/enr#26 on stricter type enforcement for spec-reserved keys.)

Found during a cross-client discv5 audit; the same "one bad record poisons the batch" pattern showed up independently in several implementations.

[pawanjay176]

NODES records are relayed from a peer's own routing table, so a single malformed-but-signed ENR that lands in many peers' tables degrades the NODES responses those honest peers serve — not just responses from a malicious peer.

I don't get this. I would expect a malformed ENR to never land in a peer's dht. Is that happening somehow?

[MysticRyuujin]

You're right for strict clients — sigp/discv5 rejects it at decode, so a Lighthouse/Nimbus/Teku node never stores or relays it. I overstated that.

But not everyone decodes strictly. Prysm uses go-ethereum's enr, which stores k/v pairs as raw RLP without checking them — a bad tcp is only caught on an explicit Load (Node.TCP() returns 0). So it keeps the record and re-serves it in NODES, and a sigp node doing a lookup gets it inside an otherwise-fine batch and drops the whole batch.

So it's real, but propagation is via lenient impls, not other strict nodes, and impact is minor — a dropped batch and slightly slower discovery. Skipping the bad record just lets strict clients degrade gracefully when a lenient peer relays one.

[pawanjay176]

I don't have a strong preference but feel like stricter is better here. Tagging others for opinions @jxs @ackintosh

[ackintosh]

I also prefer the stricter approach. It seems to me that the more fundamental issue lies with implementations that accept and re-serve malformed ENRs.