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

Duplicate entries in Noir circuit ALLOWED_TREASURE_HASHES permanently break the withdraw() sweep mechanism

Author Revealed upon completion

Root + Impact

Description

  • Thewithdraw()function is intended to allow the contract owner to withdraw remaining funds after all treasures have been claimed (whenclaimsCount >= MAX_TREASURES)

  • The function completely lacks access control, allowing any external address to trigger the withdrawal once the hunt is complete, even though the funds are sent to the hardcoded owner address

// Root cause in the codebase
function withdraw() external { // @> No access control modifier or 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, but anyone can trigger
require(sent, "ETH_TRANSFER_FAILED");
​
emit Withdrawn(balance, address(this).balance);
}
​
// Compare to other admin functions that DO have access control:
function pause() external {
require(msg.sender == owner, "ONLY_OWNER_CAN_PAUSE"); // @> Has access control
...
}

Risk

Likelihood:MEDIUM

  • This will occur when any malicious actor or bot monitors the contract and callswithdraw()after the 10th treasure is claimed

  • Front-running bots will likely call this function before the owner can

Impact:MEDIUM

  • Allows unauthorized triggering of owner withdrawal, violating principle of least privilege

  • Creates griefing vector through front-running the owner's intended withdrawal timing

  • Could cause issues if owner is a smart contract with conditional receive() logic

  • EmitsWithdrawnevent with unauthorized caller's context, potentially corrupting off-chain monitoring systems

  • While funds go to correct address, this breaks administrative control expectations

Proof of Concept

The vulnerability allows any external address to trigger the withdrawal function after all treasures are claimed. While the funds are sent to the correct owner address, this breaks the expected access control pattern and can cause operational issues.

// Setup: Deploy contract and claim all 10 treasures
TreasureHunt hunt = new TreasureHunt(verifierAddress);
// ... claim 10 treasures ...
assert(hunt.claimsCount() == 10); // All treasures claimed
​
// Attack scenario: Random attacker triggers withdrawal
address randomAttacker = address(0xDEAD);
address contractOwner = hunt.owner();
uint256 ownerBalanceBefore = contractOwner.balance;
uint256 contractBalance = address(hunt).balance; // Remaining funds in contract
​
// Attacker (not the owner) calls withdraw
vm.prank(randomAttacker);
hunt.withdraw(); // Succeeds! No access control check prevents this
​
// Verify results:
assert(contractOwner.balance == ownerBalanceBefore + contractBalance); // Owner got funds (correct)
assert(address(hunt).balance == 0); // Contract drained
// But: Withdrawn event was emitted with randomAttacker as msg.sender (wrong context)
​
// Edge case: If owner is a smart contract with logic like:
contract OwnerContract {
mapping(address => bool) authorized;
receive() external payable {
// Only accept ETH from authorized sources
require(authorized[msg.sender], "Unauthorized transfer");
}
}
// The unauthorized withdraw() call would cause the ETH transfer to revert
// Funds would be stuck until owner updates their contract logic

Recommended Mitigation

Add access control to match all other administrative functions:

function withdraw() external {

require(msg.sender == owner, "ONLY_OWNER"); // @> Add this line

.....}

- remove this code
+ require(msg.sender == owner, "ONLY_OWNER"); // @> Add this line


Alternative: Use the existing but unused onlyOwner modifier

- function withdraw() external {
+ function withdraw() external onlyOwner {

Support

FAQs

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

Give us feedback!