[1] - Withdrawer can incur loss due to bad withdraw timing. Loss pressure not distributed equally

Withdrawer can incur loss due to bad withdraw timing. Loss pressure not distributed equally. · Issue #44 · code-423n4/2023-02-ethos-findings (github.com)

Lines of code

https://github.com/code-423n4/2023-02-ethos/blob/main/Ethos-Vault/contracts/ReaperVaultV2.sol#L359

Impact

Severity: QA (Quality Assurance)

When withdrawing, if not enough balance is present, strategies will be sold to be able to fullfil the withdrawal. When "selling" strategies any loss is charged to the withdrawer. Therefore a withdrawer might think is withdrawing 'x' but in reality is withdrawing way less. The issue is that the code does not share loss between users, but applies the total loss pressure of a strategy to the withdrawing user, even though the user has no control over what strategies are going to be applied.

Proof of Concept

In ReaperVaultV2.sol there is a function with which users interact called withdraw() this function then calls the internal _withdraw().

Inside the internal function the value of the shares the user wishes to withdraw is calculated.
The following line checks if the vault contains enough liquid to fulfill the withdrawal or it needs to "sell" strategies in order to continue the withdrawal.
if (value > token.balanceOf(address(this)))

In the case that the contract does not contain enough liquid to pay it will proceed to sell strategies until enough token is accumulated to continue with the withdrawal.

for (uint256 i = 0; i < queueLength; i = i.uncheckedInc()) {
                vaultBalance = token.balanceOf(address(this));
                if (value <= vaultBalance) {
                    break;
                }

                address stratAddr = withdrawalQueue[i];
                uint256 strategyBal = strategies[stratAddr].allocated;
                if (strategyBal == 0) {
                    continue;
                }

                uint256 remaining = value - vaultBalance;

                uint256 loss = IStrategy(stratAddr).withdraw(Math.min(remaining, strategyBal));
                uint256 actualWithdrawn = token.balanceOf(address(this)) - vaultBalance;

                // Withdrawer incurs any losses from withdrawing as reported by strat
                if (loss != 0) {
                    value -= loss;
                    totalLoss += loss;
                    _reportLoss(stratAddr, loss);
                }

                strategies[stratAddr].allocated -= actualWithdrawn;
                totalAllocated -= actualWithdrawn;
            }

In this for loop any loss from the strategies is charged to the withdrawer. The loss is calculated only for the strategy liquidated. Code inside if(loss != 0) will apply complete loss pressure to the user.

This can lead to withdrawers not knowing how much will they receive from the withdrawal before hand. As well as it could lead to other users trying to time well withdrawals to avoid paying the losses, therefore placing the full weight of the loss on the rest of the users.

For example:

1. Bob deposits 50 token.

2. Alice deposits 50 token

3. Out of the 100 total token inside the vault, 50 stay as liquid and 50 are deposited into 1 strategy.

4. This strategy losses 10 out of the 50 token. Having now 40.

5. Bob decided to withdraw its token. He withdraws 50 token, happening to be 50 liquid token inside the vault Bob receives 50.

6. Alice decides to withdraw. Since the vault does not have enough liquid, since Bob already withdrawed, the vault needs to liquidate the strategy. Since the strategy has lost 10 of the token only 40 are obtained when liquidating. Since loss is charged to the withdrawer Alice only receives 40.

Both Alice and Bob deposited the same amount of token at "the same time". And both withdrawed the same amount of token when the strategy had lost 10. But only Bob has received the total amount invested. Therefore applying the whole loss pressure to Alice.

Loss pressure should be distributed among the different users on the platform. As well as notifying the users what potential loss they will have if they withdraw.

Without this a user can see its withdrawal reduced due to "unpredictable" circumstances. In a worst case scenario, a strategy could have an unpredicted high loss and a user withdrawing (a quantity superior to the one applied to the strategy) could be charged the full weight of that loss significantly reducing their withdrawal.

Withdrawers should pay for the loss of the strategies, but applying an homogeneous pressure in all of them.

Tools Used

Visual Studio Code

Recommended Mitigation Steps

A loss index should be calculated for all the strategies. Therefore charging a proportion of the loss to every user that withdraws. In other words calculate the total amount of loss for all strategies and apply that to the value of the withdrawal. A way to do this could be to calculate the total loss of all strategies, and subtract it from the total amount of token, then get a percentage of loss per unit token and apply that to the quantity of the withdrawal. Therefore applying a fair and homogeneous portion to the users when the strategies are at a loss.
Calculating the loss percentage each time that a withdrawal can be quite expensive if the amount of strategies is large. An alternative could be to calculate a loss index every x period of time, for example every day, or every hour.

The goal should be to distribute the loss among all the users, applying it only when the users withdraw.

If there is no loss as a total, then users will experience no reduction in their withdrawal. If there is a loss as a total, users will see their withdrawals reduced.