Stragety withdrawal fee estimation is not accurate in BeefyAdapter.sol

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/adapter/beefy/BeefyAdapter.sol#L155 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/adapter/beefy/BeefyAdapter.sol#L176

Vulnerability details

Impact

Stragety withdrawal fee estimation is not accurate in BeefAdapter.sol

Proof of Concept

In the current implementation of the BeefAdapter.sol, when calling previewWithdraw and and previewRedeem, the code tries to estimate the withdrawal fee from Beefy's stragety contract.

/// @notice `previewRedeem` that takes beefy withdrawal fees into account
function previewRedeem(uint256 shares)
    public
    view
    override
    returns (uint256)
{
    uint256 assets = _convertToAssets(shares, Math.Rounding.Down);

    IBeefyStrat strat = IBeefyStrat(beefyVault.strategy());
    uint256 beefyFee = strat.withdrawalFee();
    if (beefyFee > 0)
        assets -= assets.mulDiv(
            beefyFee,
            BPS_DENOMINATOR,
            Math.Rounding.Up
        );

    return assets;
}

and

/// @notice `previewWithdraw` that takes beefy withdrawal fees into account
function previewWithdraw(uint256 assets)
    public
    view
    override
    returns (uint256)
{
    IBeefyStrat strat = IBeefyStrat(beefyVault.strategy());
    uint256 beefyFee = strat.withdrawalFee();
    if (beefyFee > 0)
        assets = assets.mulDiv(
            BPS_DENOMINATOR,
            BPS_DENOMINATOR - beefyFee,
            Math.Rounding.Up
        );

    return _convertToShares(assets, Math.Rounding.Up);
}

However, the withdrawalFee estimate is not accuare.

If we look into the stragety contract if the Beefy finance.

First of all, Beefy finance integrate and develop a lot of stragety.

Some stragety has withdrawalFee exposed.

https://github.com/beefyfinance/beefy-contracts/blob/1a92ee47db78bb625445e8425f53af31fe5e3543/contracts/BIFI/strategies/Curve/StrategyConvex.sol#L102

While others expose the hardcoded parameter:

https://github.com/beefyfinance/beefy-contracts/blob/1a92ee47db78bb625445e8425f53af31fe5e3543/contracts/BIFI/strategies/Mdex/StrategyMdexLP.sol#L79

uint constant public WITHDRAWAL_FEE = 10;

If a stragety does not expose withdrawalFee as a read-only parameter, clearly function below does not work

int256 beefyFee = strat.withdrawalFee();

Also even though the stragety contract exposes the parameter withdrawalFee,

the fee estimation does not match the fee implementation in the Beefy stragety contract.

The fee estimation used is:

   uint256 beefyFee = strat.withdrawalFee();
    if (beefyFee > 0)
        assets = assets.mulDiv(
            BPS_DENOMINATOR,
            BPS_DENOMINATOR - beefyFee,
            Math.Rounding.Up
        );

However, in the Beefy stragety, is

    function withdraw(uint256 _amount) external {
        require(msg.sender == vault, "!vault");

        uint256 wantBal = IERC20(want).balanceOf(address(this));

        if (wantBal < _amount) {
            IConvexRewardPool(rewardPool).withdrawAndUnwrap(_amount - wantBal, false);
            wantBal = IERC20(want).balanceOf(address(this));
        }

        if (wantBal > _amount) {
            wantBal = _amount;
        }

        if (tx.origin != owner() && !paused()) {
            uint256 withdrawalFeeAmount = wantBal * withdrawalFee / WITHDRAWAL_MAX;
            wantBal = wantBal - withdrawalFeeAmount;
        }

        IERC20(want).safeTransfer(vault, wantBal);

        emit Withdraw(balanceOf());
    }

which is:;

https://github.com/beefyfinance/beefy-contracts/blob/1a92ee47db78bb625445e8425f53af31fe5e3543/contracts/BIFI/strategies/Curve/StrategyConvex.sol#L131

if (tx.origin != owner() && !paused()) {
    uint256 withdrawalFeeAmount = wantBal * withdrawalFee / WITHDRAWAL_MAX;
    wantBal = wantBal - withdrawalFeeAmount;
}

Or if the WITHDRAWAL_FEE is used, the withdrawal fee is calculated as:

function withdraw(uint256 _amount) external {
    require(msg.sender == vault, "!vault");

    uint256 pairBal = IERC20(lpPair).balanceOf(address(this));

    if (pairBal < _amount) {
        IMasterChef(masterchef).withdraw(poolId, _amount.sub(pairBal));
        pairBal = IERC20(lpPair).balanceOf(address(this));
    }

    if (pairBal > _amount) {
        pairBal = _amount;
    }

    uint256 withdrawalFee = pairBal.mul(WITHDRAWAL_FEE).div(WITHDRAWAL_MAX);
    IERC20(lpPair).safeTransfer(vault, pairBal.sub(withdrawalFee));
}

which is:

https://github.com/beefyfinance/beefy-contracts/blob/1a92ee47db78bb625445e8425f53af31fe5e3543/contracts/BIFI/strategies/Mdex/StrategyMdexLP.sol#L163

uint256 withdrawalFee = helmetBal.mul(WITHDRAWAL_FEE).div(WITHDRAWAL_MAX);

withdrawal max is 10000

Because the estimation of fee when previewing the withdraw and redeem asset does not match the underlying stragety's wtihdrawal fee calcuation.

The number reported by BeefAdapter.sol is previewWithdraw and previewRedeem is wrong.

Tools Used

Manual Review

Recommended Mitigation Steps

We reommend the protocol make the stragety withdrawal fee estimation match the Beefy stragety estimation. This can be difficult to implementation, the protocol may need to may the stragety name to the withdrawal fee.

[c4-judge]

dmvt marked the issue as duplicate of #370

[c4-sponsor]

RedVeil marked the issue as sponsor confirmed

[c4-judge]

dmvt marked the issue as not a duplicate

[c4-judge]

dmvt changed the severity to QA (Quality Assurance)

[c4-judge]

dmvt marked the issue as grade-b