fix(token): resolve zero-value revert audit findings (L-01, L-02)

[0xisk]

Types of changes

What types of changes does your code introduce to OpenZeppelin Midnight Contracts?
Put an x in the boxes that apply

• Bugfix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Documentation Update (if none of the other choices apply)

Fixes #425
Fixes #426

Resolves the two audit findings (L-01, L-02) that were parked behind the
unconditional-downcast compiler limitation
LFDT-Minokawa/compact#226,
now resolved upstream. With conditional downcasts available, a zero-value
balance/allowance update can be guarded without the constraint-level failure
the audit comments described.

L-01 — MultiToken._update (#425)

A zero-value transferFrom, _transfer, _mint or _burn of a token id
that was never initialized reverted on the bare _balances.lookup(id) write
inside _update, rather than failing with an explicit assertion or succeeding
as a no-op.

Fix: guard both balance writes on disclose(value > 0). A zero-value
update leaves balances unchanged, so the source write is skipped and the
recipient block no longer initializes the id. A non-zero transfer/burn of an
uninitialized id still fails with the explicit "MultiToken: insufficient balance" assertion as before.

L-02 — FungibleToken._spendAllowance (#426)

transferFrom(owner, to, 0) reverted unless a prior approval had created the
_allowances entry, because _spendAllowance asserted membership before
reading the allowance. This diverged from allowance, which treats a missing
entry as zero.

Fix: read the current allowance via allowance (missing entry → 0) and
guard the spend on disclose(value > 0). A zero-value spend is now a no-op that
neither reverts nor writes a spurious zero entry; a non-zero spend without
allowance still fails with "FungibleToken: insufficient allowance".

On disclose(value > 0)

The guard gates a ledger write, so its boolean must be disclosed. value is
already public on every balance write (and a non-zero spend already discloses
the updated allowance), so disclosing whether value is zero leaks nothing
new.

Note on @circuitInfo

The touched circuits' constraint counts shift by a few rows. The @circuitInfo
annotations are intentionally left unchanged here: the existing values do not
match the toolchain in this environment even for circuits this PR does not touch
(e.g. balanceOf), so they are best regenerated wholesale with the canonical
release toolchain rather than edited piecemeal in this fix.

PR Checklist

• I have read the Contributing Guide
• I have added tests that prove my fix is effective or that my feature works. See GUIDELINES.md#testing for more information.
• I have added documentation for new methods or changes to existing method behavior. See GUIDELINES.md#documentation for more information.
• CI Workflows Are Passing

Further comments

Both fixes compile to valid ZKIR (prover/verifier keys generate for every mock
circuit), confirming the conditional downcast + disclosure is provable. Added
tests cover: zero-value transferFrom / _transfer / _mint / _burn on an
uninitialized id (MultiToken), and zero-value transferFrom / _spendAllowance
without a pre-existing allowance plus an existing-allowance-unchanged case
(FungibleToken). All 554 token suite tests pass.

Summary by CodeRabbit

Bug Fixes

• Token transfers with zero amounts no longer fail when allowance entries don't exist; missing allowances are treated as zero.
• Zero-value balance operations no longer create unnecessary state entries.

Tests

• Added comprehensive test coverage for zero-amount token operations with missing allowance entries.
• Extended tests for zero-value transfers, mints, and burns on uninitialized tokens.

[coderabbitai]

Apologies for the duplicate attempts above — let me produce the correct, single final answer now.

---

I need to use only the provided rangeIds. Let me redo this cleanly with only the exact IDs from <all_range_ids>.

The range_7acbb20a40b0 id in <all_range_ids> does not exactly match range_7acbb2**0**a40b0 — let me use range_7acbb20a40b0 precisely as given.

Walkthrough

_spendAllowance in FungibleToken now treats zero-value spends as a no-op and reads missing allowance map entries as zero using the allowance() circuit. _update in MultiToken guards both balance decrement and increment paths behind value > 0 checks. Tests are added for both contracts covering these zero-value scenarios.

Changes

Zero-value operation guards and tests

Layer / File(s)	Summary
_spendAllowance and _update zero-value guards contracts/src/token/FungibleToken.compact, contracts/src/token/MultiToken.compact	_spendAllowance returns early when value is zero and uses allowance() to treat missing map entries as zero rather than asserting membership. _update wraps both the from-side balance decrement and the to-side increment with value > 0 guards so uninitialized token IDs are never accessed.
Zero-value test coverage contracts/src/token/test/FungibleToken.test.ts, contracts/src/token/test/MultiToken.test.ts	FungibleToken tests assert zero-value transferFrom succeeds without a prior allowance entry and _spendAllowance(0n) leaves state unchanged for both missing and existing entries. MultiToken tests assert transferFrom, _transfer, _mint, and _burn with 0n on NONEXISTENT_ID do not revert and produce 0n balances.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐇 A hop, a skip, a zero to spend,
No map entry needed — the guards defend!
value == 0? We skip the write,
Uninitialized IDs stay tucked in tight.
The bunny cheers: no spurious revert in sight! 🌿

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and specifically summarizes the main changes: fixing two audit findings (L-01 and L-02) related to zero-value operations in token contracts.
Linked Issues check	✅ Passed	The PR successfully addresses both linked issues: L-01 guards balance writes with value>0 to skip zero-value updates, and L-02 reads allowances via public function and gates spends on value>0 for consistency.
Out of Scope Changes check	✅ Passed	All changes are scoped to the two audit findings: modifications to FungibleToken._spendAllowance and MultiToken._update, plus corresponding tests validating zero-value operation behavior.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/token-zero-value-reverts

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

contracts/src/token/test/MultiToken.test.ts (1)

377-388: ⚡ Quick win

Assert that NONEXISTENT_ID remains uninitialized after zero-value ops.

These tests verify zero balances, but not the stronger audit objective that zero-value paths do not initialize the id entry. Please add a direct storage-membership assertion (via simulator/private state) after each case.

Also applies to: 933-939, 1171-1176, 1303-1307

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@contracts/src/token/test/MultiToken.test.ts` around lines 377 - 388, The test
for "should allow transfer of 0 tokens for an uninitialized id" verifies that
balances remain zero but does not directly assert that the NONEXISTENT_ID entry
remains uninitialized in storage. Add storage-membership assertions (via the
simulator's private state inspection) after the transferFrom call to directly
verify that the NONEXISTENT_ID entry was not initialized in storage, confirming
that zero-value operations do not create storage entries. Apply the same fix to
all other zero-value transfer test cases referenced (the similar test blocks
around lines 933-939, 1171-1176, and 1303-1307).

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@contracts/src/token/test/MultiToken.test.ts`:
- Around line 377-388: The test for "should allow transfer of 0 tokens for an
uninitialized id" verifies that balances remain zero but does not directly
assert that the NONEXISTENT_ID entry remains uninitialized in storage. Add
storage-membership assertions (via the simulator's private state inspection)
after the transferFrom call to directly verify that the NONEXISTENT_ID entry was
not initialized in storage, confirming that zero-value operations do not create
storage entries. Apply the same fix to all other zero-value transfer test cases
referenced (the similar test blocks around lines 933-939, 1171-1176, and
1303-1307).

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 6e480aee-3c5e-40d2-a771-181857c0c0ef

📥 Commits

Reviewing files that changed from the base of the PR and between cb74178 and 070f630.

📒 Files selected for processing (4)

• contracts/src/token/FungibleToken.compact
• contracts/src/token/MultiToken.compact
• contracts/src/token/test/FungibleToken.test.ts
• contracts/src/token/test/MultiToken.test.ts