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

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

bitguardx

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 {
Updates

Lead Judging Commences

s3mvl4d Lead Judge 18 days ago
Submission Judgement Published
Validated
Assigned finding tags:

unclaimable treasure / bricked withdraw path

The issue stems from a mismatch between the circuit and the contract’s economic assumptions: the Solidity contract is configured for `MAX_TREASURES = 10` and only allows the owner to call `withdraw()` once `claimsCount >= MAX_TREASURES`, while the Noir circuit’s baked-in `ALLOWED_TREASURE_HASHES` array does not actually contain ten distinct treasures because one hash is duplicated and another expected hash is missing. As a result, under the intended one-claim-per-treasure design described in the README, there are only nine uniquely claimable treasures even though the system is funded and accounted as if ten rewards can be legitimately redeemed. That creates two linked consequences from the same root cause: first, one treasure is effectively unclaimable because no valid proof can ever be generated for the missing allowed hash, and second, the normal “hunt over” withdrawal path becomes bricked because honest participants can never reach ten legitimate unique claims, leaving the post-hunt fund recovery logic via `withdraw` function permanently unreachable. The owner can still intervene through the emergency path.

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!