I think it makes sense to leave an ability for an authorized party to pause the strategy in case of an emergency situation. While this increases the trust on admin, it shouldn't be a problem because an admin can setStrategy in Vault anyway. This also gives the chance to save at least some funds in case the strategy is exploited.
For example, in theory, such a scenario is possible: admin tries to setStrategy in Vault, and this function succeeds only if strategy.investedAssets() == 0. investedAssets is usually dependant on underlyingBalance, aUstBalance, etc. An admin can try to drain the strategy (withdrawAllToVault) before setting the new strategy, but a malicious actor can monitor the mempool and backrun it sending the smallest amount (dust) of underlying to prevent new setStrategy and because all the deposits / withdrawals are async (require several steps), a hacker can postpone setStrategy for a long time. If you add a pausing functionality, new inits of deposit / withdraw should become unavailable when paused, and the admin can safely migrate to a new strategy.
naps62
commented
2 years ago
Handle
pauliax
Vulnerability details
Impact
I think it makes sense to leave an ability for an authorized party to pause the strategy in case of an emergency situation. While this increases the trust on admin, it shouldn't be a problem because an admin can setStrategy in Vault anyway. This also gives the chance to save at least some funds in case the strategy is exploited.
For example, in theory, such a scenario is possible: admin tries to setStrategy in Vault, and this function succeeds only if strategy.investedAssets() == 0. investedAssets is usually dependant on underlyingBalance, aUstBalance, etc. An admin can try to drain the strategy (withdrawAllToVault) before setting the new strategy, but a malicious actor can monitor the mempool and backrun it sending the smallest amount (dust) of underlying to prevent new setStrategy and because all the deposits / withdrawals are async (require several steps), a hacker can postpone setStrategy for a long time. If you add a pausing functionality, new inits of deposit / withdraw should become unavailable when paused, and the admin can safely migrate to a new strategy.
Recommended Mitigation Steps
Consider introducing pausing functionality.