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

C-01: Double-spend protection uses wrong storage key allowing the same treasure to be claimed repeatedly

altshar

Root + Impact

Description

  • TreasureHunt.claim() is designed to mark each treasure as claimed after the first successful proof submission, preventing a valid proof from being reused to collect multiple rewards.

  • The replay-protection check reads from claimed[_treasureHash] where _treasureHash is a bytes32 immutable declared at the contract level but never assigned in the constructor, so its value is always bytes32(0). The write that follows the check correctly targets claimed[treasureHash] (the function parameter). Because the guard reads from a permanently-false key, it never reverts and the same proof can be submitted repeatedly.

contract TreasureHunt {
// @> _treasureHash is declared immutable but never assigned — always bytes32(0)
bytes32 private immutable _treasureHash;
​
function claim(bytes calldata proof, bytes32 treasureHash, address payable recipient) external nonReentrant() {
...
// @> reads claimed[bytes32(0)] — always false, guard never fires
if (claimed[_treasureHash]) revert AlreadyClaimed(treasureHash);
...
// @> writes to the correct key — but the guard above never checks it
_markClaimed(treasureHash);
...
(bool sent, ) = recipient.call{value: REWARD}("");
}
}

Risk

Likelihood:

  • A participant who has generated one valid ZK proof immediately has the ability to repeat the call, with no special conditions required beyond a valid proof.

  • The test suite already demonstrates the bug: testClaimDoubleSpendReverts has its vm.expectRevert() commented out, confirming the second claim succeeds.

Impact:

  • A single valid proof can drain the entire 100 ETH contract balance by calling claim() ten times before AllTreasuresClaimed fires.

  • All other treasure finders are denied their rewards as the balance is exhausted.

Proof of Concept

The existing test in TreasureHunt.t.sol directly demonstrates the vulnerability. The vm.expectRevert() line that should catch the second claim has been commented out, confirming that the duplicate call succeeds. An attacker who has obtained a single valid proof can loop this call up to ten times, collecting 10 ETH per iteration and fully draining the 100 ETH prize pool before any other participant can collect their reward.

// From TreasureHunt.t.sol lines 134–147
function testClaimDoubleSpendReverts() public {
(bytes memory proof, bytes32 treasureHash, address payable recipient) = _loadFixture();
​
vm.startPrank(participant);
hunt.claim(proof, treasureHash, recipient); // succeeds — balance: 90 ETH
​
//vm.expectRevert(); <-- commented out; second claim also succeeds
hunt.claim(proof, treasureHash, recipient); // succeeds again — balance: 80 ETH
vm.stopPrank();
// Repeat 8 more times to drain 100 ETH with one proof
}

Recommended Mitigation

The root cause is a one-character typo: the replay guard reads from _treasureHash (the unassigned immutable, always bytes32(0)) instead of treasureHash (the caller-supplied parameter). Fixing the variable name in the guard expression closes the attack vector with a minimal, low-risk change. The immutable declaration should also be removed to eliminate the confusingly-named dead variable that enabled the bug.

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

Also remove the unused declaration:

- bytes32 private immutable _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!