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

`withdraw()` is publicly callable despite being documented as an owner withdrawal flow

raihanmd

`withdraw()` is publicly callable despite being documented as an owner withdrawal flow

Description

  • The README and NatSpec describe `withdraw()` as the owner's post-hunt withdrawal path for leftover funds.

  • The implementation omits any ownership check, so any external account can trigger the withdrawal once `claimsCount >= MAX_TREASURES`. Although the funds still go to `owner`, the access-control boundary described by the project is not enforced on-chain.

/// @notice Allow the owner to withdraw unclaimed funds after the hunt is over.
function withdraw() external {
require(claimsCount >= MAX_TREASURES, "HUNT_NOT_OVER");
​
uint256 balance = address(this).balance;
require(balance > 0, "NO_FUNDS_TO_WITHDRAW");
// @> anyone can trigger the owner's withdrawal flow
(bool sent,) = owner.call{value: balance}("");
require(sent, "ETH_TRANSFER_FAILED");
}
​

Risk

Likelihood: MEDIUM

  • Any arbitrary account can call `withdraw()` as soon as the treasure-count condition is satisfied and the contract later receives additional ETH.

  • The PoC demonstrates the behavior with no privileged access by letting an attacker trigger the final withdrawal after the owner replenishes the contract.

Impact: MEDIUM

  • The contract does not enforce the access-control model communicated to users and integrators.

  • Unauthorized callers can force protocol settlement timing and trigger owner-only operational flows unexpectedly.

Proof of Concept

Paste this test function to test file and run it

function testPoCLow_AnyoneCanTriggerWithdrawFlow() public {
(bytes memory proof, bytes32 treasureHash, address payable recipient) = _loadFixture();
​
_claimTenTimes(proof, treasureHash, recipient);
​
uint256 ownerBalanceBeforeFund = owner.balance;
vm.prank(owner);
hunt.fund{value: 1 ether}();
assertEq(owner.balance, ownerBalanceBeforeFund - 1 ether);
​
uint256 ownerBalanceBeforeWithdraw = owner.balance;
​
vm.prank(attacker);
hunt.withdraw();
​
assertEq(address(hunt).balance, 0);
assertEq(owner.balance, ownerBalanceBeforeWithdraw + 1 ether);
}
​

Recommended Mitigation

Use the modifier onlyOwner that has been defined in this function

+ 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!