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

Unused onlyOwner modifier and inconsistent access control pattern

Author Revealed upon completion

Description

  • The contract defines an onlyOwner modifier but does not use it. Instead, access control checks are implemented inline via repeated require(msg.sender == owner, ...) statements across multiple functions.

  • This may lead to code duplication, reduced readability, and potential for inconsistency in future modifications.

// @> onlyOwner modifier is defined but never applied to any function
modifier onlyOwner() {
require(msg.sender == owner, "ONLY_OWNER");
_;
}
​
// @> Instead, access control is duplicated inline across all restricted functions
function pause() external {
require(msg.sender == owner, "ONLY_OWNER_CAN_PAUSE"); // @> inline check
paused = true;
}
​
function unpause() external {
require(msg.sender == owner, "ONLY_OWNER_CAN_UNPAUSE"); // @> inline check
paused = false;
}
​
function fund() external payable {
require(msg.sender == owner, "ONLY_OWNER_CAN_FUND"); // @> inline check
require(msg.value > 0, "NO_ETH_SENT");
}

Risk

Likelihood:

  • The unused modifier and duplicated inline checks are present across every restricted function in the contract, making this a consistent pattern throughout the entire codebase rather than an isolated occurrence.

  • Any future modification to access control logic will require updating every inline require statement individually, increasing the likelihood of an inconsistency being introduced.

Impact:

  • Code duplication across multiple functions increases maintenance burden and the risk of inconsistency in future modifications.

  • Reduced readability makes it harder for auditors and developers to reason about the access control model of the contract.

Proof of Concept

// The onlyOwner modifier is unused while inline require statements are repeated across:
// - pause()
// - unpause()
// - fund()
// - emergencyWithdraw()
// - updateVerifier()

Recommended Mitigation

Either use the onlyOwner modifier consistently across all restricted functions, or remove the unused modifier entirely.

function updateVerifier(IVerifier newVerifier) external {
require(paused, "THE_CONTRACT_MUST_BE_PAUSED");
- require(msg.sender == owner, "ONLY_OWNER_CAN_UPDATE_VERIFIER");
verifier = newVerifier;
​
emit VerifierUpdated(address(newVerifier));
}
​
+ function updateVerifier(IVerifier newVerifier) external onlyOwner {
require(paused, "THE_CONTRACT_MUST_BE_PAUSED");
verifier = newVerifier;
​
emit VerifierUpdated(address(newVerifier));
}
​
​
function emergencyWithdraw(address payable recipient, uint256 amount) external {
require(paused, "THE_CONTRACT_MUST_BE_PAUSED");
- require(msg.sender == owner, "ONLY_OWNER_CAN_EMERGENCY_WITHDRAW");
require(recipient != address(0) && recipient != address(this) && recipient != owner, "INVALID_RECIPIENT");
require(amount > 0 && amount <= address(this).balance, "INVALID_AMOUNT");
​
(bool sent,) = recipient.call{value: amount}("");
require(sent, "ETH_TRANSFER_FAILED");
​
emit EmergencyWithdraw(recipient, amount);
}
​
+ function emergencyWithdraw(address payable recipient, uint256 amount) external onlyOwner{
require(paused, "THE_CONTRACT_MUST_BE_PAUSED");
require(recipient != address(0) && recipient != address(this) && recipient != owner, "INVALID_RECIPIENT");
require(amount > 0 && amount <= address(this).balance, "INVALID_AMOUNT");
​
(bool sent,) = recipient.call{value: amount}("");
require(sent, "ETH_TRANSFER_FAILED");
​
emit EmergencyWithdraw(recipient, amount);
}
​
function unpause() external {
- require(msg.sender == owner, "ONLY_OWNER_CAN_UNPAUSE");
paused = false;
​
emit Unpaused(msg.sender);
}
​
+ function unpause() external onlyOwner {
paused = false;
​
emit Unpaused(msg.sender);
}
​
function pause() external {
- require(msg.sender == owner, "ONLY_OWNER_CAN_PAUSE");
paused = true;
​
emit Paused(msg.sender);
}
​
+ function pause() external onlyOwner {
paused = true;
​
emit Paused(msg.sender);
}
​
function fund() external payable {
- require(msg.sender == owner, "ONLY_OWNER_CAN_FUND");
require(msg.value > 0, "NO_ETH_SENT");
​
emit Funded(msg.value, address(this).balance);
}
​
+ function fund() external onlyOwner payable {
require(msg.value > 0, "NO_ETH_SENT");
​
emit Funded(msg.value, address(this).balance);
}

Support

FAQs

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

Give us feedback!