SNARKeling Treasure Hunt
First Flight #59
Beginner FriendlyGameFiFoundry
100 EXP
View all submissions
Previous Next
Submission Details
Impact: high
Likelihood: high

Wrong storage variable used in duplicate-claim check

Author Revealed upon completion

Root + Impact

Description

  • claim() is supposed to prevent the same treasure from being claimed twice by checking the claimed mapping before processing a submission. After a successful claim, \_markClaimed(treasureHash) correctly writes claimed\[treasureHash] = true using the caller-supplied parameter.

  • However, the duplicate-claim guard reads from claimed[_treasureHash] instead — where _treasureHash is a private immutable state variable that is never initialized in the constructor, making it permanently bytes32(0). The guard therefore always evaluates to false, and the same treasure can be claimed repeatedly until the contract is drained.

// @> _treasureHash is an uninitialized immutable — always bytes32(0)
bytes32 private immutable _treasureHash;
​
function claim(bytes calldata proof, bytes32 treasureHash, address payable recipient) external nonReentrant() {
// ...
// @> reads from the wrong variable; claimed[bytes32(0)] is always false
if (claimed[_treasureHash]) revert AlreadyClaimed(treasureHash);
// ...
// @> correctly writes to claimed[treasureHash], but the guard above never catches it
_markClaimed(treasureHash);
// ...
}

Risk

Likelihood:

  • Every valid proof submission succeeds regardless of whether the treasure was already claimed, because the guard is permanently bypassed.

  • No special attacker capability is required beyond possessing one valid proof.

Impact:

  • An attacker with a single valid proof can call claim() repeatedly, draining the full contract balance (up to 100 ETH) in REWARD-sized (10 ETH) increments.

  • All other treasure finders are deprived of their rewards once the contract balance falls below REWARD.

Proof of Concept

The root cause is that _treasureHash is an immutable variable declared at the contract level but never assigned in the constructor — Solidity leaves it as bytes32(0). Meanwhile, _markClaimed correctly writes to claimed[treasureHash] (the function parameter), so the mapping entry for the real treasure hash does get set to true after the first claim. The guard, however, never reads that entry: it always checks claimed[bytes32(0)], which remains false forever. This means every subsequent call with the same proof passes the guard and proceeds to transfer another REWARD.

function testDrainWithSingleProof() public {
// Attacker has one valid proof for treasureHash_1
bytes memory validProof = _getValidProof(TREASURE_HASH_1);
​
uint256 initialBalance = address(hunt).balance; // 100 ether
address attacker = makeAddr("attacker");
address recipient = makeAddr("recipient");
​
// Claim the same treasure 10 times — each succeeds because
// claimed[_treasureHash] (bytes32(0)) is never set to true
for (uint256 i = 0; i < 10; i++) {
vm.prank(attacker);
hunt.claim(validProof, TREASURE_HASH_1, payable(recipient));
}
​
// Contract fully drained
assertEq(address(hunt).balance, 0);
// Recipient received all 100 ETH
assertEq(recipient.balance, initialBalance);
// claimsCount is 10, hunt appears "over" even though only 1 treasure was found
assertEq(hunt.getClaimsCount(), 10);
}

Recommended Mitigation

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

Remove the unused immutable variable entirely:

- bytes32 private immutable _treasureHash;

Support

FAQs

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

Give us feedback!