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

Wrong variable in `claimed` check lets attacker drain all funds

vlad1k3

Description

  • When a user calls claim(), the contract verifies a ZK proof and if valid — sends 10 ETH to the recipient and marks the treasure as claimed so nobody can claim it again.

  • The problem is that the anti-double-claim check uses the wrong variable. It checks claimed[_treasureHash] where _treasureHash is an immutable field that is never set in the constructor, so it's always bytes32(0). But the actual write goes to claimed[treasureHash] (the parameter). So the check and the write are looking at completely different storage slots — the check never sees the write and always passes.

// _treasureHash is declared here but never assigned anywhere → always bytes32(0)
@> bytes32 private immutable _treasureHash;
​
function claim(bytes calldata proof, bytes32 treasureHash, address payable recipient) external {
...
@> if (claimed[_treasureHash]) revert AlreadyClaimed(treasureHash); // always false
...
_markClaimed(treasureHash); // writes to a different slot
(bool sent,) = recipient.call{value: REWARD}("");
}

Risk

Likelihood:

  • Anyone who gets a valid proof for any single treasure can replay the exact same call 10 times in a row — there's nothing stopping them.

  • No special access or setup needed, just a normal wallet and one proof generated off-chain.

Impact:

  • The attacker drains all 100 ETH from the contract in 10 transactions.

  • All claim slots get used up, so no other player can ever claim any treasure.

Proof of Concept

The attacker generates one valid proof for any treasure, then replays the same (proof, treasureHash, recipient) triple 10 times. Each call passes the claimed[bytes32(0)] check (always false) and transfers 10 ETH, draining the full contract balance.

function test_drainWithSingleProof() public {
bytes memory validProof = ...;
bytes32 treasureHash = ...;
address payable attacker = payable(makeAddr("attacker"));
address payable recipient = payable(makeAddr("recipient"));
​
for (uint256 i = 0; i < hunt.MAX_TREASURES(); i++) {
vm.prank(attacker);
hunt.claim(validProof, treasureHash, recipient);
}
​
assertEq(address(hunt).balance, 0);
assertEq(hunt.getClaimsCount(), 10);
}

Recommended Mitigation

Just remove the unused _treasureHash field and fix the check to use the function parameter:

- bytes32 private immutable _treasureHash;
​
- 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!