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

(LOW) `onlyOwner` modifier is declared but never applied; every admin function re-implements the check inline

snavohburmaa

Location: contracts/src/TreasureHunt.sol:53-56

Description

The modifier is declared but unused

modifier onlyOwner() {
require(msg.sender == owner, "ONLY_OWNER");
_;
}

Instead, fund, pause, unpause, updateVerifier, and emergencyWithdraw each check againmsg.sender == owner inline with a little bit different error strings, duplicating the logic five times.

Risk

Likelihood: Low the check is present

Impact: Low

Proof of Concept

Grepping the contract shows the duplication clearly:

grep -n "msg.sender == owner\|msg.sender==owner" contracts/src/TreasureHunt.sol

Expected output:

54: require(msg.sender == owner, "ONLY_OWNER");
70: owner = msg.sender;
89: if (msg.sender == owner) revert OwnerCannotClaim();
237: require(msg.sender==owner, "ONLY_OWNER_CAN_FUND");
246: require(msg.sender == owner, "ONLY_OWNER_CAN_PAUSE");
255: require(msg.sender == owner, "ONLY_OWNER_CAN_UNPAUSE");
265: require(msg.sender == owner, "ONLY_OWNER_CAN_UPDATE_VERIFIER");
275: require(msg.sender == owner, "ONLY_OWNER_CAN_EMERGENCY_WITHDRAW");

Five redundant copies of the same check outside the declared modifier.

Recommended Mitigation

Apply the modifier to every admin function and remove the inline checks

- function fund() external payable {
- require(msg.sender==owner, "ONLY_OWNER_CAN_FUND");
+ function fund() external payable onlyOwner {
require(msg.value > 0, "NO_ETH_SENT");
emit Funded(msg.value, address(this).balance);
}

Do the same for pause, unpause, updateVerifier, emergencyWithdraw, and withdraw.

Updates

Lead Judging Commences

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

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!