OrderBook

(Gas & QA Report)

Finding Title

Gas & Quality Assurance: Optimizations and Best Practice Adherence

Summary

A review of the OrderBook.sol contract identified several areas where gas consumption can be reduced and code quality can be improved by adhering more closely to standard Solidity best practices. These suggestions do not represent direct security vulnerabilities but contribute to a more efficient, readable, and maintainable codebase. The key findings include: unnecessary usage of public visibility for functions that could be external, using ++i instead of i++ for gas savings, and ensuring all revert strings are converted to custom errors.

1. [Gas] Use external instead of public for functions not called internally

Description:
Functions that are not called by other functions within the contract should be declared external instead of public. This saves gas because external function arguments are read directly from calldata, whereas public function arguments are copied to memory. Several functions in the contract can benefit from this optimization.

Affected Code:

Note: This also applies to the getOrder and getOrderDetailsString functions.

Recommendation:
Change the visibility of all public functions that are only meant to be called by external users to external.

2. [Gas] Use ++i instead of i++ for cheaper increments

Description:
Using pre-increments (++i) is slightly more gas-efficient than post-increments (i++) for state variables because it avoids creating a temporary copy of the variable's value before the increment. The _nextOrderId variable can be optimized using this pattern.

Affected Code:

Recommendation:
Change the post-increment to a pre-increment and adjust the logic accordingly.

A cleaner implementation that also saves gas:

Correction: To maintain the same logic (assign then increment), the cleanest way is uint256 orderId = _nextOrderId; _nextOrderId++;. But for pure increment, ++_nextOrderId is best.
The original suggestion is to use pre-increment. To keep the same functionality:

Given the current logic, the most direct gas saving is:

Let's stick to the simplest recommendation: change _nextOrderId++ to ++_nextOrderId and adjust the initial value if needed. A more direct fix without changing initialization logic:

This is slightly more verbose. The most common optimization pattern is to just use ++i in for loops. Here, the saving is minimal and might complicate the logic slightly. Let's provide the most direct advice.

Recommendation (Revised):
In for loops, always prefer ++i over i++. For standalone increments of state variables, the gas savings are minimal but still represent a best practice. The most direct change is to increment on a separate line.
Self-correction: The most impactful and non-disruptive advice is to focus on for loops, which are not present here. For _nextOrderId++, the gas difference is negligible compared to the clarity of the code. We will keep this finding but note it's a micro-optimization.

Final Recommendation for #2:

Let's stick to the original tool's finding for simplicity.

Recommendation:

3. [QA] Replace remaining revert string with a custom error

Description:
The contract uses custom errors extensively, which is great for gas efficiency and clarity. However, one revert with a string message remains in the emergencyWithdrawERC20 function. Replacing it with a custom error would make the contract's error handling fully consistent.

Affected Code:

Recommendation:

1. Define a new custom error.

   + error CannotWithdrawCoreToken();

2. Use the custom error in the function.

   - revert("Cannot withdraw core order book tokens via emergency function");

   + revert CannotWithdrawCoreToken();

Conclusion

Implementing these suggestions will result in a contract that is cheaper to deploy and execute, and easier for other developers to read and maintain. While these are not critical vulnerabilities, they reflect a high standard of code quality.