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

[H-1] Typo in claim() checks uninitialized _treasureHash instead of treasureHash argument, allowing an attacker to bypass the claim check and drain the contract

moumm

Root + Impact

Description

The claim() function is designed to verify a ZK proof and dispense a 10 ETH reward for finding a treasure. To prevent double-claiming, it checks if the treasure has already been claimed. However, a typographical error causes the function to check the claim status of an uninitialized state variable (_treasureHash) instead of the user-provided argument (treasureHash).

Because _treasureHash is never initialized in the constructor, it defaults to bytes32(0). When a user calls claim(), the check evaluates claimed[bytes32(0)], which will always pass. Later in the function, the contract correctly marks the user-provided treasureHash as claimed. Because the check and the state update use different variables, the state update has no effect on future checks. An attacker can submit the exact same valid proof and treasureHash multiple times, bypassing the check and draining the contract.

function claim(bytes calldata proof, bytes32 treasureHash, address payable recipient) external nonReentrant() {
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();
// @> Root cause: checks uninitialized state variable `_treasureHash` instead of argument `treasureHash`
if (claimed[_treasureHash]) revert AlreadyClaimed(treasureHash);
if (msg.sender == owner) revert OwnerCannotClaim();
​
// ... proof verification ...
​
_incrementClaimsCount();
// @> State update uses the correct variable `treasureHash`, so the check above is never triggered
_markClaimed(treasureHash);
​
(bool sent, ) = recipient.call{value: REWARD}("");
require(sent, "ETH_TRANSFER_FAILED");
emit Claimed(treasureHash, msg.sender);
}

Risk

Likelihood:

  • Any user who legitimately finds a single treasure (or obtains a valid proof from the mempool) can execute this attack.

  • The vulnerability requires no special preconditions other than possessing one valid ZK proof.

Impact:

  • The attacker can repeatedly call claim() in a loop until the contract is completely drained of all ETH.

  • The intended MAX_TREASURES limit (10) will be reached, but the attacker will receive all 100 ETH instead of the intended 10 ETH per treasure, stealing funds meant for other participants.

Proof of Concept

The vulnerability requires no special setup. Any user who possesses a single valid ZK proof can call claim() repeatedly with the same proof and treasureHash. Each call passes the claim check because it evaluates claimed[bytes32(0)] (the uninitialized _treasureHash variable), which is always false. The attacker receives 10 ETH per call and can loop until the contract is fully drained.
​
// 1. The contract is deployed and funded with 100 ETH.
// 2. Alice finds a treasure and generates a valid ZK proof for `treasureHashA`.
// 3. Alice calls `claim(proof, treasureHashA, aliceAddress)`.
// - Line 94 checks `claimed[bytes32(0)]` -> `false`.
// - Proof verifies successfully.
// - `claimed[treasureHashA]` is set to `true`.
// - Alice receives 10 ETH.
// 4. Alice immediately calls `claim(proof, treasureHashA, aliceAddress)` again.
// - Line 94 checks `claimed[bytes32(0)]` -> `false` (bypass successful).
// - Proof verifies successfully (it is still valid for `treasureHashA`).
// - `claimed[treasureHashA]` is set to `true` again.
// - Alice receives another 10 ETH.
// 5. Alice repeats this 8 more times in the same transaction or subsequent blocks, draining the remaining 80 ETH.

Recommended Mitigation

The fix is a single character change on line 94. Replace the uninitialized state variable _treasureHash with the function argument treasureHash so the claim check correctly tracks which treasures have already been claimed. This ensures that once a given treasureHash is claimed, subsequent calls with the same hash will revert.
​
- if (claimed[_treasureHash]) revert AlreadyClaimed(treasureHash);
+ if (claimed[treasureHash]) revert AlreadyClaimed(treasureHash);
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!