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

Broken access control on withdraw

Author Revealed upon completion

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

Support

FAQs

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

Give us feedback!