ReaperBaseStrategyv4.harvest() might revert in an emergency.

[code423n4]

Lines of code

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

Vulnerability details

Impact

ReaperBaseStrategyv4.harvest() might revert in an emergency if there is no position on the lending pool.

As a result, the funds might be locked inside the strategy.

Proof of Concept

The main problem is that Aave lending pool doesn't allow 0 withdrawals.

  function validateWithdraw(
    address reserveAddress,
    uint256 amount,
    uint256 userBalance,
    mapping(address => DataTypes.ReserveData) storage reservesData,
    DataTypes.UserConfigurationMap storage userConfig,
    mapping(uint256 => address) storage reserves,
    uint256 reservesCount,
    address oracle
  ) external view {
    require(amount != 0, Errors.VL_INVALID_AMOUNT);

So the below scenario would be possible.

1. After depositing and withdrawing from the Aave lending pool, the current position is 0 and the strategy is in debt.
2. It's possible that the strategy has some want balance in the contract but no position on the lending pool.
   It's because _adjustPosition() remains the debt during reinvesting and also, there is an authorizedWithdrawUnderlying() for STRATEGIST to withdraw from the lending pool.
3. If the strategy is in an emergency, harvest() tries to liquidate all positions(=0 actually) and it will revert because of 0 withdrawal from Aave.
4. Also, withdraw() will revert at L98 as the strategy is in the debt.

As a result, the funds might be locked inside the strategy unless the emergency mode is canceled.

Tools Used

Manual Review

Recommended Mitigation Steps

We should check 0 withdrawal in _withdrawUnderlying().

    function _withdrawUnderlying(uint256 _withdrawAmount) internal {
        uint256 withdrawable = balanceOfPool();
        _withdrawAmount = MathUpgradeable.min(_withdrawAmount, withdrawable);

        if(_withdrawAmount != 0) {
            ILendingPool(ADDRESSES_PROVIDER.getLendingPool()).withdraw(address(want), _withdrawAmount, address(this));
        }
    }

[trust1995]

Very interesting edge case.

[c4-judge]

trust1995 marked the issue as satisfactory

[c4-judge]

trust1995 marked the issue as primary issue

[tess3rac7]

Valid edge case in as far as harvests would fail. However, funds won't get locked in the strategy. They can still be withdrawn through an appropriate withdraw() TX. Recommend downgrading to low since this is purely about state handling without putting any assets at risk. See screenshot below for simulation:

[c4-sponsor]

tess3rac7 marked the issue as disagree with severity

[trust1995]

Med severity is also appropriate when core functionality is impaired, even if there is no lasting damage.

[c4-judge]

trust1995 marked the issue as selected for report

[c4-sponsor]

tess3rac7 marked the issue as sponsor confirmed