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

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

Author Revealed upon completion

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.

Support

FAQs

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

Give us feedback!