OrderBook
First Flight #43
Beginner FriendlySolidity
100 EXP
View results
Previous Next
Submission Details
Impact: low
Likelihood: low
Invalid

Inconsistent Error Handling + Increased Gas Costs

blee

Description

  • The contract should use consistent error handling patterns throughout to optimize gas usage and maintain code quality.

  • The emergencyWithdrawERC20 function uses string-based revert messages while all other functions use custom errors, creating inconsistency and higher gas costs.

// Consistent custom errors used elsewhere
error OrderNotFound();
error InvalidToken();
error InvalidAmount();
​
function emergencyWithdrawERC20(address _tokenAddress, uint256 _amount, address _to) external onlyOwner {
if (_tokenAddress == address(iWETH) || _tokenAddress == address(iWBTC) || _tokenAddress == address(iWSOL) || _tokenAddress == address(iUSDC)) {
revert("Cannot withdraw core order book tokens via emergency function");
}
// ...
}

Risk

Likelihood:

  • Every time the emergency withdrawal function is called with core tokens

  • When users or tools parse error messages for different patterns

  • During contract interactions where gas optimization matters

Impact:

  • Higher gas costs due to string storage and processing

  • Inconsistent error handling makes debugging more difficult

  • Code maintainability decreases due to mixed error patterns

Proof of Concept

Gas Cost Comparison: This demonstrates the gas difference between string and custom error handling.

function testInconsistentErrorHandling() public {
vm.startPrank(owner);
// Custom error (more gas efficient)
vm.expectRevert(OrderBook.InvalidAddress.selector);
book.withdrawFees(address(0));
// String error (less gas efficient)
vm.expectRevert("Cannot withdraw core order book tokens via emergency function");
book.emergencyWithdrawERC20(address(usdc), 1000e6, owner);
vm.stopPrank();
}

Cost Analysis:

  • Custom errors: ~4 bytes for error selector + parameters (minimal gas)

  • String errors: ~68 bytes for the long string message (significantly higher gas)

  • Deployment cost: String errors increase contract size and deployment costs

  • Runtime cost: String errors cost more to revert with due to longer data

  • Tooling integration: Mixed error patterns make automated testing and monitoring harder

Recommended Mitigation

+ error CannotWithdrawCoreTokens();
​
function emergencyWithdrawERC20(address _tokenAddress, uint256 _amount, address _to) external onlyOwner {
if (_tokenAddress == address(iWETH) || _tokenAddress == address(iWBTC) || _tokenAddress == address(iWSOL) || _tokenAddress == address(iUSDC)) {
- revert("Cannot withdraw core order book tokens via emergency function");
+ revert CannotWithdrawCoreTokens();
}
// ...
}

Why this works:

  • Consistency: Aligns with the existing custom error pattern used throughout the contract

  • Gas efficiency: Saves ~64 bytes of string data, reducing deployment and runtime costs

  • Maintainability: Easier to manage and update error messages from a central location

  • Tooling compatibility: Better integration with error handling and monitoring systems

Additional considerations:

  • Consider using error parameters if more context is needed: error CannotWithdrawCoreTokens(address token)

  • Document all custom errors in NatSpec comments for better developer experience

Updates

Lead Judging Commences

yeahchibyke Lead Judge 5 days ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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