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

Incorrect claimed check allows multiple valid claim calls for the same treasure secret

Author Revealed upon completion

Root + Impact

Description

  • According to the specification, the SNARKeling Treasure Hunt protocol should not allow claiming the reward for

    an already claimed treasure

  • An incorrect check with an invalid key over the claimed mapping allows claiming up to MAX_TREASURES times for

    the same secret

function claim(
bytes calldata proof,
bytes32 treasureHash,
address payable recipient
) external nonReentrant {
if (paused) revert ContractPaused();
if (address(this).balance < REWARD) revert NotEnoughFunds();
if (
recipient == address(0) ||
recipient == address(this) ||
recipient == owner ||
recipient == msg.sender
) revert InvalidRecipient();
if (claimsCount >= MAX_TREASURES) revert AllTreasuresClaimed();
//@audit: Function checks the _treasureHash uninitialized storage variable, which will always be false
//therefore, the function will never revert with the AlreadyClaimed custom error
@> if (claimed[_treasureHash]) revert AlreadyClaimed(treasureHash);
if (msg.sender == owner) revert OwnerCannotClaim();
​
// Public input if (claimed[_treasureHash]) revert AlreadyClaimed(treasureHash);s must match Noir circuit order:
// treasure_hash, recipient (recipient encoded into 160-bit integer in a Field).
bytes32[] memory publicInputs = new bytes32[](2);
publicInputs[0] = treasureHash;
publicInputs[1] = bytes32(uint256(uint160(address(recipient))));
​
// Verify proof against the public inputs.
// If valid, transfer the reward and mark the treasure as claimed.
bool ok = verifier.verify(proof, publicInputs);
if (!ok) revert InvalidProof();
​
_incrementClaimsCount();
_markClaimed(treasureHash);
​
(bool sent, ) = recipient.call{value: REWARD}("");
require(sent, "ETH_TRANSFER_FAILED");
​
emit Claimed(treasureHash, msg.sender);
}

Risk

Likelihood:

  • High: A malicious player that generated a valid ZK proof for a valid treasure secret can repeatedly call the claim function with the same parameters MAX_TREASURES of times,
    claiming each time the value of the reward for the same secret, which should not be allowed by the protocol

Impact:

  • High: A malicious user can steal (MAX_TREASURES - 1) * REWARD of funds from the contract with just one ZK proof (one found secret)


Proof of Concept

  1. A malicious player generates a valid ZK proof for a valid treasure secret

  2. The malicious user repeatedly calls the claim function with the same secret's ZK proof and treasureHashMAX_TREASURES of times,
    claiming each time the value of the reward for the same secret, which should not be allowed

function testClaimMaxTreasureOfTimesWithTheSameSecretProof() public {
(bytes memory proof, bytes32 treasureHash, address payable recipient) = _loadFixture();
uint256 balanceBefore = recipient.balance;
for (uint256 i = 0; i < hunt.MAX_TREASURES(); i++) {
hunt.claim(proof, treasureHash, recipient);
}
​
uint256 balanceAfter = recipient.balance;
uint256 totalRewards = hunt.MAX_TREASURES() * hunt.REWARD();
assertEq(balanceBefore + totalRewards, balanceAfter);
}

Recommended Mitigation

To prevent this protocol exploit, fix the claimed check by replacing the `_treasureHash` storage variable with the correct `treasureHash` function parameter

- if (claimed[_treasureHash]) revert AlreadyClaimed(treasureHash);
+ if (claimed[treasureHash]) revert AlreadyClaimed(treasureHash);

Support

FAQs

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

Give us feedback!