Liquid Staking

Summary

Suppose there is a broken strategy which was just discovered to have broken functionalities. Seeing that, the owner tries to remove the strategy and calls removeStrategy. However, as the strategy is broken, calls to strategy.withdraw may revert for certain reasons (explained below).

This will result in the owner permanently being unable to remove the strategy.

Possible Reasons for strategy.withdraw to fail

Even if we assume that the strategy is honest and has no malicious intends to purposely cause a DoS to the withdraw function, there are still several reasons why it may be possible.

Most prominently, logic errors discovered in the custom staking strategy code only after owner adds the strategy. Such logic errors can cause accounting issues or tokens to be stuck on the strategy's own side and hence strategy.withdraw will cause problems and revert, resulting in the whole attempt of removing the strategy to revert.

• Since strategies are also susceptible to slashings, if the strategy's logic in accounting for slashings is broken and only discovered after adding it, it may also lead to accounting errors not tallying and hence withdraw reverting.

Impact

Furthermore, one purpose of removeStrategy is also for owners who have recently changed their mind over concerns regarding a strategy's goals or implementation.

By giving the strategy.withdraw the power to decide whether the strategy can be removed(as it can revert) defeats the original intention of giving the owner the main rights to removing a strategy.

Therefore, the owner might not be able to call removeStrategy and pop it from the strategies array and most importantly will not be able to run the most crucial last line: token.safeApprove(address(strategy), 0);

Recommendation

Implement a emergency function, where if strategy.withdraw permanently reverts due to stuck tokens/accounting error. Users can call the emergency function and inside it, it only does 2 things

1. remove from strategies array

2. token.safeApprove(address(strategy), 0);

That way the Owner will still have the final say to be able to remove the strategy.