Ethos Reserve contest - 0x3b's results

Update imports

Dependencies in every contract need to be expanded. In the current implementations we see from where the things are imported, but that does not tell us what is imported. For more clearer and simpler code I suggest that you can update the imports as follows.

import "./Interfaces/ILUSDToken.sol";
import "./Interfaces/ITroveManager.sol";
import "./Dependencies/SafeMath.sol";
import "./Dependencies/CheckContract.sol";
import "./Dependencies/console.sol";

Change it to this:

import {console} from "./Dependencies/console.sol";
import {SafeMath} from "./Dependencies/SafeMath.sol";
import {ILUSDToken} from "./Interfaces/ILUSDToken.sol";
import {ITroveManager} from "./Interfaces/ITroveManager.sol";
import {CheckContract} from "./Dependencies/CheckContract.sol";

Use more modern version of solidity

In some of the contract, especially the newer ones, we see the use of sol 0.8, but in other contract we see sol 0.6.11. Newer versions are more secure and come with enhanced features, like safeMath implemented in ^0.8 . It is really important solidity contracts to be up to date, so their functionality wont be impaired but their shortcomings. Suggestion is to upgrade the current versions to at least 0.8.

No checks if the arguments are contracts

In the constructor there are no contracts checks if any of the given arguments are in fact full contract. Although it is really unlikely for those addresses not to be contract, since the developers are gonna be careful with the deployment, but at the end one mistake and the contract needs to be redeployed, thus costing enormous fees for gas.

https://github.com/code-423n4/2023-02-ethos/blob/main/Ethos-Vault/contracts/ReaperVaultV2.sol#L111

There is a great implementation on how this can be cheeked and even ethos checks it but only in some contract. in https://github.com/code-423n4/2023-02-ethos/blob/main/Ethos-Core/contracts/Dependencies/CheckContract.sol#L11

function checkContract(address _account) internal view {
    require(_account != address(0), "Account cannot be zero address");

    uint256 size;
    // solhint-disable-next-line no-inline-assembly
    assembly { size := extcodesize(_account) }
    require(size > 0, "Account code size cannot be zero");
}

Changing governance should always be 2 step procedure

Although really unlikely there could be a mistake that governance could be upgraded to a faulty or not fully functioning contract. There is a check for if the upgraded instance if it is a contract, but to be more sure that everything will turn out fine I suggest that there should be a function to allow for a new contract to claim the governance and a function in the new contract to claim it. This will prevent not fully optimized contracts to be falsely assigned to be the new governor.

https://github.com/code-423n4/2023-02-ethos/blob/main/Ethos-Core/contracts/LUSDToken.sol#L146-L157

function updateGovernance(address _newGovernanceAddress) external {
    _requireCallerIsGovernance();
    checkContract(_newGovernanceAddress); // must be a smart contract (multi-sig, timelock, etc.)
    governanceAddress = _newGovernanceAddress;
    emit GovernanceAddressChanged(_newGovernanceAddress);
}

function updateGuardian(address _newGuardianAddress) external {
    _requireCallerIsGovernance();
    checkContract(_newGuardianAddress); // must be a smart contract (multi-sig, timelock, etc.)
    guardianAddress = _newGuardianAddress;
    emit GuardianAddressChanged(_newGuardianAddress);
}

LQTY issued can not be obtained by later depositors and needs to be fixed by calling fund

issueOath can return 0 witch will lead to _triggerLQTYIssuance and _updateG not working.

As they say in the Ethos contract, they already expect that in Ethos-Core/contracts/StabilityPool.sol/L421

    * When total deposits is 0, G is not updated. In this case, the LQTY issued can not be obtained by later
    * depositors - it is missed out on, and remains in the balanceof the CommunityIssuance contract.

But the issue here is that will remain in this stuck state until someone calls fund in CommunityIssuance/L101. The only one that can call this is the owner. In the function bellow CommunityIssuance/L84, if lastIssuanceTimestamp > lastDistributionTime (this can be done if the fund is not called for more than 14 days) then the function will only emit an event and set the new lastIssuanceTimestamp to block.timestamp witch again will be larger than 14 days. In this case it will also not return any issuance since the if is preventing from issuance to be calculated.

function issueOath() external override returns (uint issuance) {
    _requireCallerIsStabilityPool();
    if (lastIssuanceTimestamp < lastDistributionTime) {
        uint256 endTimestamp = block.timestamp > lastDistributionTime ? lastDistributionTime : block.timestamp;
        uint256 timePassed = endTimestamp.sub(lastIssuanceTimestamp);
        issuance = timePassed.mul(rewardPerSecond);
        totalOATHIssued = totalOATHIssued.add(issuance);
    }

    lastIssuanceTimestamp = block.timestamp;
    emit TotalOATHIssuedUpdated(totalOATHIssued);
}

The only was for this to be fixed is the owner to call fund witch will update lastDistributionTime and it will make it larger than lastIssuanceTimestamp.

    rewardPerSecond = amount.div(distributionPeriod);
    lastDistributionTime = block.timestamp.add(distributionPeriod);
    lastIssuanceTimestamp = block.timestamp;

My suggestion is to is to also update the lastDistributionTime in issueOath and in this way the function will remain working or at least reduce the likelihood of this happening.

Unnecessary SLOAD

https://github.com/code-423n4/2023-02-ethos/blob/main/Ethos-Core/contracts/StabilityPool.sol#L422-L431

    uint totalLUSD = totalLUSDDeposits; // cached to save an SLOAD

    if (totalLUSD == 0 || _LQTYIssuance == 0) {return;}

    uint LQTYPerUnitStaked;
    LQTYPerUnitStaked =_computeLQTYPerUnitStaked(_LQTYIssuance, totalLUSD);

    uint marginalLQTYGain = LQTYPerUnitStaked.mul(P);

In the if statement, it is not necessary to check totalLUSD since totalLUSD=totalLUSDDeposits, and since it is not needed there is also no need for totalLUSD to be initialized, since it is only used in 1 place- LQTYPerUnitStaked =_computeLQTYPerUnitStaked(_LQTYIssuance, totalLUSD) to fix the code you can redo it as follows:

    if (_LQTYIssuance == 0) {return;}

    uint LQTYPerUnitStaked;
    LQTYPerUnitStaked =_computeLQTYPerUnitStaked(_LQTYIssuance, totalLUSDDeposits);

This can be even extended to the code bellow, although this is increasing complexity and lowers code readability.

uint marginalLQTYGain = _computeLQTYPerUnitStaked(_LQTYIssuance, totalLUSDDeposits).mul(P)

Use uint256(1)/uint256(2) instead of bool in structs

This prevents unnecessary gas usage, since uint is cheaper than bool https://github.com/code-423n4/2023-02-ethos/blob/main/Ethos-Core/contracts/CollateralConfig.sol#L28

    struct Config {
    bool allowed; 
    uint256 decimals;
    uint256 MCR;
    uint256 CCR;
   }

To be:

  struct Config {
    uint allowed;
    uint256 decimals;
    uint256 MCR;
    uint256 CCR;
   }

Useless struct

The one line of code located in Deposit can be moved out of the struct and the struct can be removed. Since there is no purpose for the struct to hold only 1 variable.

https://github.com/code-423n4/2023-02-ethos/blob/main/Ethos-Core/contracts/StabilityPool.sol#L174-L176

   struct Deposit {
   uint initialValue;
   }

Mark secure math with unchecked{}

https://github.com/code-423n4/2023-02-ethos/blob/main/Ethos-Vault/contracts/abstract/ReaperBaseStrategyv4.sol#L181

    upgradeProposalTime = block.timestamp + FUTURE_NEXT_PROPOSAL_TIME;

Since upgradeProposalTime is uint256 and FUTURE_NEXT_PROPOSAL_TIME is only 3 153 600 000, it is unlikely to overflow

a=a+b is cheaper than a+=b

It is much cheaper to make it as a = a+b rather than a+=b.

https://github.com/code-423n4/2023-02-ethos/blob/main/Ethos-Vault/contracts/ReaperVaultV2.sol#L505 https://github.com/code-423n4/2023-02-ethos/blob/main/Ethos-Vault/contracts/ReaperVaultV2.sol#L194-L196 https://github.com/code-423n4/2023-02-ethos/blob/main/Ethos-Vault/contracts/ReaperVaultV2.sol#L444-L452 https://github.com/code-423n4/2023-02-ethos/blob/main/Ethos-Vault/contracts/ReaperStrategyGranarySupplyOnly.sol#L133

strategy.gains += vars.gain; 

should be:

strategy.gains = strategy.gains + vars.gain;

Use named returns

Ethos-Core/contracts/LQTY/LQTYStaking.sol/L217

 function _getPendingLUSDGain(address _user) internal view returns (uint ) {
    uint F_LUSD_Snapshot = snapshots[_user].F_LUSD_Snapshot;
    uint LUSDGain = stakes[_user].mul(F_LUSD.sub(F_LUSD_Snapshot)).div(DECIMAL_PRECISION);
    return LUSDGain;
  }

Fix:

 function _getPendingLUSDGain(address _user) internal view returns (uint LUSDGain) {
    uint F_LUSD_Snapshot = snapshots[_user].F_LUSD_Snapshot;
    uint LUSDGain = stakes[_user].mul(F_LUSD.sub(F_LUSD_Snapshot)).div(DECIMAL_PRECISION);
  }

Ethos-Vault/contracts/ReaperVaultV2.sol/L462

Ethos-Core/contracts/TroveManager.sol/L1066 Ethos-Core/contracts/TroveManager.sol/L1201 Ethos-Core/contracts/TroveManager.sol/L1045

Ethos-Core/contracts/BorrowerOperations.sol/L661 Ethos-Core/contracts/BorrowerOperations.sol/L682 Ethos-Core/contracts/BorrowerOperations.sol/L703 Ethos-Core/contracts/BorrowerOperations.sol/L724