Ethos Reserve contest - delfin454000's results

Summary

Low risk findings

Non-critical findings

Low risk findings

Below is an example of valid use of assert in Ethos Reserve:

TroveManager.sol: L1219-1224

            * The following assert() holds true because:
            * - The system always contains >= 1 trove
            * - When we close or liquidate a trove, we redistribute the pending rewards, so if all troves were closed/liquidated,
            * rewards would’ve been emptied and totalCollateralSnapshot would be zero too.
            */
            assert(totalStakesSnapshot[_collateral] > 0);

On the other hand, the following assert function does not to meet the criteria and should be replaced by a require:

BorrowerOperations.sol: L128

        assert(MIN_NET_DEBT > 0);

Similarly for the following assert functions:

BorrowerOperations.sol: L197

BorrowerOperations.sol: L301

BorrowerOperations.sol: L331

TroveManager.sol: L417

TroveManager.sol: L1279

TroveManager.sol: L1342

TroveManager.sol: L1348

TroveManager.sol: L1414

TroveManager.sol: L1489

StabilityPool.sol: L526

StabilityPool.sol: L551

StabilityPool.sol: L591

LUSDToken.sol: L312

LUSDToken.sol: L313

LUSDToken.sol: L321

LUSDToken.sol: L329

LUSDToken.sol: L337

LUSDToken.sol: L338

---

---

---

CollateralConfig.sol: L78-100

    // Admin function to lower the collateralization requirements for a particular collateral.
    // Can only lower, not increase.
    //
    // !!!PLEASE USE EXTREME CARE AND CAUTION. THIS IS IRREVERSIBLE!!!
    //
    // You probably don't want to do this unless a specific asset has proved itself through tough times.
    // Doing this irresponsibly can permanently harm the protocol.
    function updateCollateralRatios(
        address _collateral,
        uint256 _MCR,
        uint256 _CCR
    ) external onlyOwner checkCollateral(_collateral) {
        Config storage config = collateralConfig[_collateral];
        require(_MCR <= config.MCR, "Can only walk down the MCR");
        require(_CCR <= config.CCR, "Can only walk down the CCR");

        require(_MCR >= MIN_ALLOWED_MCR, "MCR below allowed minimum");
        config.MCR = _MCR;

        require(_CCR >= MIN_ALLOWED_CCR, "CCR below allowed minimum");
        config.CCR = _CCR;
        emit CollateralRatiosUpdated(_collateral, _MCR, _CCR);
    }

---

---

Non-critical findings

Below, function _liquidateNormalMode defines named return variable singleLiquidation

TroveManager.sol: L321-329

    function _liquidateNormalMode(
        IActivePool _activePool,
        IDefaultPool _defaultPool,
        address _collateral,
        address _borrower,
        uint _LUSDInStabPool
    )
        internal
        returns (LiquidationValues memory singleLiquidation)

An unnecesssary return statement for singleLiquidation is given at the end of the function:

TroveManager.sol: L353-355

        return singleLiquidation;
    }

Similarly for function `_liquidateRecoveryMode':

TroveManager.sol: L436-438

        return singleLiquidation;
    }

---

Below, function _addLiquidationValuesToTotals defines named return variable newTotals

TroveManager.sol: L898-899

    function _addLiquidationValuesToTotals(LiquidationTotals memory oldTotals, LiquidationValues memory singleLiquidation)
    internal pure returns(LiquidationTotals memory newTotals) {

An unnecesssary return statement for newTotals is given at the end of the function:

TroveManager.sol: L912-914

        return newTotals;
    }

---

Below, function _addTroveOwnerToArray defines named return variable index

TroveManager.sol: L1321

    function _addTroveOwnerToArray(address _borrower, address _collateral) internal returns (uint128 index) {

An unnecesssary return statement for index is given at the end of the function:

TroveManager.sol: L1332-1334

        return index;
    }

---

Below, function _computeRewardsPerUnitStaked defines named return variables collGainPerUnitStaked and LUSDLossPerUnitStaked

StabilityPool.sol: L504-511

    function _computeRewardsPerUnitStaked(
        address _collateral,
        uint _collToAdd,
        uint _debtToOffset,
        uint _totalLUSDDeposits
    )
        internal
        returns (uint collGainPerUnitStaked, uint LUSDLossPerUnitStaked)

An unnecesssary return statement for these variables is given at the end of the function:

StabilityPool.sol: L543-545

        return (collGainPerUnitStaked, LUSDLossPerUnitStaked);
    }

---

---

ReaperVaultV2e.sol: L73-76

    bytes32 public constant DEPOSITOR = keccak256("DEPOSITOR");
    bytes32 public constant STRATEGIST = keccak256("STRATEGIST");
    bytes32 public constant GUARDIAN = keccak256("GUARDIAN");
    bytes32 public constant ADMIN = keccak256("ADMIN");

---

ReaperBaseStrategyv4.sol: L49-52

    bytes32 public constant KEEPER = keccak256("KEEPER");
    bytes32 public constant STRATEGIST = keccak256("STRATEGIST");
    bytes32 public constant GUARDIAN = keccak256("GUARDIAN");
    bytes32 public constant ADMIN = keccak256("ADMIN");

---

TroveManager.sol: L14

// import "./Dependencies/Ownable.sol";

Recommendation:

import "./Dependencies/Ownable.sol";

---

StabilityPool.sol: L5-8

import './Interfaces/IBorrowerOperations.sol';
import "./Interfaces/ICollateralConfig.sol";
import './Interfaces/IStabilityPool.sol';
import './Interfaces/IBorrowerOperations.sol';

Recommendation: Remove duplicate import of IBorrowerOperations.sol

---

Below are examples of extra-long comments (over 120 characters) whose readability could be improved by wrapping, as shown. Note that a small indentation is used in each continuation line (which flags for the reader that the comment is ongoing), along with a period at the close (to signal the end of the comment). Also, if an extra-long line contains both code and a comment, it makes sense to put the comment in the line above.

---

BorrowerOperations.sol: L300

        // Confirm the operation is either a borrower adjusting their own trove, or a pure collateral transfer from the Stability Pool to a trove

Suggestion:

        // Confirm the operation is either a borrower adjusting their own trove, 
        //   or a pure collateral transfer from the Stability Pool to a trove.

---

TroveManager.sol: L102

    * Where L_Collateral(0) and L_LUSDDebt(0) are snapshots of L_Collateral and L_LUSDDebt for the active Trove taken at the instant the stake was made

Suggestion:

    * Where L_Collateral(0) and L_LUSDDebt(0) are snapshots of L_Collateral and
    *   L_LUSDDebt for the active Trove taken at the instant the stake was made.

---

[TroveManager.sol: L934-937(https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/TroveManager.sol#L934-L937)

    * The redeemer swaps (debt - liquidation reserve) LUSD for (debt - liquidation reserve) worth of collateral, so the LUSD liquidation reserve left corresponds to the remaining debt.
    * In order to close the trove, the LUSD liquidation reserve is burned, and the corresponding debt is removed from the active pool.
    * The debt recorded on the trove's struct is zero'd elswhere, in _closeTrove.
    * Any surplus collateral left in the trove, is sent to the Coll surplus pool, and can be later claimed by the borrower.

Suggestion:

    * The redeemer swaps (debt - liquidation reserve) LUSD for (debt - liquidation reserve) worth of collateral,
    *   so the LUSD liquidation reserve left corresponds to the remaining debt.
    * In order to close the trove, the LUSD liquidation reserve is burned,
    *   and the corresponding debt is removed from the active pool.
    * The debt recorded on the trove's struct is zero'd elsewhere, in _closeTrove.
    * Any surplus collateral left in the trove, is sent to the Coll surplus pool,
    *   and can be later claimed by the borrower.

---

TroveManager.sol: L1044

    // Return the nominal collateral ratio (ICR) of a given Trove, without the price. Takes a trove's pending coll and debt rewards from redistributions into account.

Suggestion:

    // Return the nominal collateral ratio (ICR) of a given Trove, without the price —
    //   takes a trove's pending coll and debt rewards from redistributions into account.

---

ReaperVaultERC4626.sol: L207

        shares = previewWithdraw(assets); // previewWithdraw() rounds up so exactly "assets" are withdrawn and not 1 wei less

Suggestion:

        // previewWithdraw() rounds up so exactly "assets" are withdrawn and not 1 wei less
        shares = previewWithdraw(assets);

---

Similarly for the extra-long comment lines referenced below:

BorrowerOperations.sol

L276, L510, L588

TroveManager.sol

L58, L97, L114, L995-1014

L1053, L1221, L1296, L1303

L1323, L1336, L1372

ActivePool.sol

L287

StabilityPool.sol

L25, L27-35, L42, L45-46, L52

L55, L58-59, L65, L68-69, L71

L74-75, L80, L83, L89, L92-94

L99-103, L109, L130, L136, L142

L189, L205, L217, L220, L319

L361, L553, L659, L762-763

LUSDToken.sol

L22, L25

ReaperVaultERC4626.sol

L50, L65, L71-72, L76, L85-86

L89, L93-94, L103, L119, L130, L136

L159, L163, L180, L182, L214

---

---

LUSDToken.sol: L21

* 1) Transfer protection: blacklist of addresses that are invalid recipients (i.e. core Liquity contracts) in external 

Suggestion: Change blacklist of to disallow

---

CollateralConfig.sol: L11

 * Houses whitelist of allowed collaterals (ERC20s) for the entire system. Also provides access to collateral-specific

Suggestion: Change whitelist to allowlist

---

Similarly for the following instances of whitelisted:

CollateralConfig.sol: L37

CollateralConfig.sol: L72

---

---

CollateralConfig.sol: L54

        require(_CCRs.length == _collaterals.length, "Array lenghts must match");

Change lenghts to lengths

---

TroveManager.sol: L936

    * The debt recorded on the trove's struct is zero'd elswhere, in _closeTrove.

Change elswhere to elsewhere

---

TroveManager.sol: L1155

        * this indicates that rewards have occured since the snapshot was made, and the user therefore has

Change occured to occurred

---

ActivePool.sol: L157

    *Not necessarily equal to the the contract's raw collateral balance - collateral can be forcibly sent to contracts.

Remove repeated word the

---

ReaperBaseStrategyv4.sol: L37

     * {KEEPER} - Stricly permissioned trustless access for off-chain programs or third party keepers.

Change Stricly to Strictly

---

ReaperBaseStrategyv4.sol: L98

        require(_amount <= balanceOf(), "Ammount must be less than balance");

Change Ammount to Amount

---

ReaperBaseStrategyv4.sol: L185

     * @dev This function must be overriden simply for access control purposes.

Change overriden to overridden

---

ReaperBaseStrategyv4.sol: L235

     * difference is due to a realized loss, or if there is some other sitution at play

Change sitution to situation

---

BorrowerOperations.sol: L432-444

    function _getUSDValue(uint _coll, uint _price, uint256 _collDecimals) internal pure returns (uint) {
        uint usdValue = _price.mul(_coll).div(10**_collDecimals);

        return usdValue;
    }

    function _getCollChange(
        uint _collReceived,
        uint _requestedCollWithdrawal
    )
        internal
        pure
        returns(uint collChange, bool isCollIncrease)

The returns spacing should be corrected for the following:

BorrowerOperations.sol: L444

TroveManager.sol: L593

TroveManager.sol: L682

TroveManager.sol: L795

TroveManager.sol: L875

TroveManager.sol: L899

---

BorrowerOperations.sol: L632-637

    function _requireAtLeastMinNetDebt(uint _netDebt) internal pure {
        require (_netDebt >= MIN_NET_DEBT, "BorrowerOps: Trove's net debt must be greater than minimum");
    }

    function _requireValidLUSDRepayment(uint _currentDebt, uint _debtRepayment) internal pure {
        require(_debtRepayment <= _currentDebt.sub(LUSD_GAS_COMPENSATION), "BorrowerOps: Amount repaid must not be larger than the Trove's debt");

The require spacing should be corrected for the following statements:

BorrowerOperations.sol: L633

TroveManager.sol: L1539

---

There are specific upgrades that have been applied since 0.8.9. For example, a version of at least 0.8.10 is required to have external calls skip contract existence checks if the external call has a return value, 0.8.12 is necessary for string.concat to be used instead of abi.encodePacked, and 0.8.13 or later is needed for the ability to use using for with a list of free functions. In addition, only the latest version receives security fixes.

---

ReaperVaultV2.sol: L624-631

    /**
     * @notice Only DEFAULT_ADMIN_ROLE can update treasury address.
     */
    function updateTreasury(address newTreasury) external {
        _atLeastRole(DEFAULT_ADMIN_ROLE);
        require(newTreasury != address(0), "Invalid address");
        treasury = newTreasury;
    }

Missing: @param newTreasury

---

ReaperBaseStrategyV4.sol: L90-95

    /**
     * @dev Withdraws funds and sends them back to the vault. Can only
     *      be called by the vault. _amount must be valid and security fee
     *      is deducted up-front.
     */
    function withdraw(uint256 _amount) external override returns (uint256 loss) {

Missing: @param _amount and @return

---

ReaperStrategyGranarySupplyOnly.sol: L210-216

    /**
     * @dev Attempts to safely withdraw {_amount} from the pool.
     */
    function authorizedWithdrawUnderlying(uint256 _amount) external {
        _atLeastRole(STRATEGIST);
        _withdrawUnderlying(_amount);
    }

Missing: @param _amount

---

ReaperVaultV2.sol: L219-229

    /**
     * @notice Called by a strategy to determine the amount of capital that the vault is
     * able to provide it. A positive amount means that vault has excess capital to provide
     * the strategy, while a negative amount means that the strategy has a balance owing to
     * the vault.
     */
    function availableCapital() public view returns (int256) {
        address stratAddr = msg.sender;
        if (totalAllocBPS == 0 || emergencyShutdown) {
            return -int256(strategies[stratAddr].allocated);
        }

@return is also the only missing Natspec for the following functions:

ReaperVaultV2.sol: L273-280

ReaperVaultV2.sol: L291-297

ReaperVaultV2.sol: L486-493

ReaperVaultV2.sol: L646-652

ReaperBaseStrategyV4.sol: L109

ReaperBaseStrategyV4.sol: L144

ReaperStrategyGranarySupplyOnly.sol: L218-222

---

CollateralConfig.sol

L85, L102, L106

L110, L116, L122

BorrowerOperations.sol

L110, L172, L242, L248

L254, L259, L264, L268

L375, L412, L748

TroveManager.sol

L232, L299, L303, L310, L513, L715

L939, L957, L962, L982, L1016, L1045

L1054, L1076, L1111, L1123, L1138, L1152

L1164, L1183, L1195, L1273, L1316, L1361

L1366, L1397, L1425, L1429, L1440

L1444, L1456, L1460, L1471, L1475

L1485, L1544, L1548, L1552, L1556

L1562, L1567, L1574, L1581, L1588

ActivePool.sol

L71, L125, L132, L138, L144, L159

L164, L171, L190, L197, L204, L214

StabilityPool.sol

L261, L307, L311, L323, L367, L410

L466, L486, L626, L683, L718

CommunityIssuance.sol

L61, L84, L101, L120, L124

LQTYStaking.sol

L66, L104, L142, L177

L187, L199, L213

LUSDToken.sol

L133, L141, L146, L153, L160, L185

L191, L196, L201, L206, L212

L216, L220, L226, L230, L235

L242, L247, L254, L262, L292

L399, L403, L407, L411, L415

ReaperVaultERC4626.sol

L29, L37, L51, L66, L79, L96

L110, L122, L139, L154, L165

L183, L202, L220, L240, L258

ReaperStrategyGranarySupplyOnly.sol

L147, L160, L226, L230

---

Natspec is partially missing for some constructors

ReaperVaultV2.sol: L102-118

    /**
     * @notice Initializes the vault's own 'RF' token.
     * This token is minted when someone does a deposit. It is burned in order
     * to withdraw the corresponding portion of the underlying assets.
     * @param _token the token to maximize.
     * @param _name the name of the vault token.
     * @param _symbol the symbol of the vault token.
     * @param _tvlCap initial deposit cap for scaling TVL safely
     */
    constructor(
        address _token,
        string memory _name,
        string memory _symbol,
        uint256 _tvlCap,
        address _treasury,
        address[] memory _strategists,
        address[] memory _multisigRoles

Missing: @param _treasury, @param _strategists and @param _multisigRoles

Similarly for ReaperVaultERC4626.sol's constructor:

ReaperVaultERC4626.sol: L15-23

---

TroveManager.sol: L225

CommunityIssuance.sol: L57

LUSDToken.sol: L84

---

---