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

Incorrect `claimed` Status Check Allows Repeated Claims of the Same treasureHash and Drains the Contract

victifa

Root + Impact

Description

In the claim function, the contract uses an uninitialized immutable variable _treasureHash to check whether a treasure has already been claimed:

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

Since _treasureHash is never assigned a value in the constructor, its default value is 0x0. However, when marking the claim status, the contract uses the treasureHash provided by the user:

_markClaimed(treasureHash);

This means that as long as the user-provided treasureHash is not 0x0, the claimed[_treasureHash] check (which always checks claimed[0x0]) will always return false. Consequently, a user can execute multiple claim calls using a single valid treasureHash until the pool is completely drained.

Risk

Likelihood: High

  • Any user who obtains a single valid proof can immediately exploit this vulnerability.

Impact: High

  • A user can use the same treasure and ZK proof to repeatedly call the claim function until all rewards in the contract are withdrawn.

Proof of Concept

  1. Alice finds the first treasure.

  2. Alice generates a ZK proof for her own address.

  3. Alice calls claim(proof, hash1, aliceAddress). The contract checks claimed[0x0], finds it is false, and passes the check.

  4. The contract marks claimed[hash1] as true and pays out 10 ETH.

  5. Alice calls claim(proof, hash1, aliceAddress) again. The contract still checks claimed[0x0]; since 0x0 was never marked, the check passes again.

  6. Alice repeats this operation 10 times until claimsCount reaches MAX_TREASURES or the balance is exhausted.

function test_PoC_DoubleSpending_DrainsContract() public {
(
bytes memory proof,
bytes32 treasureHash,
address payable recipient
) = _loadFixture();
​
address payable aliceRecipient = payable(makeAddr("aliceRecipient"));
uint256 contractBalanceBefore = address(hunt).balance;
uint256 recipientBalanceBefore = aliceRecipient.balance;
​
// First claim - legal
hunt.claim(proof, treasureHash, aliceRecipient);
// Use the exact same proof and hash to claim again
// Even though `treasureHash` has been marked as claimed, the check logic inspects `_treasureHash` (0x0)
for(uint256 i = 0; i < 9; i++) {
hunt.claim(proof, treasureHash, aliceRecipient);
}
​
uint256 recipientBalanceAfter = aliceRecipient.balance;
// Alice claimed all 100 ETH, even though she only found 1 treasure
assertEq(recipientBalanceAfter - recipientBalanceBefore, 100 ether);
assertEq(address(hunt).balance, 0);
console.log("Contract drained! Total claimed:", hunt.claimsCount());
}

Recommended Mitigation

  1. Remove the unnecessary state variable:

    Since the state variable treasureHash is never initialized and used except in the function claim and results in misunderstandings easily, it should be removed.

    - bytes32 private immutable _treasureHash;

  2. modify the require check in function claim

    Change the wrong check towards _treasureHash into that towards treasureHash provided by user.

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();
- if (claimed[_treasureHash]) revert AlreadyClaimed(treasureHash);
+ if (claimed[treasureHash]) revert AlreadyClaimed(treasureHash);
if (msg.sender == owner) revert OwnerCannotClaim();
​
​
// Public inputs must match Noir circuit order:
// treasure_hash, recipient (recipient encoded into 160-bit integer in a Field).
bytes32[] memory publicInputs = new bytes32[](2);
publicInputs[0] = treasureHash;
publicInputs[1] = bytes32(uint256(uint160(address(recipient))));
​
// Verify proof against the public inputs.
// If valid, transfer the reward and mark the treasure as claimed.
bool ok = verifier.verify(proof, publicInputs);
if (!ok) revert InvalidProof();
​
_incrementClaimsCount();
_markClaimed(treasureHash);
​
​
(bool sent, ) = recipient.call{value: REWARD}("");
require(sent, "ETH_TRANSFER_FAILED");
​
emit Claimed(treasureHash, msg.sender);
}
​
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!