Ethos Reserve contest - bin2chen's results

Lines of code

https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/StabilityPool.sol#L531

Vulnerability details

Impact

no judgment that lastLUSDLossError_Offset!=0, but _debtToOffset is 0 _computeRewardsPerUnitStaked() may overflow

Proof of Concept

Use lastLUSDLossError_Offset in the _computeRewardsPerUnitStaked() method to store the last excess The code is as follows:

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

       if (_debtToOffset == _totalLUSDDeposits) {
            LUSDLossPerUnitStaked = DECIMAL_PRECISION;  // When the Pool depletes to 0, so does each deposit 
            lastLUSDLossError_Offset = 0;
        } else {

            uint LUSDLossNumerator = _debtToOffset.mul(DECIMAL_PRECISION).sub(lastLUSDLossError_Offset);//<----------If _debtToOffset==0,but lastLUSDLossError_Offset!=0 will overflow
            /*    

Here is a problem, there is no judgment that lastLUSDLossError_Offset!=0, but _debtToOffset is 0, so when this method is executed will overflow is it possible for _debtToOffset to be 0? This is possible updateRewardSum() is to pass _debtToOffset=0 The code is as follows:

    function updateRewardSum(address _collateral, uint _collToAdd) external override {
        _requireCallerIsActivePool();
        uint totalLUSD = totalLUSDDeposits; // cached to save an SLOAD
        if (totalLUSD == 0) { return; }

        _triggerLQTYIssuance(communityIssuance);

        (uint collGainPerUnitStaked, ) = _computeRewardsPerUnitStaked(_collateral, _collToAdd, 0, totalLUSD);//<---------_debtToOffset==0


    }

In ActivePool._rebalance() will call StabilityPool.updateRewardSum()

This will cause the ActivePool to overflow and fail if there is a residual lastLUSDLossError_Offset after the execution of StabilityPool.offset().

Tools Used

Recommended Mitigation Steps

_debtToOffset < lastLUSDLossError_Offset then return LUSDLossPerUnitStaked =0

Lines of code

https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Vault/contracts/ReaperStrategyGranarySupplyOnly.sol#L141

Vulnerability details

Impact

_harvestCore() roi calculation error,may double

Proof of Concept

The _harvestCore() will calculate the roi and repayment values The implementation code is as follows:

    function _harvestCore(uint256 _debt) internal override returns (int256 roi, uint256 repayment) {
        _claimRewards();
        uint256 numSteps = steps.length;
        for (uint256 i = 0; i < numSteps; i = i.uncheckedInc()) {
            address[2] storage step = steps[i];
            IERC20Upgradeable startToken = IERC20Upgradeable(step[0]);
            uint256 amount = startToken.balanceOf(address(this));
            if (amount == 0) {
                continue;
            }
            _swapVelo(step[0], step[1], amount, VELO_ROUTER);
        }

        uint256 allocated = IVault(vault).strategies(address(this)).allocated;
        uint256 totalAssets = balanceOf();
        uint256 toFree = _debt;

        if (totalAssets > allocated) {
            uint256 profit = totalAssets - allocated;
            toFree += profit;
            roi = int256(profit);
        } else if (totalAssets < allocated) {
            roi = -int256(allocated - totalAssets);
        }

        (uint256 amountFreed, uint256 loss) = _liquidatePosition(toFree);
        repayment = MathUpgradeable.min(_debt, amountFreed);
        roi -= int256(loss);//<------这个地方可能会导致重复
    }

The last line may cause double counting of losses For example, the current: vault.allocated = 9 vault.strategy.allocBPS = 9000 strategy.totalAssets = 9

Suppose that after some time, strategy loses 2, then: strategy.totalAssets = 9 - 2 = 7 Also the administrator sets vault.strategy.allocBPS = 0

This executes harvest()->_harvestCore(9) to get roi = 4 repayment = 7

The actual loss of 2, but roi = 4 (double), test code as follows:

add to test/starter-test.js 'Vault Tests'

    it.only('test_roi', async function () {
      const {vault, strategy, wantHolder, strategist} = await loadFixture(deployVaultAndStrategyAndGetSigners);
      const depositAmount = toWantUnit('10');
      await vault.connect(wantHolder)['deposit(uint256)'](depositAmount);
      await strategy.harvest();

      const balanceOf = await strategy.balanceOf();
      console.log(`strategy balanceOf: ${balanceOf}`);
      // allocated = 9
      // 1.loss 2, left 7
      await strategy.lossFortest(toWantUnit('2'));
      // 2.modify bps=>0
      await vault.connect(strategist).updateStrategyAllocBPS(strategy.address, 0);
      // 3.so debt = 9
      await strategy.harvest();

      const {allocated, losses, allocBPS} = await vault.strategies(strategy.address);
      console.log(`losses: ${losses}`);
      console.log(`allocated: ${allocated}`);
      console.log(`allocBPS: ${allocBPS}`);
    });

add to ReaperStrategyGranarySupplyOnly.sol

    function lossFortest(uint256 amout) external{
        ILendingPool(ADDRESSES_PROVIDER.getLendingPool()).withdraw(address(want), amout, address(1));        
    }

$ npx hardhat test test/starter-test.js

  Vaults
    Vault Tests
strategy balanceOf: 900000000
losses: 400000000     <--------will double
allocated: 0
allocBPS: 0

The last vault's allocated is correct, but the loss is wrong Statistics and bpsChange of _reportLoss() will be wrong

Tools Used

Recommended Mitigation Steps

remove roi -= int256(loss);

Lines of code

https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Core/contracts/ActivePool.sol#L251

Vulnerability details

Impact

There may be a loss of precision in the vault deposit, resulting in _rebalance overflow

Proof of Concept

After executing ActivePool.rebalance() it will first calculate the current profit The code is as follows:

    function _rebalance(address _collateral, uint256 _amountLeavingPool) internal {
...
        vars.currentAllocated = yieldingAmount[_collateral];
        

        vars.yieldGenerator = IERC4626(yieldGenerator[_collateral]);
        vars.ownedShares = vars.yieldGenerator.balanceOf(address(this));
        vars.sharesToAssets = vars.yieldGenerator.convertToAssets(vars.ownedShares);

        vars.profit = vars.sharesToAssets.sub(vars.currentAllocated); //<-------vars.profit is uint256
        if (vars.profit < yieldClaimThreshold[_collateral]) {
            vars.profit = 0;
        }    

where vars.fit is uint256, the default is not negative The documentation is described below: The vault will be farming the lowest-risk yield possible, so you can assume that the principal will be protected from loss

But there is a problem, if the vault is not exclusive (only one depositor), when making a deposit is generally a loss of precision If you deposit 100 (* e18), you will get 99.99999999999999999999 (*e18) if you take it immediately Simplify ReaperVaultV2.sol as follows:

  function test() external {
    uint256 totalSupply = 333*10**18;
    uint256 totalAsset = 340*10**18;
    uint256 depositAsset = 100*10**18;

    //1.get shares
    uint256 getShares = depositAsset * totalSupply / totalAsset; //round down
    totalAsset  += depositAsset;
    totalSupply += getShares;
    //2.convertToAssets use round down
    uint256 getAsset = getShares * totalAsset / totalSupply; //round down
    console.log("depositAsset:",depositAsset);
    console.log("getAsset:",getAsset);
  }

$ forge test -vvv

[PASS] test() (gas: 4593)
Logs:
  depositAsset: 100000000000000000000
  getAsset: 99999999999999999999

This will result in vars.profit = vars.sharesToAssets.sub(vars.currentAllocated); overflow ActivePool.sendCollateral()/ActivePool.pullCollateralFromBorrowerOperationsOrDefaultPool() will revert Until there are gains or other depositors

Tools Used

Recommended Mitigation Steps

vars.sharesToAssets.sub(vars.currentAllocated) If less than 0, then set profit=0

L-1: CollateralConfig.updateCollateralRatios() will modify the MCR and CCR, these two parameters will affect whether the user is liquidated, related to the user's collateral. Although the administrator is a multi-signature wallet, but still need to have an effective time, to give the user sufficient time to choose whether to keep trove or repayment

L-2: increaseLUSDDebt()/decreaseLUSDDebt() Event missing emit

    function increaseLUSDDebt(address _collateral, uint _amount) external override {
        _requireValidCollateralAddress(_collateral);
        _requireCallerIsBOorTroveM();
        LUSDDebt[_collateral] = LUSDDebt[_collateral].add(_amount);
-       ActivePoolLUSDDebtUpdated(_collateral, LUSDDebt[_collateral]);
+       emit ActivePoolLUSDDebtUpdated(_collateral, LUSDDebt[_collateral]);
    }

L-3: TroveManager._removeTroveOwner() Forgot settings Troves[_borrower][_collateral].arrayIndex = 0 when clearing Trove Owner

function _removeTroveOwner(address _borrower, address _collateral, uint TroveOwnersArrayLength) internal {
    Status troveStatus = Troves[_borrower][_collateral].status;
    // It’s set in caller function `_closeTrove`
    assert(troveStatus != Status.nonExistent && troveStatus != Status.active);

    uint128 index = Troves[_borrower][_collateral].arrayIndex;
    uint length = TroveOwnersArrayLength;
    uint idxLast = length.sub(1);

    assert(index <= idxLast);

    address addressToMove = TroveOwners[_collateral][idxLast];

    TroveOwners[_collateral][index] = addressToMove;
    Troves[addressToMove][_collateral].arrayIndex = index;
    emit TroveIndexUpdated(addressToMove, _collateral, index);

•   Troves[_borrower][_collateral].arrayIndex = 0;
    TroveOwners[_collateral].pop();
  

  }