Description

In the provided code snippet at BeefyWrapper.sol, line #142, within the _withdraw() function, a valid execution logic would involve reverting the function if the value of assets exceeds the balance. This condition should ideally never evaluate to true.

function _withdraw(
        address caller,
        address receiver,
        address owner,
        uint256 assets,
        uint256 shares
    ) internal virtual override {
        if (caller != owner) {
            _spendAllowance(owner, caller, shares);
        }
        _burn(owner, shares);

        IVault(vault).withdraw(shares);
        uint balance = IERC20Upgradeable(asset()).balanceOf(address(this));
!        if (assets > balance) {
!            assets = balance;
        }

        IERC20Upgradeable(asset()).safeTransfer(receiver, assets);

        emit Withdraw(caller, receiver, owner, assets, shares);
    }

Impact

If the condition where assets is greater than balance is not handled properly and allowed to proceed, it might result in unexpected behavior and cause confusion.

Recommendations

Consider changing the logic of the function to revert in such a scenario.

Remediation

This finding has been acknowledged by Beefy Finance. Their official response is paraphrased below:

We will sometimes withdraw less from a vault than calculated as the ERC4626 preview calculation doesn't take into account any withdrawal fees, either from our strategy contracts or from underlying contracts. Any underlying contract's fees are not currently fetched dynamically and would require very custom code for each vault to fetch. Even a 0.01% fee on the strategy will cause the revert. We would like to avoid reverts in any withdrawal, which is exactly what the existing logic prevents. We're not swapping in the withdrawal so don't expect slippage, only withdraw fees.