c4-bot-2 commented 6 months ago

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/main/src/V3Vault.sol#L906-L908

Vulnerability details

Issue Description

The _deposit function is invoked in both the deposit and mint functions when a user wants to lend assets. This function is intended to ensure that the total amount of assets lent does not exceed the protocol limit globalLendLimit.

function _deposit(
    address receiver,
    uint256 amount,
    bool isShare,
    bytes memory permitData
) internal returns (uint256 assets, uint256 shares) {
    ...

    _mint(receiver, shares);

    //@audit must convert totalSupply() to assets before comparing with globalLendLimit
    if (totalSupply() > globalLendLimit) {
        revert GlobalLendLimit();
    }

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

In the provided code snippet, the _deposit function checks the totalSupply() against the globalLendLimit limit. However, totalSupply() represents the lenders' share amount and does not represent the actual asset amount lent. It must first be converted to assets using the _convertToAssets function.

This mistake is evident because in the maxDeposit function, the correct check is implemented:

function maxDeposit(address) external view override returns (uint256) {
    (, uint256 lendExchangeRateX96) = _calculateGlobalInterest();
    uint256 value = _convertToAssets(
        totalSupply(),
        lendExchangeRateX96,
        Math.Rounding.Up
    );
    if (value >= globalLendLimit) {
        return 0;
    } else {
        return globalLendLimit - value;
    }
}

Because the _deposit function performs the wrong check, it will allow more assets to be lent in the protocol. This is due to the fact that the lending exchange rate lastLendExchangeRateX96 will be greater than 1 (due to interest accrual), and so we will always have totalSupply() < _convertToAssets(totalSupply(), lendExchangeRateX96, Math.Rounding.Up) (the only case this might not hold is when there is a significant bad debt after liquidation, which would not occur under normal circumstances).

Impact

Incorrect global lending checking in the _deposit function will result in more assets being lent than allowed by the protocol.

Tools Used

Manual review, VS Code

Recommended Mitigation

To address this issue, the totalSupply() must be converted to an asset amount before checking it against globalLendLimit, as is done in the maxDeposit function:

function _deposit(
    address receiver,
    uint256 amount,
    bool isShare,
    bytes memory permitData
) internal returns (uint256 assets, uint256 shares) {
    ...

    _mint(receiver, shares);

++  //@audit must convert totalSupply() to assets before comparing with globalLendLimit
++  uint256 value = _convertToAssets(totalSupply(), newLendExchangeRateX96, Math.Rounding.Up);
++  if (value > globalLendLimit) {
--  if (totalSupply() > globalLendLimit) {
        revert GlobalLendLimit();
    }

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