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

Duplicate Treasure Hashes in Noir Circuit Permanently Block withdraw() Function

Author Revealed upon completion

Root + Impact

Description

  • TheWithdrawnevent is intended to log the amount withdrawn and the contract's new balance after withdrawal

  • The event emitsaddress(``this``).balanceas thenewBalanceparameter after the ETH transfer has already occurred, which will always be 0 (or near 0) for a full withdrawal, and the pre-transfer balance snapshot could be stale if concurrent deposits occur

// Root cause in the codebase
function withdraw() external {
require(claimsCount >= MAX_TREASURES, "HUNT_NOT_OVER");
​
uint256 balance = address(this).balance; // @> Snapshot taken before transfer
require(balance > 0, "NO_FUNDS_TO_WITHDRAW");
(bool sent, ) = owner.call{value: balance}(""); // @> Transfer happens here
require(sent, "ETH_TRANSFER_FAILED");
​
emit Withdrawn(balance, address(this).balance); // @> address(this).balance is now 0 after transfer
}

Risk

Likelihood: LOW

  • This will occur every timewithdraw()is called after all treasures are claimed

  • Edge case occurs when ETH is sent to the contract (viareceive()) between balance snapshot and transfer

Impact: LOW

  • Off-chain monitoring systems receive potentially misleading event data

  • Theamountfield could be incorrect if concurrent deposits occur during execution

  • ThenewBalancefield is redundant (always 0 after full withdrawal)

  • Could cause minor accounting discrepancies in indexers and monitoring tools

Proof of Concept

The vulnerability causes incorrect event emission data, particularly in edge cases with concurrent ETH deposits during withdrawal execution.

// Normal case: Standard withdrawal after hunt completion
TreasureHunt hunt = new TreasureHunt{value: 100 ether}(verifierAddress);
// ... claim 10 treasures, leaving 0 ETH ...
// Someone sends 5 ETH remainder to contract
payable(address(hunt)).transfer(5 ether);
​
// Owner calls withdraw
uint256 contractBalanceBefore = address(hunt).balance; // 5 ETH
hunt.withdraw();
​
// Check emitted event:
// Event: Withdrawn(amount=5 ETH, newBalance=0)
// The newBalance is always 0 after full withdrawal (technically correct but redundant)
​
// Edge case: Concurrent deposit during withdrawal
contract ConcurrentDeposit {
TreasureHunt hunt;
function exploitEdgeCase() external payable {
// Step 1: withdraw() starts, balance snapshot = 5 ETH
// Step 2: During execution, someone sends 1 ETH via receive()
// Step 3: owner.call{value: balance} actually transfers 6 ETH (full balance)
// Step 4: Event emits: Withdrawn(amount=5 ETH, newBalance=0)
// Problem: Event says 5 ETH withdrawn, but actually 6 ETH was transferred
}
}
​
// Demonstration with granular control:
function testWithdrawEventEdgeCase() external {
// Setup: Contract has 5 ETH after claims
assert(address(hunt).balance == 5 ether);
// Hook into withdraw execution (pseudo-code for illustration)
vm.mockCall(
address(hunt.owner()),
abi.encodeWithSelector(bytes4(keccak256("call()"))),
// Between balance snapshot and transfer, inject 1 ETH
abi.encode(true)
);
// During withdraw(), 1 ETH is added after snapshot
payable(address(hunt)).transfer(1 ether);
hunt.withdraw();
// Event shows: Withdrawn(5 ETH, 0)
// Reality: 6 ETH was transferred (5 + 1 concurrent deposit)
// Off-chain systems see wrong amount
}

Recommended Mitigation

Ensure consistent event data by either capturing the actual transferred amount or simplifying the event:

// Option 1: Simplify - newBalance is always 0 for full withdrawal

- emit Withdrawn(balance, address(this).balance);
+ emit Withdrawn(balance, 0); // @> Explicitly emit 0 for clarity

// Option 2: Prevent concurrent deposits during withdrawal

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

Support

FAQs

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

Give us feedback!