Description

The vault contract takes two arguments _minAmount0 and _minAmount1 that are intended for the user to provide the minimum amount of tokens they expect to receive, with the call reverting if the amount they would actually receive would be lower than that due to slippage. In the function, the amount the user is to receive for their shares is first calculated as _amount0 and _amount1, which are then checked to be at least _minAmount0 and _minAmount1, with the function otherwise reverting. However, the amount actually transferred to the user later might be smaller than the value of _amount0 and _amount1 at this point, as the value of these variables might be reduced if the strategy failed to send a sufficient amount of tokens.

function withdraw(uint256 _shares, uint256 _minAmount0, uint256 _minAmount1) public {
    // ... _amount0 and _amount1 calculated here
    if (_amount0 < _minAmount0 || _amount1 < _minAmount1) revert TooMuchSlippage();

    (uint before0, uint before1)  = available();
    if (before0 < _amount0 ||  before1 < _amount1) {
        // ...
        if (diff0 < _withdraw0) _amount0 = before0 + diff0; // _amount0 changed to a possibly smaller value
        if (diff1 < _withdraw1) _amount1 = before1 + diff1;
    }

    (address token0, address token1) = wants();
    IERC20Upgradeable(token0).safeTransfer(msg.sender, _amount0); // the new value of _amount0 is used to transfer tokens to the caller, not the old one checked against _minAmount0
    IERC20Upgradeable(token1).safeTransfer(msg.sender, _amount1);

    emit Withdraw(msg.sender, _shares, _amount0, _amount1);
}

Impact

From the perspective of the vault's withdraw function, it could happen that the caller receives less tokens than the minimum amount they specified.

However, taking into account the actual implementation of the StrategyPassiveManagerUniswap contract's withdraw function, we see that the requested amount is always transferred exactly (safeTransfer will revert if the balance is insufficient).

function withdraw(uint256 _amount0, uint256 _amount1) external {
    _onlyVault();

    // Liquidity has already been removed in beforeAction() so this is just a simple withdraw.
    if (_amount0 > 0) IERC20Metadata(lpToken0).safeTransfer(vault, _amount0);
    if (_amount1 > 0) IERC20Metadata(lpToken1).safeTransfer(vault, _amount1);

    // ...
}

Hence, when the StrategyPassiveManagerUniswap contract is used as the strategy contract, this issue cannot occur, as diff0 < _withdraw0 and diff1 < _withdraw1 will never be true.

Recommendations

Consider checking the amount transferred to the caller against the minimum amount just before doing the actual transfer, thus taking into account any adjustments made before.

Remediation

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