Depositing assets doesn't contain a slippage check

[c4-bot-8]

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/main/src/V3Vault.sol?plain=1#L877-L917
https://github.com/code-423n4/2024-03-revert-lend/blob/main/src/V3Vault.sol?plain=1#L1281-L1295

Vulnerability details

Impact

When a user calls V3Vault.mint() or V3Vault.deposit(), the user is unable to provide slippage arguments to ensure that they receive at least X amount of lending shares. If their transaction is delayed before it's executed or is frontrun, the user may receive less lending shares than expected.

Proof of Concept

Below is the functionality for _deposit() which calculates how many lending shares to mint and contains no slippage checks:

function _deposit(address receiver, uint256 amount, bool isShare, bytes memory permitData)
    internal
    returns (uint256 assets, uint256 shares)
{
    (, uint256 newLendExchangeRateX96) = _updateGlobalInterest();

    _resetDailyLendIncreaseLimit(newLendExchangeRateX96, false);

    if (isShare) {
        shares = amount;
        assets = _convertToAssets(shares, newLendExchangeRateX96, Math.Rounding.Up);
    } else {
        assets = amount;
        shares = _convertToShares(assets, newLendExchangeRateX96, Math.Rounding.Down);
    }

    if (permitData.length > 0) {
        (ISignatureTransfer.PermitTransferFrom memory permit, bytes memory signature) =
            abi.decode(permitData, (ISignatureTransfer.PermitTransferFrom, bytes));
        permit2.permitTransferFrom(
            permit, ISignatureTransfer.SignatureTransferDetails(address(this), assets), msg.sender, signature
        );
    } else {
        // fails if not enough token approved
        SafeERC20.safeTransferFrom(IERC20(asset), msg.sender, address(this), assets);
    }

    _mint(receiver, shares);

    if (totalSupply() > globalLendLimit) {
        revert GlobalLendLimit();
    }

    if (assets > dailyLendIncreaseLimitLeft) {
        revert DailyLendIncreaseLimit();
    } else {
        dailyLendIncreaseLimitLeft -= assets;
    }

    emit Deposit(msg.sender, receiver, assets, shares);
}

To determine the amount of shares to mint for the user in _deposit(), the protocol utilizes the newLendExchangeRateX96 to calculate how many shares to mint:

if (isShare) {
    shares = amount;
    assets = _convertToAssets(shares, newLendExchangeRateX96, Math.Rounding.Up);
} else {
    assets = amount;
    shares = _convertToShares(assets, newLendExchangeRateX96, Math.Rounding.Down);
}

function _convertToShares(uint256 amount, uint256 exchangeRateX96, Math.Rounding rounding)
    internal
    pure
    returns (uint256)
{
    return amount.mulDiv(Q96, exchangeRateX96, rounding);
}

function _convertToAssets(uint256 shares, uint256 exchangeRateX96, Math.Rounding rounding)
    internal
    pure
    returns (uint256)
{
    return shares.mulDiv(exchangeRateX96, Q96, rounding);
}

Since the newLendExchangeRateX96 will update over time (see _calculateGlobalInterest() code snippet below), the amount of shares minted will decrease over time. This leads to a user experiencing slippage.

// always growing or equal
uint256 lastRateUpdate = lastExchangeRateUpdate;

if (lastRateUpdate > 0) {
    newDebtExchangeRateX96 = oldDebtExchangeRateX96
        + oldDebtExchangeRateX96 * (block.timestamp - lastRateUpdate) * borrowRateX96 / Q96;
    // lend exchange rate changes over time
    newLendExchangeRateX96 = oldLendExchangeRateX96
        + oldLendExchangeRateX96 * (block.timestamp - lastRateUpdate) * supplyRateX96 / Q96;
} else {
    newDebtExchangeRateX96 = oldDebtExchangeRateX96;
    newLendExchangeRateX96 = oldLendExchangeRateX96;
}

Tools Used

Manual inspection.

Recommended Mitigation Steps

Allow the user to provide minimum slippage checks when calling mint() (slippage check against assets required to create X amount of shares) or deposit() (slippage checks against shares minted). This will ensure that the user receives at least X amount of lending shares when calling mint() or deposit().

Assessed type

Timing

[c4-pre-sort]

0xEVom marked the issue as sufficient quality report

[c4-pre-sort]

0xEVom marked the issue as duplicate of #281

[c4-judge]

jhsagd76 changed the severity to QA (Quality Assurance)

[c4-judge]

This previously downgraded issue has been upgraded by jhsagd76

[c4-judge]

jhsagd76 changed the severity to QA (Quality Assurance)