SavingsAccount withdrawAll and switchStrategy can freeze user funds by ignoring possible strategy liquidity issues

[code423n4]

Handle

hyh

Vulnerability details

Impact

Full withdrawal and moving funds between strategies can lead to wrong accounting if the corresponding market has tight liquidity, which can be the case at least for AaveYield. That is, as the whole amount is required to be moved at once from Aave, both withdrawAll and switchStrategy will incorrectly account for partial withdrawal as if it was full whenever the corresponding underlying yield pool had liquidity issues.

withdrawAll will delete user entry, locking the user funds in the strategy: user will get partial withdrawal and have the corresponding accounting entry removed, while the remaining actual funds will be frozen within the system.

switchStrategy will subtract full number of shares for the _amount requested from the old strategy, while adding lesser partial number of shares for _tokensReceived to the new one with the same effect of freezing user's funds within the system.

Proof of Concept

SavingsAccount.withdrawAll https://github.com/code-423n4/2021-12-sublime/blob/main/contracts/SavingsAccount/SavingsAccount.sol#L286

SavingsAccount.switchStrategy: https://github.com/code-423n4/2021-12-sublime/blob/main/contracts/SavingsAccount/SavingsAccount.sol#L152

When full withdrawal or strategy switch is performed it is one withdraw via unlockTokens without checking the amount received.

In the same time the withdraw can fail for example for the strategy switch if old strategy is having liquidity issues at the moment, i.e. Aave market is currently have utilization rate too high to withdraw the amount requested given current size of the lending pool.

Aave unlockTokens return is correctly not matched with amount requested: https://github.com/code-423n4/2021-12-sublime/blob/main/contracts/yield/AaveYield.sol#L217

But, for example, withdrawAll ignores the fact that some funds can remain in the strategy and deletes the use entry after one withdraw attempt: https://github.com/code-423n4/2021-12-sublime/blob/main/contracts/SavingsAccount/SavingsAccount.sol#L294 https://github.com/code-423n4/2021-12-sublime/blob/main/contracts/SavingsAccount/SavingsAccount.sol#L312

switchStrategy removes the old entry completely: https://github.com/code-423n4/2021-12-sublime/blob/main/contracts/SavingsAccount/SavingsAccount.sol#L181

Recommended Mitigation Steps

For both withdrawAll and switchStrategy the immediate fix is to account for tokens received in both cases, which are _amount after unlockTokens for withdrawAll and _tokensReceived for switchStrategy.

More general handling of the liquidity issues ideally to be addressed architecturally, given the potential issues with liquidity availability any strategy withdrawals can be done as follows:

1. Withdraw what is possible on demand, leave the amount due as is, i.e. do not commit to completing the action in one go and notify the user the action was partial (return actual amount)
2. Save to query and repeat for the remainder funds on the next similar action (this can be separate flag triggered mode)

[ritik99]

The above issue requires making a few assumptions - (i) the underlying yield protocol does not have sufficient reserves to facilitate the withdrawal of a single user, (ii) the user attempts to withdraw all their assets during such times of insufficient reserves.

We agree that the above could be a possibility, but would be unlikely. The underlying yield protocols undergo an interest rate spike during high utilization ratios to bring reserves back to normal levels, and some revert if they cannot withdraw the necessary amount (for eg, Compound). During live deployment, only those strategies that work expectedly would be onboarded, while others wouldn't (for eg, Aave as a strategy wouldn't be integrated until their wrappers for aTokens are ready for use). Hence we suggest reducing severity to (2) medium-risk

[ritik99]

also similar to #144

[0xean]

While I understand the argument regarding this being an unlikely scenario, I don't believe that is a sufficient reason to downgrade the issue give the impact to a user and the lost funds.

2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

In this scenario - Assets are at a direct risk.