This code review highlights Three areas for improvement: 1) Record both old and new values for critical parameter changes without unnecessary caching to reduce memory usage and potentially improve code readability; 2) Prioritize error over require and assert for more gas-efficient error handling.3) Consider locking pragma versions in LiquidationPool and LiquidationPoolManager contracts for consistency and security.
Record both old and new values: When a critical parameter is modified, it is recommended to record both its previous state and its updated value. This creates a detailed change history, enabling accurate tracking of parameter modifications, informed analysis of change patterns and potential rollback to a previous state if required.
Eliminate unnecessary caching: When the old value is only required for immediate actions (e.g., event emission), access it directly within the setter to streamline code and potentially reduce memory overhead. This optimization can improve code readability and potentially reduce memory usage.
LiquidationPoolManager::setPoolFeePercentage
All other instances entailed:
SmartVaultManagerV5::setMintFeeRate, setBurnFeeRate, setSwapFeeRate
In Solidity, there are three ways to throw an exception: error, require, and assert. error is the most gas-efficient, followed by assert, and require is the most gas-consuming. Therefore, error is a good choice for both providing informative error messages to users and saving gas.
Error statements can provide more detailed error messages than require statements. This can be helpful for developers debugging their contracts or users troubleshooting problems. It can be used to handle errors more gracefully than require statements and it can be more gas-efficient than require statements.
All other instances entailed:
SmartVaultV3::onlyManager, onlyVaultManager, ifMinted, ifNotLiquidated
SmartVaultV3ManagerV5::onlyLiquidator
LiquidationPool and Liquidation Pool ManagerBoth LiquidationPool and Liquidation Pool Manager contracts are floating the pragma version. Consider locking the pragma versions to ensure consistent compilation and avoid potential issues with outdated compiler features or vulnerabilities.
If these contracts are specifically designed for use by other developers (e.g., as libraries or packages), floating pragmas might be acceptable to accommodate different project setups.
LiquidationPool
Liquidation Pool Manager
Limited Tracing and Analysis: Without capturing both old and new values, accurate tracking of parameter changes, analyzing trends, and diagnosing issues becomes challenging. Reconstructing previous states becomes problematic in case of needed rollbacks, leading to delays, inaccuracies, or even an inability to revert.
Inefficient Error Handling: Reliance on require might incur higher gas costs and less informative error messages.
Security Concerns: Floating pragmas could expose contracts to vulnerabilities associated with outdated compiler versions. For libraries and packages, floating pragmas might be acceptable to allow project-specific compiler choice and seamless integration.
Manual
Capture Critical Changes by Optimized Memory Usage: Capture both old and new values for critical parameters by directly accessing old values within setter functions when feasible.
Prioritize error for exception handling: Utilize error statements for informative messages and potential gas savings.
Review pragma versions: Assess locking or floating pragmas based on contract usage and security considerations.
The contest is live. Earn rewards by submitting a finding.
This is your time to appeal against judgements on your submissions.
Appeals are being carefully reviewed by our judges.