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

Inconsistent Error Handling Pattern

Author Revealed upon completion

Root + Impact

Description

  • The contract should use custom errors or string-based require statements consistently throughout the contract

  • The contract mixes custom errors with string-based require statements inconsistently. For example:

    • Line 54: require(msg.sender == owner, "ONLY_OWNER"); uses string

    • Line 84: if (paused) revert ContractPaused(); uses custom error

// contracts/src/TreasureHunt.sol throughout
​

Risk

Likelihood:

  • Reason 1

  • Reason 2

Impact:

  • Impact 1: Inconsistent error handling makes the contract harder to maintain

  • Impact 2: String errors are more expensive in terms of gas than custom errors

  • Some custom errors defined are never used

Proof of Concept

Some custom errors defined are never used:

// contracts/src/TreasureHunt.sol
contract TreasureHunt {
// ----- errors -----
error AlreadyClaimed(bytes32 treasureHash);
error InvalidProof();
error NotEnoughFunds();
error InvalidRecipient();
error AllTreasuresClaimed();
error OwnerCannotClaim();
error OwnerCannotBeRecipient(); //NOT used
error InvalidVerifier();
error HuntNotOver(); //NOT used
error NoFundsToWithdraw();
error OnlyOwnerCanFund(); //NOT used
error OnlyOwnerCanPause(); //NOT used
error OnlyOwnerCanUnpause(); //NOT used
error ContractPaused(); //NOT used
error TheContractMustBePaused(); //NOT used
error OnlyOwnerCanUpdateVerifier(); //NOT used
error OnlyOwnerCanEmergencyWithdraw(); //NOT used
error InvalidAmount(); //NOT used
​
// ----- constants -----
uint256 public constant REWARD = 10 ether;
uint256 public constant MAX_TREASURES = 10;
​
// ----- immutable config -----
IVerifier private verifier;
address private immutable owner;
bytes32 private immutable _treasureHash;
​
// ----- state -----
mapping(bytes32 => bool) public claimed;
uint256 public claimsCount;
bool private paused;
bool private locked;
​
// ----- events -----
event Claimed(bytes32 indexed treasureHash, address indexed recipient);
event Funded(uint256 amount, uint256 newBalance);
event Withdrawn(uint256 amount, uint256 newBalance);
event VerifierUpdated(address indexed newVerifier);
event EmergencyWithdraw(address indexed recipient, uint256 amount);
event Paused(address indexed by);
event Unpaused(address indexed by);
​
// ----- modifiers -----
modifier onlyOwner() {
require(msg.sender == owner, "ONLY_OWNER");
_;
}

Recommended Mitigation

Change error handling to use custom errors defined.

// @contracts/src/TreasureHunt.withdraw()
function withdraw() external {
- require(claimsCount >= MAX_TREASURES, "HUNT_NOT_OVER");
+ if(claimsCount < MAX_TREASURES) revert HuntNotOver();
​
// @contracts/src/TreasureHunt.pause()
function pause() external {
- require(msg.sender == owner, "ONLY_OWNER_CAN_PAUSE");
+ if (msg.sender != owner) revert OnlyOwnerCanPause();
​
​
// @contracts/src/TreasureHunt.fund()
function fund() external payable {
- require(msg.sender==owner, "ONLY_OWNER_CAN_FUND");
+ if (msg.sender != owner) revert OnlyOwnerCanFund();
​
// other similar changes for:
// OnlyOwnerCanUnpause()
// TheContractMustBePaused()
// OnlyOwnerCanUpdateVerifier()
// OnlyOwnerCanEmergencyWithdraw()
​

Support

FAQs

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

Give us feedback!