diff --git a/contracts/sharibo/src/lib.rs b/contracts/sharibo/src/lib.rs index b9236da..e4971d6 100644 --- a/contracts/sharibo/src/lib.rs +++ b/contracts/sharibo/src/lib.rs @@ -136,6 +136,10 @@ pub struct Circle { /// Funding is unshielded (addresses are already public), so storing /// them here imposes no additional privacy loss — see issue #82. pub contributors: Vec
, + /// Nullifier hashes used in successful claims for this circle. + /// Embedded inside the Circle persistent entry so they inherit the + /// continuously-extended TTL lifecycle of the circle itself (issue #254). + pub nullifiers: Vec, /// True once `cancel_circle` has been called; prevents any further /// `fund` or `claim` calls so the circle is permanently closed. pub cancelled: bool, @@ -323,6 +327,7 @@ impl Contract { pot: 0, vk, contributors: Vec::new(&env), + nullifiers: Vec::new(&env), cancelled: false, round_deadline_ledgers, round_started_ledger, @@ -509,8 +514,7 @@ impl Contract { } // 3. this nullifier must not have claimed before (any round, this circle) - let nullifier_key = DataKey::Nullifier(circle_id, nullifier_hash.clone()); - if env.storage().persistent().has(&nullifier_key) { + if circle.nullifiers.contains(&nullifier_hash) { panic_with_error!(&env, Error::AlreadyClaimed); } @@ -533,11 +537,6 @@ impl Contract { } // effects - env.storage().persistent().set(&nullifier_key, &true); - env.storage() - .persistent() - .extend_ttl(&nullifier_key, LEDGER_THRESHOLD, LEDGER_EXTEND_TO); - let token_client = token::Client::new(&env, &circle.token); token_client.transfer(&env.current_contract_address(), &recipient, &circle.pot); @@ -545,6 +544,7 @@ impl Contract { circle.round += 1; circle.contributors = Vec::new(&env); circle.round_started_ledger = env.ledger().sequence(); + circle.nullifiers.push_back(nullifier_hash); env.storage().persistent().set(&key, &circle); env.storage() .persistent() @@ -681,9 +681,12 @@ impl Contract { /// `true` if the nullifier has ever been used in a successful claim for /// this circle (any round); the associated identity cannot claim again. pub fn has_claimed(env: Env, circle_id: u64, nullifier_hash: Fr) -> bool { - env.storage() - .persistent() - .has(&DataKey::Nullifier(circle_id, nullifier_hash)) + let key = DataKey::Circle(circle_id); + if let Some(circle) = env.storage().persistent().get::<_, Circle>(&key) { + circle.nullifiers.contains(&nullifier_hash) + } else { + false + } } /// Step 1 of two-step admin transfer: the current admin nominates a diff --git a/contracts/sharibo/src/test.rs b/contracts/sharibo/src/test.rs index 0999c20..7884bc9 100644 --- a/contracts/sharibo/src/test.rs +++ b/contracts/sharibo/src/test.rs @@ -1308,6 +1308,52 @@ fn instance_ttl_extended_after_create_fund_claim() { assert_eq!(circle.round, 1, "claim should have advanced round to 1"); } +#[test] +fn nullifier_fence_survives_ttl_expiry() { + // Regression test for issue #254: + // Nullifier storage entries embedded inside the Circle struct inherit the + // Circle's continuously-extended TTL. When the ledger advances past the + // initial extend_to period, re-extending the Circle entry ensures that + // stored nullifiers are preserved and cannot be bypassed. + let s = setup(5, 100); + let client = ContractClient::new(&s.env, &s.client_id); + + for m in s.members.iter() { + client.fund(&s.circle_id, m); + } + + let recipient = Address::generate(&s.env); + let nullifier_hash = real_nullifier_hash(&s.env); + let external_nullifier = real_external_nullifier_round0(&s.env); + let proof = real_valid_proof(&s.env); + + client.claim( + &s.circle_id, + &recipient, + &nullifier_hash, + &external_nullifier, + &proof, + ); + + assert!(client.has_claimed(&s.circle_id, &nullifier_hash)); + + // Advance the ledger sequence past LEDGER_THRESHOLD. + s.env.ledger().with_mut(|l| { + l.sequence_number += LEDGER_THRESHOLD + 10; + l.timestamp += u64::from(LEDGER_THRESHOLD + 10) * 5; + }); + + // Re-funding for round 1 extends the Circle entry TTL. + let token_admin_client = token::StellarAssetClient::new(&s.env, &s.token); + for m in s.members.iter() { + token_admin_client.mint(m, &s.contribution); + client.fund(&s.circle_id, m); + } + + // Verify nullifier fence is still intact after ledger advancement and Circle TTL extension. + assert!(client.has_claimed(&s.circle_id, &nullifier_hash)); +} + // ---- Proptest: apply_fee invariants ---- // // Two sub-suites: diff --git a/docs/adr/004-storage-archival.md b/docs/adr/004-storage-archival.md new file mode 100644 index 0000000..f29e7ee --- /dev/null +++ b/docs/adr/004-storage-archival.md @@ -0,0 +1,50 @@ +# ADR 004: Storage Archival and Nullifier Lifetime + +- **Status:** Accepted +- **Date:** 2026-08-31 +- **Context:** Design for issue #254 (Nullifier entries can be archived — a claim could be replayed after TTL expiry). + +## Context + +In Soroban, persistent storage entries are archived when their TTL lapses. + +Previously, the double-claim fence in `Contract::claim` relied on standalone persistent storage entries: +```rust +let nullifier_key = DataKey::Nullifier(circle_id, nullifier_hash.clone()); +if env.storage().persistent().has(&nullifier_key) { + panic_with_error!(&env, Error::AlreadyClaimed); +} +``` + +Soroban persistent entries have their TTL extended at write time to `LEDGER_EXTEND_TO = 500_000` ledgers (~1 month at 5s/ledger). Nullifiers are write-once per claim and were never accessed or extended again. + +In contrast, the `Circle` entry (`DataKey::Circle(circle_id)`) is continuously re-extended on every `fund` and `claim` call. If a circle remains active or is extended, but individual `DataKey::Nullifier` entries lapse and are archived, `env.storage().persistent().has(&nullifier_key)` would return `false`. This opened a vulnerability where a member could replay a previously used claim/nullifier in a subsequent round after its TTL expired. + +## Options Analyzed + +### Option (a) — Re-extend every circle's nullifier TTLs on each claim +- On every claim, iterate through all previously stored nullifiers for the circle and extend their TTLs. +- **Drawback**: Unbounded storage lookups and CPU/TTL extension work as the number of claims grows; fails to scale. + +### Option (b) — Store nullifiers as a bounded `Vec` inside the `Circle` struct (Chosen) +- Add `pub nullifiers: Vec` to `pub struct Circle`. +- Read and check `circle.nullifiers.contains(&nullifier_hash)` directly within `claim` and `has_claimed`. +- Store nullifiers directly inside `Circle` at `DataKey::Circle(circle_id)`. +- **Advantages**: + - Nullifiers inherit the `Circle` entry's continuously-extended TTL lifecycle. As long as the `Circle` entry is live or re-extended, all nullifiers registered for that circle remain live. + - For a fixed-size ROSCA, the number of nullifiers in a circle is bounded by design (`size` members per cycle). + - In-memory `Vec::contains()` on a small `soroban_sdk::Vec` is O(N) where N ≤ circle size, which is cheap and fits comfortably within CPU instruction limits. + +### Option (c) — Declare circles time-bounded +- Require circles to complete within the 500,000 ledger TTL window and accept replay risks for expired circles. +- **Drawback**: Leaves residual vulnerability if a circle spans more than ~1 month. + +## Decision + +Adopt **Option (b)**. Nullifiers are embedded within `Circle.nullifiers`. + +## Consequences + +- Nullifiers never archive independently of the `Circle` entry. +- Double-claim fences survive arbitrary ledger advancement as long as the circle exists. +- State size per `Circle` increases by `32 * N` bytes, naturally bounded by circle size. diff --git a/docs/threat-model.md b/docs/threat-model.md index 96c2f24..edc8279 100644 --- a/docs/threat-model.md +++ b/docs/threat-model.md @@ -37,13 +37,14 @@ Scope: `contracts/sharibo/src/lib.rs`, `circuits/membership.template.circom`, `p ### 2. No double claim — one payout per nullifier -**Mechanism.** `nullifierHash = Poseidon(identityNullifier, externalNullifier)` is emitted as a circuit output (`circuits/membership.template.circom:113-116`). The contract records it under `DataKey::Nullifier(circle_id, nullifier_hash)` and rejects any claim that reuses one (`lib.rs:213-217`, marked used at `:231`), independent of the pairing check. +**Mechanism.** `nullifierHash = Poseidon(identityNullifier, externalNullifier)` is emitted as a circuit output (`circuits/membership.template.circom:113-116`). The contract records it in `Circle.nullifiers` (`lib.rs`) and rejects any claim that reuses one, independent of the pairing check. Storing nullifiers inside the `Circle` persistent entry ensures nullifiers share the circle's continuously-extended TTL lifecycle and cannot be archived independently (see `docs/adr/004-storage-archival.md`). -**Tests.** `contracts/sharibo/src/test.rs:284` (`second_claim_with_same_nullifier_reverts`), `:548` (`has_claimed_false_before_true_after`). +**Tests.** `contracts/sharibo/src/test.rs` (`second_claim_with_same_nullifier_reverts`, `has_claimed_false_before_true_after`, `nullifier_fence_survives_ttl_expiry`). **Limits.** -- `recipient` is a plain contract argument (`lib.rs:186`), not a circuit input — it is never bound to the proof (compare the circuit's signal list, `circuits/membership.template.circom:79-96`, which has no recipient signal at all; `circuits/test/membership.test.js:182` documents that the circuit does not bind the proof to any single expected value beyond what the verifier separately checks). Concretely: `(nullifier_hash, external_nullifier, proof)` is a valid claim for *any* `recipient`. If those values become visible before the original claim transaction is finalized — a careless or malicious relayer, or another party observing the pending transaction — anyone can resubmit the same tuple with a different `recipient` and redirect the payout. This is a payout-hijacking risk, not a privacy break: it costs the original claimant their payout, it does not deanonymize them. It's the same class of risk Tornado-Cash-style relayer designs address by adding the recipient (and a relayer fee) as a public input the circuit itself commits to; Sharibo does not do this today. -- In the demo, `claim` is always submitted through the admin's client (`scripts/e2e.ts:201`, `app/src/App.tsx:424,464`), which is exactly the delivery path where the above risk would surface first — the admin (or whoever holds that key) sees the proof and recipient before broadcasting and is a required relayer for the demo flow, even though `claim` itself has no `require_auth` call (`lib.rs:183-249`) and would accept submission from any account. +- `recipient` is a plain contract argument (`lib.rs`), not a circuit input — it is never bound to the proof (compare the circuit's signal list, `circuits/membership.template.circom:79-96`, which has no recipient signal at all; `circuits/test/membership.test.js:182` documents that the circuit does not bind the proof to any single expected value beyond what the verifier separately checks). Concretely: `(nullifier_hash, external_nullifier, proof)` is a valid claim for *any* `recipient`. If those values become visible before the original claim transaction is finalized — a careless or malicious relayer, or another party observing the pending transaction — anyone can resubmit the same tuple with a different `recipient` and redirect the payout. This is a payout-hijacking risk, not a privacy break: it costs the original claimant their payout, it does not deanonymize them. It's the same class of risk Tornado-Cash-style relayer designs address by adding the recipient (and a relayer fee) as a public input the circuit itself commits to; Sharibo does not do this today. +- In the demo, `claim` is always submitted through the admin's client (`scripts/e2e.ts:201`, `app/src/App.tsx:424,464`), which is exactly the delivery path where the above risk would surface first — the admin (or whoever holds that key) sees the proof and recipient before broadcasting and is a required relayer for the demo flow, even though `claim` itself has no `require_auth` call (`lib.rs`) and would accept submission from any account. +- Storage footprint per circle grows by `32 * N` bytes, bounded by the circle's size `N`. ### 3. Round binding — a proof for round *N* cannot be used in round *N+1*