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

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

0xawy

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");
​
​
Updates

Lead Judging Commences

s3mvl4d Lead Judge 18 days ago
Submission Judgement Published
Validated
Assigned finding tags:

broken access control in withdraw()

The `withdraw()` function is intended as an owner-only post-hunt recovery function, but the implementation does not actually enforce any ownership check before transferring the full remaining balance to owner. The function only requires that `claimsCount >= MAX_TREASURES` and that the contract balance is nonzero, after which it sends all ETH to the stored owner address regardless of who called the function. Therefore, the access control on the function itself is incomplete because any external account can trigger the withdrawal path once the hunt is considered over.

Support

FAQs

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

Give us feedback!