Security/zk core hardening#107
Merged
Merged
Conversation
The Poseidon hashers used `.expect()` on both circuit init and hashing. A panic there behaves differently across execution environments: a native panic aborts the node process while a WASM panic is a recoverable trap. The same malformed input could therefore crash native validators while WASM nodes merely reject the block — a consensus-divergence risk. Route all arities (hash_2, hash_4, hash_5, poseidon_hash_1) through a single internal `poseidon_circom` helper that returns a deterministic value instead of panicking. The failure path is provably unreachable (constant arity, already-reduced Fr inputs) and is now documented as an invariant, keeping native and WASM behavior identical by construction. Add tests covering boundary field elements (0 and p-1) for no-panic and determinism, plus core/trait equivalence.
poseidon_hash_2/4 asserted their inputs were exactly 32 bytes and then copy_from_slice'd them. Both paths panic on a wrong-length input. As native host functions a panic aborts the node process (a WASM trap is recoverable), so a malformed length could crash native validators while WASM nodes stay up — a consensus-divergence risk. Read inputs through a read_32_le helper that zero-pads or truncates to 32 bytes deterministically and never panics. Callers always pass 32 bytes, so this is purely defensive; behavior is identical on native and WASM.
… vectors The existing native-vs-WASM test compared the host function against LightPoseidonHasher, which the host calls internally — a tautology. And nothing checked the hash against the circuit it must match. Add tests/poseidon_vectors.rs with canonical circomlib BN254 Poseidon outputs for arities 1/2/4/5, plus native/WASM equivalence over a spread of inputs. These vectors fail if light-poseidon-nostd ever drifts from the compiled circuit's parameters. Document the Native==Wasm==Circuit invariant in the crate docs.
Fr::from_le_bytes_mod_order reduces its input mod p, so x and x+p (both 32 bytes) yield the same field element while being different byte strings. Layers that compare raw bytes — nullifier sets, commitment maps, external-address commitments — would treat them as distinct while a proof over the reduced element accepts both, a collision/double-spend vector. Add FieldElement::is_canonical_le and from_canonical_le so trust boundaries can reject non-canonical (>= p) encodings before use. The host functions are unchanged: they only receive bytes produced by field_to_bytes, which are canonical by construction. Applying the check at the raw-byte boundaries lives in the consuming pallets.
The crate is no_std but `default` enables std. Consumers that forget `default-features = false` would leak std into the WASM runtime build. All in-tree consumers already disable defaults; this locks that in. Document the contract in Cargo.toml and add a CI step that checks the crate builds with --no-default-features and with only poseidon-native, so a future feature or dependency change can't silently break the WASM build. `default` is left unchanged (Substrate convention; no consumer relies on it).
field_to_bytes / bytes_to_field and the host-function output used copy_from_slice(&bytes[..32]), which panics if to_bytes_le() ever returns fewer than 32 bytes. BN254 Fr always yields 32, so this was latent, but the slice is fragile under a type change. Copy by min-length (zero-pad/truncate) and reuse read_32_le for the host output. Add round-trip and no-panic tests for boundary field elements.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
orbinum-zk-core 1.1.0
Security and robustness hardening of the Poseidon primitives:
See primitives/zk-core/CHANGELOG.md for details.