search

code-423n4 / 2022-01-sherlock-findings

0 stars 0 forks source link

updateYieldStrategy will freeze some funds with the old Strategy if yieldStrategy fails to withdraw all the funds because of liquidity issues #76

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Handle

hyh

Vulnerability details

Impact

Part of the funds held with the strategy can be frozen if the current strategy has tight liquidity when updateYieldStrategy is run as this function makes an attempt to withdraw all the funds and then unconditionally removes the strategy.

The Sherlock to YieldStrategy link will be broken as a result: Sherlock points to the new Strategy, while old Strategy still allows only this Sherlock contract to withdraw.

This way back and forth switches will be required in the future to return the funds: withdraw all from new strategy and switch to old, withdraw all from old and point to new one again, reinvest there.

Proof of Concept

In peer-to-peer lending protocols it is not always possible for the token supplier to withdraw all what's due. This happens on high utilization of the market (when it has a kind of liquidity crunch).

This way yieldStrategy.withdrawAll is not guaranteed to obtain all the funds held with the strategy:

https://github.com/code-423n4/2022-01-sherlock/blob/main/contracts/Sherlock.sol#L286

The worst case scenario here seems to be the remainder funds to be left frozen within the strategy.

For example, AaveV2Strategy withdraw and withdrawAll have onlySherlockCore modifier:

https://github.com/code-423n4/2022-01-sherlock/blob/main/contracts/managers/AaveV2Strategy.sol#L78-100

While Sherlock core is immutable for the Strategy by default:

https://github.com/code-423n4/2022-01-sherlock/blob/main/contracts/managers/Manager.sol#L26-41

Recommended Mitigation Steps

Consider implementing a new method that fails whenever a strategy cannot withdraw all what's due now, and rename current implementation to, for example, forceUpdateYieldStrategy, to have a degree of flexibility around liquidity issues.

Also, to avoid back and forth switching, a strategy argument can be introduced to yieldStrategyWithdrawAll to allow withdrawals from any (not only current) yieldStrategy:

https://github.com/code-423n4/2022-01-sherlock/blob/main/contracts/Sherlock.sol#L322

Now:

function yieldStrategyWithdrawAll() external override onlyOwner {

To be (if _yieldStrategy is zero then utilize current):

function yieldStrategyWithdrawAll(IStrategyManager _yieldStrategy) external override onlyOwner {
Evert0x commented 2 years ago

I think this should be low risk but it's an interesting feature to add

Evert0x commented 2 years ago

Similar findings

jack-the-pug commented 2 years ago

I think this worths a Med, the scenario is not impossible to happen, and the handling in the current implementation is quite rough.