Skip to content

fix: correct auditor-key claims in THREAT_MODEL.md and circuit comments - #134

Open
tech-adrian wants to merge 1 commit into
devfrom
fix/auditor-scoping-114-followup
Open

fix: correct auditor-key claims in THREAT_MODEL.md and circuit comments#134
tech-adrian wants to merge 1 commit into
devfrom
fix/auditor-scoping-114-followup

Conversation

@tech-adrian

Copy link
Copy Markdown
Contributor

Summary

#133's threat model listed "auditor scoping" as an enforced property of the compliance and disclosure circuits, and both circuits carried a comment claiming a Poseidon2 hash provides that binding. Neither claim was accurate as stated:

  • The hash (_disclosure_commitment / _binding) is never asserted against anything — it's dead code. I verified removing it changes the compiled bytecode (auditor_key becomes an unused parameter and the artifact hash changes), so it stays in place rather than being deleted. Only the comment claiming it does cryptographic work was wrong.
  • The real anti-redirect property — a proof can't be silently retargeted to a different auditor_key after generation — comes from auditor_key being a pub argument to main. Any public input is bound into what the proof verifies against; that's independent of this specific hash.
  • Neither the circuit nor the compliance contract checks that auditor_key corresponds to a real, registered auditor. There's no auditor registry anywhere in contracts/compliance/src/lib.rs; a prover may set it to any Field value.

Changes

  • circuits/compliance/src/main.nr, circuits/disclosure/src/main.nr: corrected comments to state the actual mechanism; renamed the dead value from _disclosure_commitment/_binding to _auditor_key_referenced so it doesn't imply a security property it doesn't have.
  • docs/THREAT_MODEL.md:
    • New "Auditor-key binding" section explaining what's actually enforced vs. assumed.
    • Moved the registry gap from the enforced-properties column into External assumptions for both circuit rows (matching the doc's existing pattern for other unenforced properties).
    • Added a bullet under Residual assumptions pointing at bug(critical): deposit note secrets can be permanently lost if the tab closes before confirmation #63 — the existing "users protect their own secrets" line glossed over a real app-level bug where a closed tab during deposit confirmation can strand a note with no recoverable secret.

Validation

  • bb write_vk against the recompiled bytecode for both circuits produces byte-identical VKs to the currently-committed ones — confirmed by diffing the regenerated vk files against circuits/{compliance,disclosure}/target/vk. This is a comment/doc-only fix cryptographically; no proof fixtures, deployed VKs, or frontend circuit artifacts need to change.
  • cargo test --workspace: 145 passed, 0 failed.
  • pnpm test: 140 passed, 0 failed.

Test plan

  • Both circuits compile cleanly
  • VK bytes unchanged (diffed against committed artifacts)
  • Contract test suite passes unchanged
  • Frontend test suite passes unchanged

#133's threat model listed "auditor scoping" as an enforced property of
the compliance and disclosure circuits, and both circuits carried a
comment claiming a Poseidon2 hash provides "auditor-scoped binding, so
a disclosure produced for one auditor cannot be replayed as though it
were addressed to another."

Neither claim was accurate as stated:

- The hash (`_disclosure_commitment` / `_binding`) is never asserted
  against anything -- it's dead. Removing it changes the compiled
  bytecode (verified: `auditor_key` becomes an unused parameter and
  the artifact hash changes), so it stays in place rather than being
  deleted; only the comment claiming it does work was wrong.
- The anti-redirect property the comment describes is real, but it
  comes from `auditor_key` being a `pub` argument -- any public input
  is bound into the proof's verification, independent of this hash --
  not from the hash itself.
- Neither the circuit nor the compliance contract checks that
  `auditor_key` corresponds to a real, registered auditor. There is no
  auditor registry; a prover may set it to any Field value.

Corrected both circuit comments to state the actual mechanism, and
renamed the dead value so it doesn't imply a security property. Added
an "Auditor-key binding" section to THREAT_MODEL.md, moved the registry
gap into External assumptions for both circuit rows (matching the
pattern the doc already used elsewhere), and added a bullet to
Residual assumptions pointing at #63 -- the doc's "users protect their
own secrets" assumption currently glosses over a real app-level bug
where a closed tab during deposit confirmation can strand a note with
no recoverable secret.

Verified the fix is a no-op cryptographically: bb write_vk against the
recompiled bytecode produces byte-identical VKs to the committed ones
for both circuits, so no proof fixtures, deployed VKs, or frontend
circuit artifacts need to change. 145 contract tests and 140 frontend
tests pass unchanged.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant