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

claim() performs external verifier call before state updates, violating CEI pattern and making nonReentrant the sole protection against double-claims

blackburn

Root + Impact

Description

  • The claim() function follows a C-I-E-I pattern instead of the correct C-E-I pattern. The external call to verifier.verify() happens before claimsCount and claimed are updated. This means if a malicious or compromised verifier re-enters claim() during verification, the state has not yet been updated and all checks pass again. The nonReentrant modifier currently prevents this, but it is load-bearing for two independent reasons simultaneously — reentrancy protection AND replay protection (due to the broken claimed check in L-1). Removing or bypassing the guard would make the contract instantly drainable in a single transaction.

function claim(...) external nonReentrant() {
// Checks
if (claimed[_treasureHash]) revert AlreadyClaimed(treasureHash);
​
// @> Interaction BEFORE effects — external call to verifier
bool ok = verifier.verify(proof, publicInputs);
if (!ok) revert InvalidProof();
​
// @> Effects come after the external call — too late
_incrementClaimsCount();
_markClaimed(treasureHash);
​
// Second interaction
(bool sent, ) = recipient.call{value: REWARD}("");
}

Risk

Likelihood:

  • Requires a malicious or compromised verifier contract

  • Owner is trusted, so verifier swap is unlikely to be malicious

  • Low likelihood under normal operation

Impact:

  • If nonReentrant is ever removed, a malicious verifier can reenter claim() before state updates and drain the contract in a single transaction

  • nonReentrant is currently carrying two separate responsibilities — removing it for any reason exposes both reentrancy and replay vectors simultaneously

  • Correct CEI costs nothing to implement

Recommended Mitigation

bool ok = verifier.verify(proof, publicInputs);
if (!ok) revert InvalidProof();
​
+ _incrementClaimsCount();
+ _markClaimed(treasureHash);
​
- _incrementClaimsCount();
- _markClaimed(treasureHash);
​
(bool sent, ) = recipient.call{value: REWARD}("");
Updates

Lead Judging Commences

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

broken double-claim protection

In `claim()`, the guard uses `claimed[_treasureHash]`, where `_treasureHash` is an immutable state variable that is never initialized to the caller-supplied treasure identifier, while the contract later marks `claimed[treasureHash] = true` using the function argument instead. As a result, the duplicate-claim check and the state update are performed against different keys, which means a previously claimed treasure is not actually blocked from being claimed again with the same valid proof and `treasureHash`. This breaks a core invariant of the protocol described in the README, namely, that each treasure can only be redeemed once, and allows one valid treasure/proof pair to be reused to drain rewards repeatedly until either the `MAX_TREASURES` cap or the contract balance is exhausted.

Support

FAQs

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

Give us feedback!