Ethos Reserve contest - MiloTruck's results

Lines of code

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

Vulnerability details

In the ActivePool contract, the _rebalance() function subtracts the amount of collateral currently in the vault (vars.sharesToAssets) from the amount originally allocated (vars.currentAllocated) to calculate its profit. This can be seen in ActivePool.sol#L248-L251:

vars.sharesToAssets = vars.yieldGenerator.convertToAssets(vars.ownedShares);

// if we have profit that's more than the threshold, record it for withdrawal and redistribution
vars.profit = vars.sharesToAssets.sub(vars.currentAllocated);

As seen from above, the sub() function from SafeMath is used to do the calculation. Hence, whenever the vault is at a loss, _rebalance() will always revert as vars.currentAllocated is less than vars.sharesToAssets, leading to an arithmetic underflow.

This affects the following functions as they call the _rebalance() function:

• BorrowOperations.sol
  • openTrove
  • closeTrove
  • addColl
  • withdrawColl
  • withdrawLUSD
  • repayLUSD
  • adjustTrove
• TroveManager.sol
  • redeemCollateral
  • liquidate
  • batchLiquidateTroves
  • liquidateTroves

Impact

Whenever a vault containing one type of collateral is at a loss, all the functions listed above will revert when called with the same type of collateral, massively affecting the protocol's functionality.

Moreover, since all the functions used to withdraw/transfer collateral are affected, all the borrowers who opened troves with the same type of collateral will have their collateral stuck in the protocol until the vault has a non-negative profit.

Proof of Concept

The test in this gist demonstrates how a vault at a loss causes all the functions listed above to revert.

To run the test:

• Copy MockBorrowerOperations.sol into Ethos-Core/contracts/TestContracts.
• Copy RebalanceDOSTest.js into Ethos-Core/test.
• Run the following command: npx hardhat test ./test/RebalanceDOSTest.js

Recommended Mitigation Steps

Consider declaring vars.profit as a int256, and changing ActivePool.sol#L250-L254 to the following:

// if we have profit that's more than the threshold, record it for withdrawal and redistribution
if (vars.sharesToAssets < vars.currentAllocated && vars.currentAllocated - vars.sharesToAssets >= yieldClaimThreshold[_collateral]) {
    vars.profit = -int256(vars.currentAllocated - vars.sharesToAssets);
} else if (vars.sharesToAssets > vars.currentAllocated && vars.sharesToAssets - vars.currentAllocated >= yieldClaimThreshold[_collateral]) {
    vars.profit = int256(vars.sharesToAssets - vars.currentAllocated);
}