Skip to content

security: add domain separation to Poseidon2 leaf/node/KYC hashing #61

Description

@tech-adrian

Area
circuits/shielded_pool/src/main.nr, circuits/compliance/src/main.nr, circuits/disclosure/src/main.nr, circuits/hasher, contracts/pool, contracts/compliance

Problem
Merkle leaves and internal tree nodes both use the same undifferentiated hash2(a, b) (e.g. leaf = hash2(nullifier, secret), internal node = hash2(left, right)). Because commitments are public via get_commitments(), this is the classic "same compression function for leaves and internal nodes" anti-pattern: nothing on-chain or in-circuit distinguishes a genuine deposit leaf from hash2(commitment_i, commitment_{i+1}), a value anyone can compute from two public sibling commitments. The compliance KYC hash (hash2(kyc_preimage, 0)) uses the same undifferentiated form as the nullifier hash, so the pool-nullifier and compliance-KYC hash domains aren't cryptographically separated from each other either. This isn't a currently-known exploitable theft path (an attacker would still need a real deposit at the relevant slot), but it's a real cryptographic hygiene gap worth closing before any external audit or mainnet consideration.

Proposed change
Introduce domain-separated hashing: a distinct tag for leaf hashing vs. internal-node hashing (e.g. hash3(LEAF_DOMAIN, nullifier, secret) for leaves, plain hash2 for internal nodes, or vice versa), and a distinct tag for the KYC-hash construction. This must be changed consistently across circuit, frontend (poseidon2.ts), and contract (soroban_poseidon) call sites, since all three currently have to agree byte-for-byte (per the existing pinned test_poseidon_matches_circuit_zero_hash / test_reconstructed_root_matches_onchain_root tests) — this is a VK-breaking change and needs bb write_vk diff + redeploy, planned deliberately rather than shipped incidentally.

Acceptance Criteria

  • Domain-separated hashing designed and documented (which tag for leaf vs. node vs. KYC hash)
  • Implemented consistently across circuit/frontend/contract, with regression tests locking the new fixed values (like the existing Poseidon2 consistency tests)
  • Confirmed this requires (and includes) a VK regen + redeploy plan, called out explicitly in the PR
  • Relevant tests (just test, pnpm test, cargo test, or nargo test as applicable) pass locally and in CI
  • A screenshot or short screen recording is attached to the PR showing the change working (a full local deploy + deposit + withdraw round-trip with the new domain-separated hashes) — required before this can be merged.

Open your PR against the dev branch, not main. All active development merges into dev.

Discuss this issue / coordinate work: join the DShield contributor Telegram group: https://t.me/+SiGHH24No9U2MDJk

Metadata

Metadata

Assignees

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions