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

`withdraw()` is publicly callable, enabling griefing of the owner after the hunt ends

Author Revealed upon completion

Description

  • Sibling admin functions in the same contract (fund(), pause(), unpause(), updateVerifier(), emergencyWithdraw()) all enforce msg.sender == owner, and the NatSpec above withdraw() says "Allow the owner to withdraw unclaimed funds after the hunt is over." The intent is clearly owner-only.

  • withdraw() itself has no caller check. Any address can invoke it once claimsCount >= MAX_TREASURES, forcing the contract to transfer its remaining balance to owner. Funds are not at risk (the transfer target is still owner), but the owner loses control over the timing and gas price of the finalization event, enabling griefing and off-chain bookkeeping confusion.

// contracts/src/TreasureHunt.sol
error OnlyOwnerCanFund(); // declared on line 18, unused — a strong hint that an equivalent guard was planned here
​
/// @notice Allow the owner to withdraw unclaimed funds after the hunt is over.
function withdraw() external { // @> no onlyOwner / msg.sender == owner check
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}(""); // funds go to owner regardless of caller
require(sent, "ETH_TRANSFER_FAILED");
emit Withdrawn(balance, address(this).balance);
}
​
// By contrast, fund() on line 237 does enforce the owner check:
function fund() external payable {
require(msg.sender == owner, "ONLY_OWNER_CAN_FUND");
...
}

Risk

Likelihood:

  • Consistently callable by any address once claimsCount == MAX_TREASURES. No special conditions required; the first external observer who notices the hunt has ended can invoke it.

  • Easy to automate: a watcher bot subscribing to Claimed events can race to call withdraw() the moment the tenth claim confirms.

Impact:

  • No theft of funds — owner remains the transfer destination hardcoded on line 228.

  • Griefing: a non-owner forces the owner to bear the cost of a gas-price-of-their-choosing finalization log, or emits the Withdrawn event at an inconvenient moment (e.g. right before a multi-tx owner batch).

  • Off-chain automation that assumes Withdrawn is emitted from a transaction where tx.origin == owner (for ACL, cron, or notification purposes) misclassifies the event.

  • Minor searcher MEV risk: generic hunters who earn rewards for emitting specific events may front-run the owner's finalization.

Proof of Concept

Deploy the contract, fund it with 100 ETH, and simulate ten valid claims (or use vm.deal and direct storage writes in Foundry to set claimsCount = MAX_TREASURES). Then, from a non-owner EOA, call hunt.withdraw(). The call succeeds, transferring the entire remaining balance to owner and emitting Withdrawn from a transaction the owner did not authorize.

// Foundry sketch — any non-owner EOA can trigger withdraw() once the hunt closes
function test_anyoneCanTriggerWithdraw() public {
// arrange: deposit 100 ETH, mark hunt as complete
vm.deal(address(hunt), 100 ether);
stdstore.target(address(hunt)).sig("claimsCount()").checked_write(uint256(10));
​
// act: random EOA calls withdraw()
address randomAttacker = makeAddr("attacker");
vm.prank(randomAttacker);
hunt.withdraw();
​
// assert: funds landed with owner even though attacker triggered the call
assertEq(address(hunt).balance, 0);
assertEq(owner.balance, 100 ether);
// The Withdrawn event was emitted under attacker's tx, not owner's — ACLs keyed on tx.origin misclassify it.
}

Recommended Mitigation

Add the owner check already used by sibling admin functions. The contract even declares the matching error OnlyOwnerCanFund but never uses it, and the existing onlyOwner modifier on line 53 is the idiomatic fit.

/// @notice Allow the owner to withdraw unclaimed funds after the hunt is over.
- function withdraw() external {
+ function withdraw() external onlyOwner {
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!