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

`withdraw()` is callable by any address after the hunt ends

virgilbb

Root + Impact

Description

  • Normal behavior: Post-hunt withdrawal of leftover funds is an organizer-controlled action, or it is documented that any party may trigger settlement.

  • Problem: withdraw() has no onlyOwner modifier. Once claimsCount >= MAX_TREASURES, any caller can invoke it. ETH still goes to owner, but the caller and timing are not restricted to the owner.

// @> no access modifier — any msg.sender may call after claimsCount >= MAX_TREASURES
function withdraw() external {
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:

  • After the tenth claim, any EOA or contract can call withdraw() on the next block or later.

  • Bots or third parties routinely observe contract state and call public settlement-style functions.

Impact:

  • Owner loses exclusive control over when post-hunt funds are swept, which matters for accounting, taxes, and coordination with off-chain hunt closure.

  • No direct theft to an arbitrary address here, but policy and operational expectations break.

Proof of Concept

Explanation: After ten claims, ATTACKER calls withdraw() while OWNER receives the ETH. The test proves msg.sender need not be the owner.

Supporting code — run (fixtures from circuits/scripts/build.sh):

forge test --match-test testPoC_AnyoneCanForceHuntEndAndTriggerWithdraw -vv
​

Supporting code — test:

// contracts/test/TreasureHuntPoC.t.sol — testPoC_AnyoneCanForceHuntEndAndTriggerWithdraw
function testPoC_AnyoneCanForceHuntEndAndTriggerWithdraw() public {
vm.startPrank(OWNER);
hunt.fund{value: 20 ether}();
vm.stopPrank();
​
(bytes memory proof, bytes32 treasureHash, address payable recipient) = _fixture();
vm.startPrank(PARTICIPANT);
for (uint256 i = 0; i < 10; i++) {
hunt.claim(proof, treasureHash, recipient);
}
vm.stopPrank();
​
uint256 ownerBefore = OWNER.balance;
vm.prank(ATTACKER);
hunt.withdraw();
​
assertEq(OWNER.balance, ownerBefore + 20 ether);
}
​

Recommended Mitigation

Explanation: Post-hunt withdrawal should be an owner-controlled administrative action. Restricting withdraw with onlyOwner preserves exclusive control over timing and accounting while keeping funds routed to owner as today.

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