Skip to content

fix(privacy): bind open-note deposits to note ids created in the same batch - #935

Open
avi-starkware wants to merge 1 commit into
mainfrom
avi/privacy-open-note-deposit-identity
Open

fix(privacy): bind open-note deposits to note ids created in the same batch#935
avi-starkware wants to merge 1 commit into
mainfrom
avi/privacy-open-note-deposit-identity

Conversation

@avi-starkware

@avi-starkware avi-starkware commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

What

Replaces the scalar undeposited_open_notes counter in _apply_actions with a batch-local list of emitted note ids, and requires every returned OpenNoteDeposit to name one of them.

Why

The counter proved only that the number of EmitOpenNoteCreated actions equalled the number of deposits — not that each deposit funded a note the batch actually created. _deposit_to_open_note validates that the target note exists, carries OPEN_NOTE_SALT, is unfunded and has a matching token, but nothing tied it to the current batch.

So a deposit naming an unfunded open note from outside the batch satisfied the invariant while the note the batch had just emitted stayed unfunded. The out-of-batch target is then permanently marked funded (current_amount != 0), so the deposit it was actually waiting for can never land — and note-index sequencing means that id cannot be recreated.

Worth being precise about reachability: the old code already enforced #deposits == #creations per transaction, and a deposit can only target an unfunded note, so by induction apply_actions never leaves an unfunded open note in storage. The state this needs is therefore not reachable through apply_actions on the current class — it requires a legacy note left by a contract replacement. This is hardening of an invariant that was previously only guaranteed by a non-local induction argument, not a fix for a live exploit path.

How

  • EmitOpenNoteCreated appends the note id already present in the event payload — no recomputation.
  • _apply_invoke_and_deposits takes ref undeposited_open_note_ids: Array<felt252>.
  • New utils::consume_undeposited_open_note removes exactly one matching id, or reverts with the new OPEN_NOTE_NOT_CREATED_IN_TX. Cairo arrays are append-only, so removal rebuilds the array minus the first match rather than swap-removing; the one-per-call property is what matters.
  • Final assertion becomes undeposited_open_note_ids.is_empty(), keeping UNDEPOSITED_OPEN_NOTES.
  • internal_errors::TOO_MANY_OPEN_NOTES_DEPOSITED is removed — the membership check now fires first in every case that previously underflowed the counter. No references anywhere in the repo.
  • interface.cairo documents the new precondition and both reverts.

Deposits may still arrive in any order relative to their creations; only identity is enforced.

Behavior change for callers

A deposit naming a note outside the current batch now reverts with OPEN_NOTE_NOT_CREATED_IN_TX instead of NOTE_NOT_FOUND / NOTE_NOT_OPEN / NOTE_ALREADY_DEPOSITED, because the membership check runs before _deposit_to_open_note. Five existing tests asserted those errors while deliberately omitting the same-batch creation; each now passes a bare EmitOpenNoteCreated for the deposit's id so it still reaches its original assertion, keeping coverage of all four errors.

Tests

  • test_open_note_deposit_rejects_note_created_in_another_tx — the core case, driven through the real client compile path.
  • test_open_note_deposits_matched_out_of_creation_order — two creations, deposits returned reversed, both funded. Proves identity matching, not positional.
  • test_open_note_deposit_rejects_double_consumption[X, X] for one creation reverts, proving removal happened.
  • test_undeposited_open_notes extended with "2 creations, 1 deposit".

Verified the new test has teeth: with the identity check reduced to the old count-only behavior the attack test passes (the out-of-batch note is written funded from the invoked contract's tokens); with the check in place it reverts.

Verification

scarb fmt --check   exit 0
scarb build         OK
snforge test        306 passed, 0 failed, 3 ignored

The 3 ignored are pre-existing #[ignore]s in tests/generate_reference_data.cairo.

Gas cost of the added scan: +0.03% l2_gas on open-note tests (e.g. test_open_note_multiple_depositors 96,348,011 to 96,376,651); l1_gas and l1_data_gas unchanged.

Note

packages/privacy/README.md documents a standalone deposit_to_open_note() entrypoint with a "caller must be the specified depositor" rule that does not exist in the contract. Pre-existing doc drift, left alone as separable.


This change is Reviewable

@avi-starkware
avi-starkware force-pushed the avi/privacy-open-note-deposit-identity branch 4 times, most recently from 91c6b84 to c2e7fb5 Compare August 10, 2026 12:14
@avi-starkware
avi-starkware force-pushed the avi/privacy-open-note-deposit-identity branch from c2e7fb5 to 305afdd Compare August 11, 2026 15:52
… batch

`_apply_actions` reconciled creation against funding with a counter, which proves
only that the two counts match. A returned deposit could name an unfunded open
note from an earlier batch while the note this batch emitted stayed unfunded,
leaving the target marked funded so its intended deposit can never land.

Track the emitted note ids instead and require each `OpenNoteDeposit` to name one,
removing the match on use so a single creation cannot absorb two deposits.
Deposits may still arrive in any order relative to their creations.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@avi-starkware
avi-starkware force-pushed the avi/privacy-open-note-deposit-identity branch from 305afdd to 9cd6528 Compare August 17, 2026 07:40
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