fix(shielded-pool)!: reject non-canonical and zero commitments and nullifiers

[nol4lej]

What this fixes

Commitments and nullifiers are stored as raw bytes and used as raw StorageMap keys, while byte-to-field conversion reduces modulo the BN254 prime p. Without a canonicity check, n and n + p are two distinct keys for the same field element — a nullifier presented twice under different bytes, and one note spent twice.

Nothing reached storage before this. The verifier passes public inputs through to_field_elements, which re-encodes and compares byte for byte, so a non-canonical value already failed verification. But that was a single check on a single path: any future route that records a nullifier without full verification — a relayer pre-check, a mempool dedup cache, a runtime API — would have lost the protection silently.

Commitment::is_canonical and Nullifier::is_canonical now guard every write path: shield, where the depositor picks the bytes with no proof constraining them and which is therefore the least guarded way into the tree; private_transfer, for both nullifiers and output commitments; and unshield, for the nullifier and the change commitment.

The helper this delegates to (FieldElement::is_canonical_le in zk-core) already existed, with a doc saying callers should reject non-canonical input at the trust boundary. It had no callers.

Canonicity is kept separate from validate() / is_valid(): an all-zero nullifier marks a dummy input slot and is itself canonical, so folding the two together would have rejected legitimate transfers.

The gap the dev-node probe found

The first adversarial run came back 17/18. The failure was real, not a bad test.

A zero commitment was accepted. Zero is canonical, so the canonicity check alone let it through. It has to be refused separately: the tree represents an absent leaf with [0u8; 32] — literally get_zero_hash_cached(0) — so a stored zero leaf is indistinguishable from an empty slot when subtree_root rebuilds a pruned level, which is exactly the path sealed-tree pruning introduced. Nobody can prove a preimage for it either, so it would sit in the tree permanently as dead weight.

Commitment::is_valid() existed for precisely this. Nothing called it. shield and private_transfer now check it alongside is_canonical(); unshield already skipped a zero change commitment through its has_change branch.

Breaking change

A client generating raw 32-byte values without reducing mod p is now rejected with InvalidPublicSignals. Nothing the chain produces is affected — commitments and nullifiers come out of Poseidon and are always canonical.

Ten of this repo's own tests were submitting exactly that, using fillers like [0xC1; 32] and [0x31; 32]. Both sit above p, whose top byte is 0x30, so they were exercising a shape the chain never produces. [0x31; 32] is the instructive one: it fails by a single byte over the line. All moved onto canonical values through documented helpers.

One of them held its value in a loop variable rather than a literal, so it survived a pattern search and only surfaced when the tests ran.

Verification

pallet-shielded-pool	321 (14 new)
pallet-evm-precompile-shielded-pool	65
Clippy, CI feature set, -D warnings	exit 0
Dev-node adversarial	18/18

The decisive unit test builds n and n + p, asserts they are different bytes but the same field element, and requires that only the canonical one is accepted. Without that second assertion it would pass against a guard that rejected everything.

The dev-node run submits the same pair on-chain and confirms only one ends up in the reverse index — storage as ground truth, not the dispatch result. It also checks that p itself and all-ones are refused while p - 1, the largest canonical value, is accepted; that a zero commitment is refused despite being canonical; and that 25 rejected shields in a burst grow the tree by nothing. Block height is verified after each batch, so a rejection that wedged block production would show up as a failure rather than as silence.

Clippy caught a duplicated #[test] attribute that an insertion left behind. The tests passed either way — rustc tolerates it — but it failed under -D warnings.

Versions

pallet-shielded-pool 0.15.0 → 0.16.0. Minor rather than patch: the change rejects input it previously accepted. 0.15.0 is already merged, so this opens a new section rather than amending it.