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

`withdraw()` lacks access control, allowing any actor to trigger owner settlement

web3pavlou

withdraw() lacks access control, allowing any actor to trigger owner settlement

Description

  • Every other administrative function in the contract (fund, pause, unpause, updateVerifier, emergencyWithdraw) is restricted to the owner. withdraw() is the only exception, containing no access control and callable by any external address once claimsCount >= MAX_TREASURES.

  • Funds always go to owner, so no direct theft is possible. However, any actor can front-run the owner's intended settlement transaction, causing the owner's tx to revert with NO_FUNDS_TO_WITHDRAW and forcing the owner to rely on third parties for execution of a semantically privileged lifecycle step.

@> function withdraw() external { // @> no access control — any caller can invoke
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:

  • Any external address can call withdraw() once claimsCount >= MAX_TREASURES, meaning the condition for exploitation is met at the natural end of every hunt cycle without any special precondition.

  • A griefer monitoring the mempool can front-run the owner's settlement transaction on every withdrawal attempt, consistently forcing the owner's tx to revert with NO_FUNDS_TO_WITHDRAW.

Impact:

  • Inconsistent access control design, as withdraw() is the only admin function not restricted to the owner, undermining the contract's intended privileged lifecycle model.

  • Owner settlement can be griefed into a revert, forcing the owner to rely on third parties for execution of a semantically privileged lifecycle step.

Proof of Concept

function test_withdrawPermissionless() public {
// Manually set claimsCount = 10 via storage manipulation (slot 2)
vm.store(address(hunt), bytes32(uint256(2)), bytes32(uint256(10)));
​
assertEq(hunt.claimsCount(), 10, "storage slot sanity check");
​
uint256 ownerBalanceBefore = owner.balance;
​
// Attacker - griefer (no stake, no benefit) triggers withdrawal
vm.prank(attacker);
hunt.withdraw();
​
// Funds went to owner nonetheless, attacker gets nothing
assertGt(owner.balance, ownerBalanceBefore);
assertEq(address(hunt).balance, 0);
​
// Owner's own attempt though now fails — griefed into revert
vm.prank(owner);
vm.expectRevert("NO_FUNDS_TO_WITHDRAW");
hunt.withdraw();
}

Recommended Mitigation

Add an onlyOwner check, consistent with every other admin function in the contract, or use the existing onlyOwner modifier:

-function withdraw() external {
+function withdraw() external onlyOwner {
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!