Sherlock contest - harleythedog's results

Handle

harleythedog

Vulnerability details

Impact

In Sherlock.sol, the function updateYieldStrategy ignores all errors when yieldStrategy.withdrawAll() is called, and I believe this is an dangerous design choice. If yieldStrategy.withdrawAll() reverts, then all the funds that are deposited in the yield strategy are abandoned, because once a new strategy is set there is no way to call withdraw or withdrawAll on the old strategy (unless you switch back to the old strategy later). There are many reasons why yieldStrategy.withdrawAll() might revert, and most of these reasons should not lead to abandoning the funds in the strategy. For example, if you inspect the code for the Aave lending pool, the transaction can revert simply because the pool is paused temporarily. If the contract force switches and the funds aren't withdrawn, there isn't really a point anyways because there will be no funds to deposit into the new strategy.

Most of the time, I believe the errors on yieldStrategy.withdrawAll() should not be ignored. I think a better choice would be to add a boolean parameter "force" to the updateYieldStrategy function, and based on this boolean, errors will/won't be ignored. This way, funds won't be unnecessarily abandoned in strategies for small errors (like the pool being paused), and the owners still have the choice to ignore errors if they desire.

Proof of Concept

See the code for updateYieldStrategy here.

Tools Used

Inspection.

Recommended Mitigation Steps

Change updateYieldStrategy to the following:

function updateYieldStrategy(IStrategyManager _yieldStrategy, bool force) external override onlyOwner {
    if (address(_yieldStrategy) == address(0)) revert ZeroArgument();
    if (yieldStrategy == _yieldStrategy) revert InvalidArgument();

    // This call is surrounded with a try catch as there is a non-zero chance the underlying yield protocol(s) will fail
    // For example; the Aave V2 withdraw can fail in case there is not enough liquidity available for whatever reason.
    // In case this happens. We still want the yield strategy to be updated.
    // As the worst case could be that the Aave V2 withdraw will never work again, forcing us to never use a yield strategy ever again.
    try yieldStrategy.withdrawAll() {} catch (bytes memory reason) {
      if(!force) {
        revert InvalidConditions();
      }
      emit YieldStrategyUpdateWithdrawAllError(reason);
    }

    emit YieldStrategyUpdated(yieldStrategy, _yieldStrategy);
    yieldStrategy = _yieldStrategy;
  }