SNARKeling Treasure Hunt

Root + Impact

Description

The `withdraw()` function on lines 223-232 of `TreasureHunt.sol` lacks access control, while `owner` is declared as `immutable` on line 34 and therefore cannot be changed after deployment:

```solidity
// line 34
address private immutable owner;

// lines 223-232
function withdraw() external {
require(claimsCount >= MAX_TREASURES, "HUNT_NOT_OVER");
uint256 balance = address(this).balance;
require(balance > 0, "NO_FUNDS_TO_WITHDRAW");
(bool sent, ) = owner.call{value: balance}("");
require(sent, "ETH_TRANSFER_FAILED");
emit Withdrawn(balance, address(this).balance);
}
```

The NatSpec on line 222 ("Allow the owner to withdraw unclaimed funds") clearly documents the intended onlyOwner semantics, but the modifier is missing. In isolation the missing check seems harmless - the ETH goes to `owner` regardless of who calls it - but combined with the immutable `owner` slot, it creates a permanent-lock scenario: if the owner is (or becomes) a smart contract that reverts on plain ETH transfers, or a multisig that has lost its signers, `owner.call{value}("")` returns `sent == false` on every call and `withdraw()` reverts forever. There is no path to change the destination because `owner` is `immutable`.

Risk

Likelihood: Low - requires the specific configuration where `owner` is a contract that rejects ETH (for example a multisig with a restrictive `receive()` or an EOA replaced with a contract via CREATE2 counterfactual deployment). EOAs always accept ETH so the default path is safe.

Impact

• If `owner` turns out to be unable to receive ETH (contract with no `receive()` / reverting `fallback()`, multisig with lost signers), all remaining contract balance is permanently stranded after the hunt ends.

• The only alternative recovery path is `emergencyWithdraw()` - but that also requires `msg.sender == owner` (line 275), which is itself unreachable if the immutable owner contract cannot sign transactions.

• Griefing by front-running: anyone can call `withdraw()` at the exact moment `claimsCount` hits `MAX_TREASURES`, racing the owner. With the owner contract now on the receive end, any pending owner transaction that expected a different call ordering may fail unexpectedly.

• The missing modifier also weakens static-analysis and audit tooling guarantees - tools flag `withdraw` as "publicly callable, moves ETH" when it should be annotated as owner-only.

Proof of Concept

Exploit scenario (permanent lock via immutable owner + unreachable recipient):

1. Project team deploys `TreasureHunt.sol` from a multisig wallet as the `msg.sender` of the constructor. The multisig's `receive()` reverts to prevent accidental ETH deposits (a common defensive pattern in Safe-style wallets with custom modules).

2. The hunt runs normally - `claim()` does not rely on `owner.call` so participants redeem treasures successfully. `claimsCount` reaches `MAX_TREASURES = 10`.

3. The project tries to clean up by calling `withdraw()` from the multisig. Execution reaches line 228: `(bool sent, ) = owner.call{value: balance}("");`. The multisig's `receive()` reverts, `sent == false`, and line 229 triggers `require(sent, "ETH_TRANSFER_FAILED")` - the transaction reverts.

4. Every retry produces the same revert. The owner also cannot use `emergencyWithdraw()` as a workaround because (a) it requires `pause()` first, (b) it still uses `call{value}` to the recipient (the owner contract is excluded as a recipient by line 276, so funds cannot even be sent back to the multisig).

5. All remaining ETH (10 ETH for any under-claimed hunt, or any non-claim revenue accumulated via the unauthenticated `receive()` fallback) is permanently frozen. The `owner` slot is `immutable` - there is no admin path to change the destination.

Recommended Mitigation

Add the `onlyOwner` modifier to document and enforce the intended semantics:

```diff

• function withdraw() external {

• function withdraw() external onlyOwner {
  require(claimsCount >= MAX_TREASURES, "HUNT_NOT_OVER");
  uint256 balance = address(this).balance;
  require(balance > 0, "NO_FUNDS_TO_WITHDRAW");
  (bool sent, ) = owner.call{value: balance}("");
  require(sent, "ETH_TRANSFER_FAILED");
  emit Withdrawn(balance, address(this).balance);
  }
  ```

This aligns the implementation with the NatSpec and reduces the attack surface; anyone attempting to trigger the permanent-lock scenario as an outsider must instead wait for the actual owner, who has already committed to the same destination address.

For defense-in-depth against the permanent-lock scenario itself, accept an explicit withdrawal recipient parameter instead of hardcoding `owner`:

```solidity
function withdraw(address payable recipient) external onlyOwner {
require(claimsCount >= MAX_TREASURES, "HUNT_NOT_OVER");
require(recipient != address(0), "INVALID_RECIPIENT");
uint256 balance = address(this).balance;
require(balance > 0, "NO_FUNDS_TO_WITHDRAW");
(bool sent, ) = recipient.call{value: balance}("");
require(sent, "ETH_TRANSFER_FAILED");
emit Withdrawn(balance, address(this).balance);
}
```

An owner-controlled recipient lets the owner route leftover funds to any address that can actually receive ETH (a new treasury, a different multisig, a community wallet), preventing the immutable-owner-plus-reverting-recipient deadlock regardless of what type of contract the owner originally was.