Ethos Reserve contest - Parad0x's results

Lines of code

https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Vault/contracts/ReaperVaultV2.sol#L359-L412 https://github.com/code-423n4/2023-02-ethos/blob/73687f32b934c9d697b97745356cdf8a1f264955/Ethos-Vault/contracts/ReaperVaultV2.sol#L399-L407

Vulnerability details

Impact

When there are locked funds in the strategy, the withdrawMaxLoss feature of ReaperVaultV2 is not respected. Locked funds refer to a situation where the requested amount and the returned amount differ from the loss reported. This behavior is acceptable for a strategy, but in this scenario, the _withdraw() function calculates the realized loss incorrectly, and a user who withdraws their funds will receive less than expected while having their shares burned.

In situations where a strategy experiences a liquidity squeeze, users may lose their entire asset amount as all their requested shares may be burned, with only a small amount transferred to them. The amount transferred can be arbitrarily low and this behavior is not affected by the withdrawMaxLoss limit.

Proof of Concept

In this code snippet:

            vaultBalance = token.balanceOf(address(this));
            if (value > vaultBalance) {
                value = vaultBalance;
            }

The function withdraw will reset the value and set it to the token.balanceOf(address(this)); if the balance is not enough for withdraw. As an impact, some funds can get frozen.

In ReaperVaultV2's _withdraw() function, a situation may arise where lockedAmount = _amountNeeded - (liquidatedAmount + loss) >= 0. This amount cannot be withdrawn at the moment and is neither considered a loss. The _withdraw() function updates the value per if (value > vaultBalance) {value = vaultBalance;}, which means that the totalLoss check for the rebased value <= ((value + totalLoss) * withdrawMaxLoss) / PERCENT_DIVISOR does not accurately control for the real loss. As a result, a user may lose their entire amount because _withdraw() first burns the full amount of the requested _shares and this total loss check is the only protection in place.

Consider the scenario where there is only one strategy, and 90 out of the 100 requested tokens are currently locked due to a temporary liquidity squeeze, and there is no loss involved. Suppose there are no tokens in the vault balance before the strategy's withdrawal.

In this case, if a user initiates a withdrawal using ReaperBaseStrategyv4's withdraw() function, only 10 tokens will be transferred, and there will be no reported loss. The check for total loss, i.e., 0 = totalLoss <= ((value + totalLoss) * withdrawMaxLoss) / PERCENT_DIVISOR, will be satisfied for any withdrawMaxLoss setting.

As a result, the user, will receive only 10 tokens, and the remaining 100 tokens worth of shares will be burned.

Tools Used

Manual review.

Recommended Mitigation Steps

I would recommend to slightly adapt the logic as following:

require(
   totalLoss <= (value * withdrawMaxLoss) / PERCENT_DIVISOR,
   "Withdraw loss exceeds slippage"
);

value -= totalLoss;

vaultBalance = token.balanceOf(address(this));
[...]