Header crate security review

[n13]

Summary

Security and correctness review of the qp-header crate (primitives/header/src/lib.rs). Two latent security issues found -- neither is currently exploitable in the production runtime (BlockNumber = u32, Hashing = PoseidonHasher), but both fail silently if those assumptions are violated.

---

Finding 1: unsafe_digest_bytes_to_felts silently reduces non-canonical inputs

Severity: Medium (latent)

unsafe_digest_bytes_to_felts packs each 8-byte chunk of a 32-byte root into a Goldilocks field element via from_u64(u64::from_le_bytes(bytes)). If any chunk's u64 value falls in [p, 2^64) (where p = 2^64 - 2^32 + 1), from_int reduces it modulo p, mapping it into [0, 2^32). A different 32-byte input with the same residues would produce the same 4 felts -- a hash collision without breaking Poseidon.

This is safe today because parent_hash, state_root, and extrinsics_root are all Poseidon outputs (canonical felts, always < p). But there is no runtime assertion guarding the assumption. If any upstream component (e.g. the state trie hasher) is changed to produce non-Poseidon outputs, the hash function silently degrades.

Suggested fix: Add a debug_assert! on each chunk:

let val = u64::from_le_bytes(bytes);
debug_assert!(val < Goldilocks::ORDER_U64, "non-canonical felt in digest encoding");

---

Finding 2: Block number silently truncated to 32 bits in hash preimage

Severity: Medium (latent)

let number = self.number.into(); // Number -> U256
felts.push(Goldilocks::from_int(number.as_u32() as u64));

U256::as_u32() returns only the low 32 bits. The generic bound AtLeast32BitUnsigned permits u64/u128 number types. If Number is wider than u32, two headers whose block numbers differ only above bit 32 produce identical hash preimages -- a second-preimage collision.

The runtime uses BlockNumber = u32, so this is safe today. But the type system does not enforce the assumption.

Suggested fix: Add a debug_assert!:

let number: U256 = self.number.into();
debug_assert!(number <= U256::from(u32::MAX), "block number exceeds u32 range for felt encoding");
felts.push(Goldilocks::from_int(number.as_u32() as u64));

---

Additional notes

• No domain separator: hash_variable_length is shared across header hashing, wormhole addresses, trie nodes, etc. The sponge padding (1 || 0*) differentiates message lengths, and the natural range separation between encoding schemes (hash felts in [0, p) vs digest felts in [0, 2^32)) prevents cross-domain collisions. Low risk, but an explicit domain tag would harden the construction.

• hash() override vs Hash::hash_of: The trait impl overrides hash() with custom felt-aligned encoding. header.hash() and PoseidonHasher::hash_of(&header) produce different results. Any Substrate code path that bypasses the hash() override would compute a different block hash. This is by design, but worth documenting as a fragile integration point.

• with_capacity estimate: The 41-felt capacity hint (4*3 + 1 + 28) is tuned for typical consensus digests. Blocks with larger digests will trigger a reallocation. Not a security issue.

[n13]

That .hash() != Hash::hash_of is pretty unfortunate and counter-intuitive.

But maybe there's some trait requirements that make this impossible to change...

Quite likely source for errors in development