SNARKeling Treasure Hunt

Root + Impact

Description

`TreasureHunt.sol` declares `bytes32 private immutable _treasureHash` on line 35 but never assigns it in the constructor, so it permanently holds `bytes32(0)`. The double-claim guard on line 88 uses this uninitialized immutable instead of the function parameter:

```solidity
if (claimed[_treasureHash]) revert AlreadyClaimed(treasureHash);
```

The check always reads `claimed[bytes32(0)]`, which is never written in normal operation, so the guard is bypassed on every call. A valid `(proof, treasureHash, recipient)` tuple can therefore be replayed - Noir/Barretenberg proofs are deterministic for fixed public inputs and the contract has no nonce - until `claimsCount` reaches `MAX_TREASURES (10)`.

Risk

Likelihood: High - any participant who lawfully finds a single treasure (or any front-runner copying a participant's proof from the mempool) can trivially replay their own valid proof by simply calling `claim()` again with the same arguments.

Impact

• Attacker drains 100 ETH - 100% of the default prize pool - to a single recipient per treasure secret obtained, in 10 back-to-back transactions.

• The protocol becomes fully non-functional after a single treasure is found: the remaining 9 legitimate finders receive zero reward even if they complete the hunt.

• The `_markClaimed(treasureHash)` write on line 104 is effectively dead state - it sets the correct slot but the guard never reads it.

Proof of Concept

Exploit steps:

1. Attacker obtains one valid ZK proof `P` for public inputs `(treasureHash = H, recipient = R)`. `R` is a fresh EOA controlled by the attacker, non-zero, non-owner, non-contract, and non-`msg.sender`.

2. Attacker calls `claim(P, H, R)` - the guard reads `claimed[_treasureHash] == claimed[bytes32(0)] == false` and passes, 10 ETH (`REWARD`) is transferred to `R`, and `_markClaimed(H)` writes `claimed[H] = true` on a slot that is never re-read.

3. Attacker calls `claim(P, H, R)` nine more times - each call passes the same broken check because the guard still reads `claimed[bytes32(0)] == false`. The `nonReentrant` lock releases between transactions, `address(this).balance >= REWARD` holds until the last replay, and `claimsCount` increments from 1 to 10.

4. After the 10th replay, `address(this).balance == 0`, `claimsCount == MAX_TREASURES`, and any subsequent call reverts with `NotEnoughFunds()`. The 9 legitimate finders of the remaining treasures can never claim.

The following Foundry test reproduces the drain. `MockVerifier.verify -> true` models the attacker possessing one valid Noir/Barretenberg proof for `(H, R)`:

```solidity
function test_EXPLOIT_ReplayDrainsContract() public {
assertEq(address(hunt).balance, 100 ether); // contract fully funded
assertEq(recipient.balance, 0); // attacker recipient empty

vm.startPrank(attacker);
hunt.claim(PROOF, TREASURE_HASH, recipient);      // 1st: guard reads claimed[0]=false, pays 10 ETH
for (uint256 i = 0; i < 9; i++) {
    hunt.claim(PROOF, TREASURE_HASH, recipient);  // 9 replays, each passes identically
}
vm.stopPrank();

assertEq(recipient.balance, 100 ether);           // entire pool drained to attacker recipient
assertEq(address(hunt).balance, 0);
assertEq(hunt.getClaimsCount(), 10);

}
```

Running `forge test --match-test test_EXPLOIT_ReplayDrainsContract -vv` produces `[PASS] (gas: 367320)` with `recipient.balance = 100 ether`, contract balance 0, and `claimsCount = 10`. The protocol's "10 distinct winners, 10 ETH each" invariant is fully destroyed after a single treasure is found.

Recommended Mitigation

Fix the guard to read the function parameter and remove the dead immutable:

```diff

•   if (claimed[_treasureHash]) revert AlreadyClaimed(treasureHash);
  

•   if (claimed[treasureHash]) revert AlreadyClaimed(treasureHash);
  

```

```diff

• bytes32 private immutable _treasureHash;
  ```

The `claim` function parameter is named `treasureHash` (no underscore); swapping the read to the parameter makes the guard consult the same storage slot that `_markClaimed(treasureHash)` writes on line 104, restoring the intended "one claim per treasure" invariant. The next replay with the same `treasureHash` now reads `claimed[treasureHash] == true` and reverts with `AlreadyClaimed(treasureHash)`. The immutable on line 35 is never assigned and never read by any correct code path - it exists only as a near-identical shadow of the parameter name that led to the original typo - so deleting it prevents the same mistake from being reintroduced.

For defense-in-depth, bind the ZK proof to `msg.sender` as an additional public input by extending the Noir circuit and expanding `publicInputs` on line 95:

```solidity
bytes32[] memory publicInputs = new bytes32;
publicInputs[0] = treasureHash;
publicInputs[1] = bytes32(uint256(uint160(address(recipient))));
publicInputs[2] = bytes32(uint256(uint160(msg.sender)));
```

Binding the proof to the caller prevents a mempool observer from replaying another participant's proof even if the primary guard is later regressed by a different code change, upgrading replay resistance from "per (treasure, recipient)" to "per (treasure, recipient, submitter)". Finally, un-comment `vm.expectRevert()` inside `testClaimDoubleSpendReverts` at line 134 - it is currently commented out, so the existing test passes regardless of the guard's correctness and will not catch a future regression.