SNARKeling Treasure Hunt
First Flight #59
Beginner FriendlyGameFiFoundry
100 EXP
View results
Previous Next
Submission Details
Severity: high
Valid

H-01 Broken double-claim protection in claim()

dechn

Description

The claim() function is intended to prevent the same treasure from being claimed more than once through the claimed mapping. However, the current implementation checks claimed[_treasureHash] instead of claimed[treasureHash].

Since _treasureHash is declared as an immutable state variable but never initialized, the duplicate-claim check does not operate on the caller-supplied treasure identifier. As a result, the contract does not correctly enforce the one-claim-per-treasure invariant.

Risk

This issue directly affects the reward-distribution logic. A valid treasure may be claimed repeatedly, limited only by:

  • remaining contract balance

  • claimsCount < MAX_TREASURES

This can lead to repeated reward payouts for the same treasure.

Proof of Concept

An attack path is straightforward:

  1. Obtain one valid proof for a legitimate treasureHash.

  2. Call claim(proof, treasureHash, recipient) successfully.

  3. The contract sets claimed[treasureHash] = true, but future calls still check claimed[_treasureHash].

  4. Re-submit the same valid claim until the contract reaches MAX_TREASURES or runs out of funds.

The relevant flawed line is:

if (claimed[_treasureHash]) revert AlreadyClaimed(treasureHash);

Recommended Mitigation

Update the duplicate-claim check so that it uses the function argument treasureHash, and remove the unused _treasureHash state variable.

diff --git a/contracts/src/TreasureHunt.sol b/contracts/src/TreasureHunt.sol
--- a/contracts/src/TreasureHunt.sol
+++ b/contracts/src/TreasureHunt.sol
@@ -32,7 +32,6 @@ contract TreasureHunt {
// ----- immutable config -----
IVerifier private verifier;
address private immutable owner;
- bytes32 private immutable _treasureHash;
@@ -85,7 +84,7 @@ contract TreasureHunt {
if (paused) revert ContractPaused();
if (address(this).balance < REWARD) revert NotEnoughFunds();
if (recipient == address(0) || recipient == address(this) || recipient == owner || recipient == msg.sender) revert InvalidRecipient();
if (claimsCount >= MAX_TREASURES) revert AllTreasuresClaimed();
- if (claimed[_treasureHash]) revert AlreadyClaimed(treasureHash);
+ if (claimed[treasureHash]) revert AlreadyClaimed(treasureHash);
if (msg.sender == owner) revert OwnerCannotClaim();
Updates

Lead Judging Commences

s3mvl4d Lead Judge 18 days ago
Submission Judgement Published
Validated
Assigned finding tags:

broken double-claim protection

In `claim()`, the guard uses `claimed[_treasureHash]`, where `_treasureHash` is an immutable state variable that is never initialized to the caller-supplied treasure identifier, while the contract later marks `claimed[treasureHash] = true` using the function argument instead. As a result, the duplicate-claim check and the state update are performed against different keys, which means a previously claimed treasure is not actually blocked from being claimed again with the same valid proof and `treasureHash`. This breaks a core invariant of the protocol described in the README, namely, that each treasure can only be redeemed once, and allows one valid treasure/proof pair to be reused to drain rewards repeatedly until either the `MAX_TREASURES` cap or the contract balance is exhausted.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!