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

Broken access control on withdraw

fredo182

Broken access control on withdraw

Description

The withdraw function is documented as "Allow the owner to withdraw unclaimed funds after the hunt is over." Also, all other admin function in the contract allow only the owner to call them. However, withdraw function has no caller restriction. Anyone can call it once claims have reached the desired count, forcing the entire contract balance to be send to the owner address. While the funds are not stolen, the owner loses control over when they receive them.

/// @notice Allow the owner to withdraw unclaimed funds after the hunt is over.
function withdraw() external {
require(claimsCount >= MAX_TREASURES, "HUNT_NOT_OVER");
// @> missing access control check
​
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: High

Any external account or contract can call withdraw as soon as all 10 treasures have been claimed. No special permissions are required.

Impact: Low

Owner is forced to receive an incoming transfer. It is possible that several assumptions are connected with when the funds are received.

Proof of Concept

function test_anyoneCanWithdraw() public {
// Top up the balance
vm.deal(address(hunt), 500 ether);
​
// Take advantage of the other bug to complete the hunt
(bytes memory proof, bytes32 treasureHash, address payable recipient) = _loadFixture();
hunt.claim(proof, treasureHash, recipient);
hunt.claim(proof, treasureHash, recipient);
hunt.claim(proof, treasureHash, recipient);
hunt.claim(proof, treasureHash, recipient);
hunt.claim(proof, treasureHash, recipient);
hunt.claim(proof, treasureHash, recipient);
hunt.claim(proof, treasureHash, recipient);
hunt.claim(proof, treasureHash, recipient);
hunt.claim(proof, treasureHash, recipient);
hunt.claim(proof, treasureHash, recipient);
​
// State before withdrawal
uint256 ownerBalBefore = owner.balance;
uint256 contractBal = address(hunt).balance;
assertGt(contractBal, 0);
​
// Anyone can withdraw
address anyone = address(0xCAFE);
vm.prank(anyone);
hunt.withdraw();
​
// State after withdrawal
assertEq(owner.balance, ownerBalBefore + contractBal);
assertEq(address(hunt).balance, 0);
}

Recommended Mitigation

function withdraw() external {
+ require(msg.sender == owner, "ONLY_OWNER");
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);
}
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!