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.
Summary
Security and correctness review of the
qp-headercrate (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_feltssilently reduces non-canonical inputsSeverity: Medium (latent)
unsafe_digest_bytes_to_feltspacks each 8-byte chunk of a 32-byte root into a Goldilocks field element viafrom_u64(u64::from_le_bytes(bytes)). If any chunk's u64 value falls in[p, 2^64)(wherep = 2^64 - 2^32 + 1),from_intreduces it modulop, 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, andextrinsics_rootare 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:Finding 2: Block number silently truncated to 32 bits in hash preimage
Severity: Medium (latent)
U256::as_u32()returns only the low 32 bits. The generic boundAtLeast32BitUnsignedpermitsu64/u128number types. IfNumberis wider thanu32, 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!:Additional notes
No domain separator:
hash_variable_lengthis 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 vsHash::hash_of: The trait impl overrideshash()with custom felt-aligned encoding.header.hash()andPoseidonHasher::hash_of(&header)produce different results. Any Substrate code path that bypasses thehash()override would compute a different block hash. This is by design, but worth documenting as a fragile integration point.with_capacityestimate: 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.