Double-Claim Protection Bypassed — claimed Mapping Checks Uninitialized Immutable Instead of Function Parameter

Root Cause + Impact

The claim() function on line 88 checks claimed[_treasureHash] where _treasureHash is an uninitialized immutable (always bytes32(0)), instead of checking claimed[treasureHash] (the function parameter). The double-claim guard never fires, allowing any single valid proof to be replayed 10 times to drain all 100 ETH.

Description

• Normally, the claimed mapping should prevent the same treasure from being claimed twice. After a successful claim, _markClaimed(treasureHash) sets claimed[treasureHash] = true, and subsequent attempts to claim the same treasure should revert with AlreadyClaimed.

• The guard on line 88 reads claimed[_treasureHash] — the immutable state variable declared on line 35, which is never assigned in the constructor and defaults to bytes32(0). Meanwhile, _markClaimed() on line 104 correctly writes to claimed[treasureHash] (the function parameter). The guard checks a slot that is never written to, and the write goes to a slot that is never checked.

Risk

Likelihood:

• This occurs on every single claim() call — the guard is a no-op by default with no special conditions needed.

• The test testClaimDoubleSpendReverts (line 134-147) has vm.expectRevert() commented out on line 144, silently masking this bug in the test suite.

Impact:

• An attacker with one valid ZK proof calls claim() 10 times sequentially, extracting 10 ETH per call until claimsCount reaches MAX_TREASURES. Total loss: 100 ETH (entire contract balance).

• The core invariant — each treasure can only be claimed once — is completely broken.

Proof of Concept

Attacker preconditions: The attacker only needs to legitimately find one treasure and generate a single valid Noir proof for it. No elevated privileges, no compromised keys, no reentrancy tricks — a normal participant-level account is sufficient.

Exploit walkthrough:

1. First call to claim(proof, treasureHash, recipient): the ZK proof passes verifier.verify(), _markClaimed() sets claimed[treasureHash] = true, and recipient receives 10 ETH.

2. Second call with the exact same (proof, treasureHash, recipient) tuple:

   • Line 88 reads claimed[_treasureHash] — but _treasureHash is the uninitialized immutable declared on line 35, which permanently equals bytes32(0).

   • claimed[bytes32(0)] has never been written to anywhere in the contract, so it returns false.

   • The AlreadyClaimed guard does not fire.

3. verifier.verify(proof, publicInputs) returns true again — the verifier only checks that the proof is cryptographically valid, not that it has been used before. ZK proof systems have no built-in replay protection; that is the contract's responsibility, and the contract's check is looking at the wrong slot.

4. _markClaimed(treasureHash) writes claimed[treasureHash] = true a second time — but line 88 never reads this slot, so the write is effectively a no-op for guard purposes.

5. Steps 2–4 repeat for 8 more iterations. On the 10th successful call, claimsCount == MAX_TREASURES == 10, and only then does AllTreasuresClaimed finally revert.

Result: the attacker extracts 10 ETH per iteration × 10 iterations = 100 ETH (the full contract balance) using a single valid proof. The core protocol invariant — "each treasure can be claimed at most once" — is completely broken, and every other participant loses their reward.

The following Foundry test reproduces the full drain end-to-end:

Note on existing test coverage: the test suite already contains testClaimDoubleSpendReverts (contracts/test/TreasureHunt.t.sol, lines 134–147), which was intended to catch exactly this regression. However, the critical vm.expectRevert() assertion on line 144 is commented out, silently masking the bug and allowing CI to pass against a vulnerable contract.

Recommended Mitigation

Change line 88 to use the function parameter instead of the immutable:

Remove the unused _treasureHash immutable on line 35 to prevent future naming confusion.