Description

The strategy contract provides the tokens as liquidity in a Uniswap pool, thereby accumulating the relevant fees on the pool. For various actions by the strategy or vault contract, the liquidity is first withdrawn fully from the Uniswap pool, then added again. This is done to, for example, be able to reduce the amount locked in the Uniswap pool in order to be able to withdraw, or in order to have more precise accounting.

However, in the vault's deposit function, under some circumstances, it could happen that liquidity is first withdrawn but not added again:

function withdraw(uint256 _shares, uint256 _minAmount0, uint256 _minAmount1) public {
    // ...
    // Withdraw All Liquidity to Strat for Accounting.
    strategy.beforeAction();

    // ...
    if (before0 < _amount0 ||  before1 < _amount1) {

        uint _withdraw0 = _amount0 - before0;
        uint _withdraw1 = _amount1 - before1;

        strategy.withdraw(_withdraw0, _withdraw1);
       
        // ...
    }
    // ...
}

Towards the start of the withdraw function, strategy.beforeAction() is called, which will in particular remove liquidity. The only call afterwards in the function that will add liquidity again is strategy.withdraw(_withdraw0, _withdraw1). However, this latter call is in a conditional branch, so if this branch is not executed, the removed liquidity would not be added again.

Impact

If it happens that before0 < _amount0 || before1 < _amount1 is not true, then after this call to withdraw, all funds will held by the strategy contract, where they do not generate any profit, rather than being provided as liquidity in the Uniswap pools. The strategy contract will thus fail to realize profits it could otherwise have, until the next time a function is called that will cause liquidity to be added.

We note that in normal operations, before0 < _amount0 || before1 < _amount1 should always be true.

Recommendations

Ensure that liquidity gets added again in withdraw after the call to strategy.beforeAction() no matter whether the conditional branch is taken. For example, considering that during normal operations the vault should not hold any tokens (see ref↗), it might be an option to replace lines 226 to 240 (inclusive) by the following line, also simplifying the logic significantly.

strategy.withdraw(_withdraw0, _withdraw1);

This solution would mean, however, that any token0 or token1 sent to the vault accidentally would be stuck instead of being treated as a donation and used on withdrawals.

Remediation

This issue has been acknowledged by Beefy, and a fix was implemented in commit 8ff8e999↗.