Skip to content

fix(zk): enforce address binding, verifier hardening, and structural guards (#348) #459

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
s6pa1rta3n-lab wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

fix(zk): enforce address binding, verifier hardening, and structural guards (#348) #459

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions artifacts/zk/manifest.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,12 @@
"fee",
"denomination"
],
"schema_version": "1.20680.19972"
"schema_version": "1.23292.854"
},
"commitment": {
"circuit_id": "commitment",
"path": "commitment.json",
"artifact_sha256": "0xc664b4c5e4bb06f7d54c4e86fd6a81d42a968899941e6f42d91656ddf9f9a661",
"artifact_sha256": "0x2456b7e6e0162b8b14a8619914f964734cc7c031727c1a52ac49e3f1e1a93472",
"bytecode_sha256": "0xdfbdca2108939b0184db56a381088d4698702988647507242598869290c5d5e0",
"abi_sha256": "0xe084f973bddc1be32518abc050b9f03bcb06ffbef43034b653e9319485a4bb9d",
"name": "commitment",
Expand Down
10 changes: 5 additions & 5 deletions circuits/lib/src/hash/mod.nr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,11 @@ pub fn compute_commitment(nullifier: Field, secret: Field, pool_id: Field) -> Fi
commitment::compute_commitment(nullifier, secret, pool_id)
}

/// Compute nullifier hash bound to a specific root.
/// nullifier_hash = Hash(nullifier, root)
/// This prevents replay attacks across different pool states.
pub fn compute_nullifier_hash(nullifier: Field, root: Field) -> Field {
nullifier::compute_nullifier_hash(nullifier, root)
/// Compute nullifier hash bound to a specific pool_id.
/// nullifier_hash = Hash(nullifier, pool_id)
/// This prevents replay attacks across different pools.
pub fn compute_nullifier_hash(nullifier: Field, pool_id: Field) -> Field {
nullifier::compute_nullifier_hash(nullifier, pool_id)
}

/// Compute parent hash in Merkle tree.
Expand Down
50 changes: 0 additions & 50 deletions circuits/lib/src/validation/fee.nr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,36 +1,7 @@
// ============================================================
// circuits/lib/src/validation/fee.nr (ZK-036)
// ============================================================
// Canonical fee validation rules. These are the single source
// of truth shared between the circuit constraints and the SDK
// witness-preparation layer.
//
// Rule set
// --------
// R1. fee <= amount (prevent fee-only griefing)
// R2. fee == 0 → relayer == 0 (no phantom relayer)
// R3. fee > 0 → relayer != 0 (relayer must be set when fee is non-zero)
//
// All three rules are enforced both here (circuit side) and in
// the SDK (sdk/src/proof.ts – see `validateRelayerFeeEncoding`).
// ============================================================

/// Validate that fee does not exceed the withdrawal amount.
///
/// # Panics
/// Panics if `fee > amount` (both treated as unsigned 64-bit values).
pub fn validate_fee(fee: Field, amount: Field) {
assert(fee as u64 <= amount as u64, "fee cannot exceed withdrawal amount");
}

/// Validate the canonical relayer/fee encoding rule.
///
/// # Encoding contract
/// - fee == 0 → relayer MUST be 0 (no fee, no relayer)
/// - fee > 0 → relayer MUST be != 0 (fee without a relayer is invalid)
///
/// # Panics
/// Panics when either encoding invariant is violated.
pub fn validate_relayer(relayer: Field, fee: Field) {
if fee == 0 {
assert(relayer == 0, "relayer must be zero address if fee is zero");
Expand All @@ -39,21 +10,11 @@ pub fn validate_relayer(relayer: Field, fee: Field) {
}
}

/// Combined convenience guard – call once in the spend circuit.
pub fn validate_fee_and_relayer(fee: Field, amount: Field, relayer: Field) {
validate_fee(fee, amount);
validate_relayer(relayer, fee);
}

// ============================================================
// circuits/lib/src/validation/fee_tests.nr (ZK-036)
// ============================================================
// Noir test module for fee/relayer validation.
// Each test is self-contained and labelled so failures map
// directly to a reproduction case.
// ============================================================

#[cfg(test)]
mod tests {
use super::{validate_fee, validate_relayer, validate_fee_and_relayer};

Expand Down Expand Up @@ -83,14 +44,9 @@ mod tests {

#[test(should_fail_with = "fee cannot exceed withdrawal amount")]
fn fee_equals_max_field_panics_when_amount_is_small() {
// Largest value that fits in u64 – must fail when amount is 0
validate_fee(18446744073709551615, 0);
}

// ------------------------------------------------------------------
// validate_relayer
// ------------------------------------------------------------------

#[test]
fn zero_fee_zero_relayer_is_valid() {
validate_relayer(0, 0);
Expand All @@ -111,19 +67,13 @@ mod tests {
validate_relayer(0, 42);
}

// ------------------------------------------------------------------
// validate_fee_and_relayer (combined)
// ------------------------------------------------------------------

#[test]
fn combined_no_relayer_case() {
// fee=0, relayer=0, amount=1000
validate_fee_and_relayer(0, 1000, 0);
}

#[test]
fn combined_with_relayer_case() {
// fee=5, relayer=<address>, amount=100
validate_fee_and_relayer(5, 100, 0xc0ffee);
}

Expand Down
22 changes: 7 additions & 15 deletions circuits/lib/src/validation/test_helpers.nr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,13 +54,11 @@ pub fn build_valid_fixture() -> (Field, Field, Field, Field, [Field; 20], Field,

let hash_path = hash::zero_sibling_path();
let root = merkle::compute_root(commitment, leaf_index, hash_path);
let nullifier_hash = hash::compute_nullifier_hash(nullifier, root);
let nullifier_hash = hash::compute_nullifier_hash(nullifier, pool_id);

(nullifier, secret, pool_id, leaf_index, hash_path, root, nullifier_hash)
}

/// Setup a complete valid withdrawal with all public parameters.
/// Returns (nullifier, secret, pool_id, leaf_index, hash_path, root, nullifier_hash, recipient, relayer, fee)
pub fn setup_valid_withdrawal() -> (Field, Field, Field, Field, [Field; 20], Field, Field, Field, Field, Field) {
let (nullifier, secret, pool_id, leaf_index, hash_path, root, nullifier_hash) = build_valid_fixture();
let recipient = KAT_RECIPIENT;
Expand All @@ -69,9 +67,6 @@ pub fn setup_valid_withdrawal() -> (Field, Field, Field, Field, [Field; 20], Fie
(nullifier, secret, pool_id, leaf_index, hash_path, root, nullifier_hash, recipient, relayer, fee)
}

/// Build a withdrawal fixture for a leaf at an arbitrary index in a sparse tree.
///
/// Uses canonical zero siblings so any leaf position yields a reproducible root.
pub fn build_fixture_at_index(
nullifier: Field,
secret: Field,
Expand All @@ -81,7 +76,7 @@ pub fn build_fixture_at_index(
let commitment = hash::compute_commitment(nullifier, secret, pool_id);
let hash_path = hash::zero_sibling_path();
let root = merkle::compute_root(commitment, leaf_index, hash_path);
let nullifier_hash = hash::compute_nullifier_hash(nullifier, root);
let nullifier_hash = hash::compute_nullifier_hash(nullifier, pool_id);
(hash_path, root, nullifier_hash)
}

Expand Down Expand Up @@ -161,16 +156,14 @@ pub fn tamper_path(path: [Field; 20], level: Field) -> [Field; 20] {
fn test_build_valid_fixture_is_consistent() {
let (nullifier, secret, pool_id, leaf_index, hash_path, root, nullifier_hash) = build_valid_fixture();

// Re-derive all values and verify consistency
let commitment = hash::compute_commitment(nullifier, secret, pool_id);
let recomputed_root = merkle::compute_root(commitment, leaf_index, hash_path);
let recomputed_nh = hash::compute_nullifier_hash(nullifier, root);
let recomputed_nh = hash::compute_nullifier_hash(nullifier, pool_id);

assert(recomputed_root == root, "fixture root must match recomputed root");
assert(recomputed_nh == nullifier_hash, "fixture nullifier_hash must match recomputed value");
}

/// TC-H-02: build_two_leaf_tree -- both leaves share the same root.
#[test]
fn test_build_two_leaf_tree_shared_root() {
let c1: Field = 0x1111;
Expand All @@ -185,7 +178,6 @@ fn test_build_two_leaf_tree_shared_root() {
assert(root_from_c2 == root, "c2 path must yield the same root");
}

/// TC-H-03: tamper_path -- tampering corrupts the root.
#[test]
fn test_tamper_path_changes_root() {
let leaf: Field = 42;
Expand All @@ -199,19 +191,18 @@ fn test_tamper_path_changes_root() {
assert(honest_root != tampered_root, "tampered path must change the root");
}

/// TC-H-04: build_fixture_at_index with leaf_index=19 (high-bit set only).
#[test]
fn test_build_fixture_at_high_index() {
let nullifier: Field = 0x5555;
let secret: Field = 0x6666;
let pool_id: Field = 0x7777;
let leaf_index: Field = 524288; // bit 19 only: 0b10...0
let leaf_index: Field = 524288;

let (hash_path, root, nullifier_hash) = build_fixture_at_index(nullifier, secret, pool_id, leaf_index);

let commitment = hash::compute_commitment(nullifier, secret, pool_id);
let recomputed_root = merkle::compute_root(commitment, leaf_index, hash_path);
let recomputed_nh = hash::compute_nullifier_hash(nullifier, root);
let recomputed_nh = hash::compute_nullifier_hash(nullifier, pool_id);

assert(recomputed_root == root);
assert(recomputed_nh == nullifier_hash);
Expand Down Expand Up @@ -339,6 +330,7 @@ impl WithdrawalBuilder {

pub fn with_amount(mut self, amount: Field) -> Self {
self.amount = amount;
self.denomination = amount;
self
}

Expand Down Expand Up @@ -395,7 +387,7 @@ impl WithdrawalBuilder {
let nullifier_hash = if self.override_nullifier_hash {
self.nullifier_hash
} else {
hash::compute_nullifier_hash(self.nullifier, root)
hash::compute_nullifier_hash(self.nullifier, self.pool_id)
};

(
Expand Down
Loading