_rebalance() may overflow

[code423n4]

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

[c4-judge]

trust1995 marked the issue as primary issue

[c4-judge]

trust1995 marked the issue as satisfactory

[tess3rac7]

Within this test https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Vault/test/starter-test.js#L332 one can add console logs for userBalance and userBalanceAfterWithdraw and verify that they are same.

Moreover,

• System is not designed to handle losses as described in the docs:

The vault will be farming the lowest-risk yield possible, so you can assume that the principal will be protected from loss

• Only ActivePool will be given DEPOSITOR role

[c4-sponsor]

tess3rac7 marked the issue as sponsor disputed

[trust1995]

I don't think it is a fair certainty that strategies will not yield losses. In light of that, med severity seems appropriate.

[c4-judge]

trust1995 marked issue #632 as primary and marked this issue as a duplicate of 632