Ethos Reserve contest - RaymondFam's results

Impractical upper bound limit in _requireValidMaxFeePercentage()

In _requireValidMaxFeePercentage() of BorrowerOperations.sol, the upper bound of _maxFeePercentage is DECIMAL_PRECISION, i.e. 1e18 (or 100%). However, when getBorrowingFee() is eventually invoked by _triggerBorrowingFee(), the output returned will at most be equal to MAX_BORROWING_FEE, which is 5%. As such, _requireValidMaxFeePercentage() should be refactored as follows, regardless of the minimal impact this has on _requireUserAcceptsFee(), to be more expediently in line with the logic flow entailed:

File: BorrowerOperations.sol#L648-L656

    function _requireValidMaxFeePercentage(uint _maxFeePercentage, bool _isRecoveryMode) internal pure {
        if (_isRecoveryMode) {
-            require(_maxFeePercentage <= DECIMAL_PRECISION,
                "Max fee percentage must less than or equal to 100%");
+            require(_maxFeePercentage <= 5e15,
                "Max fee percentage must less than or equal to 5%");
        } else {
-            require(_maxFeePercentage >= BORROWING_FEE_FLOOR && _maxFeePercentage <= DECIMAL_PRECISION,
                "Max fee percentage must be between 0.5% and 100%");
+            require(_maxFeePercentage >= BORROWING_FEE_FLOOR && _maxFeePercentage <= 5e15,
                "Max fee percentage must be between 0.5% and 5%");
        }
    }

Uninitialized baseRate till the first call on redeemCollateral()

The impact, though low, is twofold. First off, the output of getBorrowingFee() is always going to be BORROWING_FEE_FLOOR, i.e. 0.5% because _baseRate == 0:

File: TroveManager.sol#L1464-L1469

    function _calcBorrowingRate(uint _baseRate) internal pure returns (uint) {
        return LiquityMath._min(
            BORROWING_FEE_FLOOR.add(_baseRate),
            MAX_BORROWING_FEE
        );
    }

Next, when baseRate finally gets assigned via updateBaseRateFromRedemption(), increasing the baseRate based on the amount redeemed, as a proportion of total supply, is going to be minimally negligible unless the redemption amount is significantly sizable. And, if subsequently LUSD borrowing operations exceed redemptions, baseRate is as good as zero comparatively in [5e15, 5e16].

In another words, this makes the complexity of introducing a decaying factor pegged to a half-life of 720 minutes pretty much obsolete considering the borrowing rate stays always at 0.5%.

Consider having _calcDecayedBaseRate() refactored as follows if it is going to take a while before the first redemption is made. Better yet, return 0 if baseRate falls below a reasonable dust value:

File: TroveManager.sol#L1509-L1514

    function _calcDecayedBaseRate() internal view returns (uint) {
+        if (baseRate == 0) return 0;

        uint minutesPassed = _minutesPassedSinceLastFeeOp();
        uint decayFactor = LiquityMath._decPow(MINUTE_DECAY_FACTOR, minutesPassed);

        return baseRate.mul(decayFactor).div(DECIMAL_PRECISION);
    }

Inadequate if else checks

In BorrowerOperations.sol, the else clauses of _moveTokensAndCollateralfromAdjustment() are allowing _repayLUSD() and _activePool.sendCollateral() to proceed even if the respective function parameters, i.e. _LUSDChange and _collChange are 0. Specifically, addColl(), moveCollateralGainToTrove(), and withdrawColl() will have _LUSDChange inputted as 0 whereas withdrawLUSD() and repayLUSD() will have zero _collChange associated. Consider having the affected function refactored as follows to stem futile executions entailing zero repayment and collateral sending:

File: BorrowerOperations.sol#L476-L502

    function _moveTokensAndCollateralfromAdjustment
    (
        IActivePool _activePool,
        ILUSDToken _lusdToken,
        address _borrower,
        address _collateral,
        uint _collChange,
        bool _isCollIncrease,
        uint _LUSDChange,
        bool _isDebtIncrease,
        uint _netDebtChange
    )
        internal
    {
        if (_isDebtIncrease) {
            _withdrawLUSD(_activePool, _lusdToken, _collateral, _borrower, _LUSDChange, _netDebtChange);
        } else {
-            _repayLUSD(_activePool, _lusdToken, _collateral, _borrower, _LUSDChange);
+            if (_LUSDChange != 0) _repayLUSD(_activePool, _lusdToken, _collateral, _borrower, _LUSDChange);
        }

        if (_isCollIncrease) {
            IERC20(_collateral).safeTransferFrom(msg.sender, address(this), _collChange);
            _activePoolAddColl(_activePool, _collateral, _collChange);
        } else {
-            _activePool.sendCollateral(_collateral, _borrower, _collChange);
+            if (_collChange != 0) _activePool.sendCollateral(_collateral, _borrower, _collChange);
        }
    }

Use a more recent version of solidity

The protocol adopts version 0.6.11 on writing some of the smart contracts. For better security, it is best practice to use the latest Solidity version, 0.8.17.

Security fix list where the protocol could benefit from, e.g. SafeMath in the versions can be found in the link below:

https://github.com/ethereum/solidity/blob/develop/Changelog.md

Here are the contract instances entailed:

File: CollateralConfig.sol#L3 File: BorrowerOperations.sol#L3 File: TroveManager.sol#L3 File: ActivePool.sol#L3 File: StabilityPool.sol#L3 File: CommunityIssuance.sol#L3 File: LQTYStaking.sol#L3 File: LUSDToken.sol#L3

Modularity on import usages

For cleaner Solidity code in conjunction with the rule of modularity and modular programming, use named imports with curly braces instead of adopting the global import approach.

For instance, the import instances below could be refactored conforming to most other instances in the code bases as follows:

File: CollateralConfig.sol#L5-L8

- import "./Dependencies/CheckContract.sol";
+ import {CheckContract} from "./Dependencies/CheckContract.sol";
- import "./Dependencies/Ownable.sol";
+ import {Ownable} from "./Dependencies/Ownable.sol";
- import "./Dependencies/SafeERC20.sol";
+ import {SafeERC20} from "./Dependencies/SafeERC20.sol";
- import "./Interfaces/ICollateralConfig.sol";
+ import {ICollateralConfig} from "./Interfaces/ICollateralConfig.sol";

Inadequate NatSpec

Solidity contracts can use a special form of comments, i.e., the Ethereum Natural Language Specification Format (NatSpec) to provide rich documentation for functions, return variables and more. Please visit the following link for further details:

https://docs.soliditylang.org/en/v0.8.16/natspec-format.html

Consider equipping all contracts with complete set of NatSpec to better facilitate users/developers interacting with the protocol's smart contracts.

Return statement on named returns

Functions with named returns should not have a return statement to avoid unnecessary confusion.

For instance, the following _getTCR() may be refactored as follows:

    function _getTCR(address _collateral, uint _price, uint256 _collateralDecimals) internal view returns (uint TCR) {
        uint entireSystemColl = getEntireSystemColl(_collateral);
        uint entireSystemDebt = getEntireSystemDebt(_collateral);

        TCR = LiquityMath._computeCR(entireSystemColl, entireSystemDebt, _price, _collateralDecimals);

-        return TCR;
    }

Lines too long

Lines in source code are typically limited to 80 characters, but it’s reasonable to stretch beyond this limit when need be as monitor screens theses days are comparatively larger. Considering the files will most likely reside in GitHub that will have a scroll bar automatically kick in when the length is over 164 characters, all code lines and comments should be split when/before hitting this length. Keep line width to max 120 characters for better readability where possible.

Here are some of the instances entailed:

File: BorrowerOperations.sol#L172 File: BorrowerOperations.sol#L268 File: BorrowerOperations.sol#L282 File: BorrowerOperations.sol#L343 File: BorrowerOperations.sol#L511

Inadequate checks in initialize() of CollateralConfig.sol

When initializing config struct variables, the function does not check if _MCRs[i] is smaller than _CCRs[i]. Consider having the function refactored as follows to avoid input error considering these state variables can only be initialized once:

File: CollateralConfig.sol#L66-L71

            require(_MCRs[i] >= MIN_ALLOWED_MCR, "MCR below allowed minimum");
            config.MCR = _MCRs[i];

            require(_CCRs[i] >= MIN_ALLOWED_CCR, "CCR below allowed minimum");
            config.CCR = _CCRs[i];

+            require(_CCRs[i] >= _MCRs[i], "CCR below MCR");

uint256 over uint

Across the codebase, there are numerous instances of uint, as opposed to uint256. In favor of explicitness, consider replacing all instances of uint with uint256.

Here are the contract instances entailed:

File: BorrowerOperations.sol File: TroveManager.sol File: ActivePool.sol File: StabilityPool.sol File: CommunityIssuance.sol File: LQTYStaking.sol File: LUSDToken.sol

Typo mistakes

File: BorrowerOperations.sol#L651

-                "Max fee percentage must less than or equal to 100%");
+                "Max fee percentage must be less than or equal to 100%");

Non-compliant contract layout with Solidity's Style Guide

According to Solidity's Style Guide below:

https://docs.soliditylang.org/en/v0.8.17/style-guide.html

In order to help readers identify which functions they can call, and find the constructor and fallback definitions more easily, functions should be grouped according to their visibility and ordered in the following manner:

constructor, receive function (if exists), fallback function (if exists), external, public, internal, private

And, within a grouping, place the view and pure functions last.

Additionally, inside each contract, library or interface, use the following order:

type declarations, state variables, events, modifiers, functions

Consider adhering to the above guidelines for all contract instances entailed.

Unspecific compiler version pragma

For some source-units the compiler version pragma is very unspecific, i.e. ^0.8.0. While this often makes sense for libraries to allow them to be included with multiple different versions of an application, it may be a security risk for the actual application implementation itself. A known vulnerable compiler version may accidentally be selected or security tools might fall-back to an older compiler version ending up actually checking a different EVM compilation that is ultimately deployed on the blockchain.

Avoid floating pragmas where possible. It is highly recommend pinning a concrete compiler version (latest without security issues) in at least the top-level “deployed” contracts to make it unambiguous which compiler version is being used. Rule of thumb: a flattened source-unit should have at least one non-floating concrete solidity compiler version pragma.

Here are the contract instances entailed:

File: ReaperVaultV2.sol File: ReaperVaultERC4626.sol File: ReaperBaseStrategyv4.sol File: ReaperStrategyGranarySupplyOnly.sol

Camel/snake casing variable names

Non-constant state variables should also be conventionally camel cased where possible. If snake case is preferred/adopted, consider lower casing them.

Here are some of the instances entailed:

File: TroveManager.sol

86:    mapping (address => mapping (address => Trove)) public Troves;

116:    mapping (address => address[]) public TroveOwners;

Use delete to clear variables

delete a assigns the initial value for the type to a. i.e. for integers it is equivalent to a = 0, but it can also be used on arrays, where it assigns a dynamic array of length zero or a static array of the same length with all elements reset. For structs, it assigns a struct with all members reset. Similarly, it can also be used to set an address to zero address or a boolean to false. It has no effect on whole mappings though (as the keys of mappings may be arbitrary and are generally unknown). However, individual keys and what they map to can be deleted: If a is a mapping, then delete a[x] will delete the value stored at x.

The delete key better conveys the intention and is also more idiomatic.

For instance, the a[x] = 0 instance below may be refactored as follows:

File: TroveManager.sol#L1192

-        Troves[_borrower][_collateral].stake = 0;
+        delete Troves[_borrower][_collateral].stake;

Unneeded assert()

The following assertion of vars.compositeDebt != 0 is unnecessary. Even if _LUSDAmount has accidentally been inputted as 0 making vars.LUSDFee == 0 too, _getCompositeDebt() is going to have the constant LUSD_GAS_COMPENSATION which is 10e18 added to vars.compositeDebt.

Consider removing it from openTrove() to save gas on function call and also contract deployment:

File: BorrowerOperations.sol#L197

-        assert(vars.compositeDebt > 0);

Similarly, asserting MIN_NET_DEBT greater than zero in setAddresses() of BorrowerOperations.sol is unnecessary since the constant has been assigned 90e18 in LiquityBase.sol:

File: BorrowerOperations.sol#L128

-        assert(MIN_NET_DEBT > 0);

Use smaller uint128 or smaller type and pack structs variables to use lesser amount of storage slots

As the solidity EVM works with 32 bytes, most variables work fine with less than 32 bytes and may be packed inside a struct when sitting next to each other so that they can be stored in the same slot, this saves gas when writing to storage ~2000 gas.

For instance, struct LocalVariables_OuterLiquidationFunction of TroveManager.sol may be refactored as follows:

File: TroveManager.sol#L129-L138

    struct LocalVariables_OuterLiquidationFunction {
-        uint256 collDecimals;
+        uint128 collDecimals;
+        bool recoveryModeAtStart;
-        uint256 collCCR;
-        uint256 collMCR;
+        uint128 collCCR;
+        uint128 collMCR;
-        uint price;
-        uint LUSDInStabPool;
+        uint128 price;
+        uint128 LUSDInStabPool;
-        bool recoveryModeAtStart;
-        uint liquidatedDebt;
-        uint liquidatedColl;
+        uint128 liquidatedDebt;
+        uint128 liquidatedColl;
    }

This alone will save ~8000 gas with 8 slots of storage reduced to 4 slots.

Internal function with one line code excution

Internal functions entailing only one line of code may be embedded inline with the function logic invoking it.

For instance, the following code line may be refactored as follows to save gas both on function calls and contract deployment considering _requireNonZeroDebtChange is only called once in BorrowerOperations.sol by _adjustTrove() and not anywhere else:

File: BorrowerOperations.sol#L294

-        _requireNonZeroDebtChange(_LUSDChange);
+        require(_LUSDChange > 0, "BorrowerOps: Debt increase requires non-zero debtChange");

Function logic of _requireValidMaxFeePercentage() can be simplified

In BorrowerOperations.sol, _triggerBorrowingFee() involving _maxFeePercentage will only be invoked in openTrove() and _adjustTrove() when isRecoveryMode == false. As such, only the else clause of the if else logic in _requireValidMaxFeePercentage() is needed to match up with the logic flow. The function entailed (along with the calling code lines) should be refactored as follows to save gas both on function calls and contract deployment:

File: BorrowerOperations.sol#L184 File: BorrowerOperations.sol#L293

-        _requireValidMaxFeePercentage(_maxFeePercentage, isRecoveryMode);
+        _requireValidMaxFeePercentage(_maxFeePercentage);

File: BorrowerOperations.sol#L648-L656

-    function _requireValidMaxFeePercentage(uint _maxFeePercentage, bool _isRecoveryMode) internal pure {
+    function _requireValidMaxFeePercentage(uint _maxFeePercentage) internal pure {
-        if (_isRecoveryMode) {
-            require(_maxFeePercentage <= DECIMAL_PRECISION,
-                "Max fee percentage must less than or equal to 100%");
-        } else {
            require(_maxFeePercentage >= BORROWING_FEE_FLOOR && _maxFeePercentage <= DECIMAL_PRECISION,
                "Max fee percentage must be between 0.5% and 100%");
-        }
    } 

Unused imports

Consider removing unused imports to help save gas on contract deployment.

Here is an instance entailed:

File: LUSDToken.sol#L9

- import "./Dependencies/console.sol";

Private function with embedded modifier reduces contract size

Consider having the logic of a modifier embedded through a private function to reduce contract size if need be. A private visibility that saves more gas on function calls than the internal visibility is adopted because the modifier will only be making this call inside the contract.

For instance, the modifier instance below may be refactored as follows:

File: CollateralConfig.sol#L128-L131

+    function _checkCollateral() private view {
+        require(collateralConfig[_collateral].allowed, "Invalid collateral address");
+    }

    modifier checkCollateral() {
-        require(collateralConfig[_collateral].allowed, "Invalid collateral address");
+        _checkCollateral();
        _;
    }

Use storage instead of memory for structs/arrays

A storage pointer is cheaper since copying a state struct in memory would incur as many SLOADs and MSTOREs as there are slots. In another words, this causes all fields of the struct/array to be read from storage, incurring a Gcoldsload (2100 gas) for each field of the struct/array, and then further incurring an additional MLOAD rather than a cheap stack read. As such, declaring the variable with the storage keyword and caching any fields that need to be re-read in stack variables will be much cheaper, involving only Gcoldsload for all associated field reads. Read the whole struct/array into a memory variable only when it is being returned by the function, passed into a function that requires memory, or if the array/struct is being read from another memory array/struct.

For instance, the specific example below may be refactored as follows:

File: TroveManager.sol#L722-L723

-        LocalVariables_OuterLiquidationFunction memory vars;
+        LocalVariables_OuterLiquidationFunction storage vars;
-        LiquidationTotals memory totals;
+        LiquidationTotals storage totals;

Unneeded structs

Non-user specific structs intended to be overwritten in the next function call utilizing them for caching memory variables may be removed from the contract to significantly reduce storage slots on top of enhancing gas optimization. This should be done if "CompilerError: Stack too deep" is not going to be encountered.

For instance, struct LocalVariables_openTrove and ContractsCache are neither having users mapped to them in BorrowerOperations.sol nor intended to be inputted as struct instances in function parameters. In lieu of this adoption, simply declare the struct variables separately as locally cached variables for corresponding assignments or function parameter inputs.

Non-strict inequalities are cheaper than strict ones

In the EVM, there is no opcode for non-strict inequalities (>=, <=) and two operations are performed (> + = or < + =).

As an example, the following >= inequality instance may be refactored as follows:

File: TroveManager.sol#L1503

-        if (timePassed >= SECONDS_IN_ONE_MINUTE) {
// Rationale for subtracting 1 on the right side of the inequality:
// x >= 10; [10, 11, 12, ...]
// x > 10 - 1 is the same as x > 9; [10, 11, 12 ...]
+        if (timePassed > SECONDS_IN_ONE_MINUTE - 1) {

Similarly, the <= instance below may be replaced with < as follows:

File: TroveManager.sol#L383

-        if (_ICR <= _100pct) {
// Rationale for adding 1 on the right side of the inequality:
// x <= 10; [10, 9, 8, ...]
// x < 10 + 1 is the same as x < 11; [10, 9, 8 ...]
+        if (_ICR < _100pct + 1) {

|| costs less gas than its equivalent &&

Rule of thumb: (x && y) is (!(!x || !y))

Even with the 10k Optimizer enabled: ||, OR conditions cost less than their equivalent &&, AND conditions.

As an example, the && instance below may be refactored as follows:

Note: Comment on the changes made for better readability where deemed fit.

File: TroveManager.sol#L817

-                if (vars.ICR >= vars.collMCR && vars.remainingLUSDInStabPool == 0) { continue; }
+                if (!(vars.ICR < vars.collMCR || vars.remainingLUSDInStabPool != 0)) { continue; }

Split require statements using &&

Instead of using the && operator in a single require statement to check multiple conditions, using multiple require statements with 1 condition per require statement will save 3 GAS per &&.

For instance, the following code line may be refactored as follows:

File: TroveManager.sol#L1539

-        require (TroveOwnersArrayLength > 1 && sortedTroves.getSize(_collateral) > 1);
+        require (TroveOwnersArrayLength > 1);
+        require (sortedTroves.getSize(_collateral) > 1);

Use of named returns for local variables saves gas

You can have further advantages in term of gas cost by simply using named return values as temporary local variable.

For instance, the code block below may be refactored as follows:

File: TroveManager.sol#L1397-L1423

    function updateBaseRateFromRedemption(
        uint _collateralDrawn,
        uint _price,
        uint256 _collDecimals,
        uint _totalLUSDSupply
-    ) external override returns (uint) {
+    ) external override returns (uint newBaseRate) {
        _requireCallerIsRedemptionHelper();
        uint decayedBaseRate = _calcDecayedBaseRate();

        /* Convert the drawn collateral back to LUSD at face value rate (1 LUSD:1 USD), in order to get
        * the fraction of total supply that was redeemed at face value. */
        uint redeemedLUSDFraction = 
            LiquityMath._getScaledCollAmount(_collateralDrawn, _collDecimals).mul(_price).div(_totalLUSDSupply);

        uint newBaseRate = decayedBaseRate.add(redeemedLUSDFraction.div(BETA));
        newBaseRate = LiquityMath._min(newBaseRate, DECIMAL_PRECISION); // cap baseRate at a maximum of 100%
        //assert(newBaseRate <= DECIMAL_PRECISION); // This is already enforced in the line above
        assert(newBaseRate > 0); // Base rate is always non-zero after redemption

        // Update the baseRate state variable
        baseRate = newBaseRate;
        emit BaseRateUpdated(newBaseRate);
        
        _updateLastFeeOpTime();

-        return newBaseRate;
    }

Function order affects gas consumption

The order of function will also have an impact on gas consumption. Because in smart contracts, there is a difference in the order of the functions. Each position will have an extra 22 gas. The order is dependent on method ID. So, if you rename the frequently accessed function to more early method ID, you can save gas cost. Please visit the following site for further information:

https://medium.com/joyso/solidity-how-does-function-name-affect-gas-consumption-in-smart-contract-47d270d8ac92

Activate the optimizer

Before deploying your contract, activate the optimizer when compiling using “solc --optimize --bin sourceFile.sol”. By default, the optimizer will optimize the contract assuming it is called 200 times across its lifetime. If you want the initial contract deployment to be cheaper and the later function executions to be more expensive, set it to “ --optimize-runs=1”. Conversely, if you expect many transactions and do not care for higher deployment cost and output size, set “--optimize-runs” to a high number.

module.exports = {
solidity: {
version: "0.8.0",
settings: {
  optimizer: {
    enabled: true,
    runs: 1000,
  },
},
},
};

Please visit the following site for further information:

https://docs.soliditylang.org/en/v0.5.4/using-the-compiler.html#using-the-commandline-compiler

Here's one example of instance on opcode comparison that delineates the gas saving mechanism:

for !=0 before optimization
PUSH1 0x00
DUP2
EQ
ISZERO
PUSH1 [cont offset]
JUMPI

after optimization
DUP1
PUSH1 [revert offset]
JUMPI

Disclaimer: There have been several bugs with security implications related to optimizations. For this reason, Solidity compiler optimizations are disabled by default, and it is unclear how many contracts in the wild actually use them. Therefore, it is unclear how well they are being tested and exercised. High-severity security issues due to optimization bugs have occurred in the past . A high-severity bug in the emscripten -generated solc-js compiler used by Truffle and Remix persisted until late 2018. The fix for this bug was not reported in the Solidity CHANGELOG. Another high-severity optimization bug resulting in incorrect bit shift results was patched in Solidity 0.5.6. Please measure the gas savings from optimizations, and carefully weigh them against the possibility of an optimization-related bug. Also, monitor the development and adoption of Solidity compiler optimizations to assess their maturity.