code-423n4 / 2023-02-ethos-findings

8 stars 6 forks source link

positioning of strategies withdrawal can be used by depositors to time their withdrawal to avoid losses and hence causing last few depositors to incur the losses of the losing strategies #681

Open code423n4 opened 1 year ago

code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

Strategies will be queued in a for loop to withdraw outstanding balance that vault cannot cover. Depositors are incentivized to wait for strategies that are in the front of the queue to not be losing money and withdraw. Final few depositors will be left to incur losses of the strategies that are losing.

Proof of Concept

Every strategy is queued for withdrawal, and if there is sufficient amount to cover the withdrawal, break will exit the loop and no withdrawal will be made for remaining strategies. The loss of a strategy will be shared by the withdrawer. The problem lies in the fact that when a strategy that is losing is placed at the back of the queue, depositors who withdraw early will be able to avoid the loss. This creates an opportunity for depositors to avoid the losses by waiting for the perfect moment when the front strategies are not losing and withdrawing their share. This will cause loss of fund to last few depositors as they are left to incur the losses of the strategies at the back.

            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;
            }

Tools Used

Manual Review

Recommended Mitigation Steps

Recommend withdrawing from all strategies proportional to the allocated funds instead of withdrawing first to last strategy.

c4-judge commented 1 year ago

trust1995 marked the issue as satisfactory

c4-judge commented 1 year ago

trust1995 marked the issue as primary issue

tess3rac7 commented 1 year ago

All withdrawers are subject to the same withdrawMaxLoss value (currently set to 1 BPS which means 0.01%). So it's not possible for some users to have a successful TX whereby loss is greater than withdrawMaxLoss.

If the following holds:

Then the following must hold for the remaining depositors trying to withdraw:

The behavior is consistent with the design of the system. A loss is realized only when funds are recovered from a strategy. And by initiating a withdraw, a user is forcing recovery of funds, thereby potentially forcing loss (that has not yet been realized). A vault whereby realized losses are somehow predicted and socialized would be a completely different system design based on a different financial philosophy.

c4-sponsor commented 1 year ago

tess3rac7 marked the issue as sponsor disputed

c4-judge commented 1 year ago

trust1995 marked the issue as unsatisfactory: Invalid

c4-judge commented 1 year ago

trust1995 changed the severity to 2 (Med Risk)

vkabc commented 1 year ago

Hi @trust1995 , withdrawMaxLoss will only cause DOS to the last few depositors and prevent them from withdrawing from strategies. It does not stop first X depositors from avoiding the loss. I don't think that the positioning of strategies should play a role in whether loss should be incurred or not as it opens up an attack vector where depositors can avoid incurring losses which will be then dumped to the last few depositors.

trust1995 commented 1 year ago

I have to side with sponsors and conclude this is a design-level decision, M-level impact is not demonstrated. Will QA it.

c4-judge commented 1 year ago

trust1995 marked the issue as satisfactory

c4-judge commented 1 year ago

trust1995 changed the severity to QA (Quality Assurance)

c4-judge commented 1 year ago

trust1995 marked the issue as grade-b