Revert Lend - deepplus's results

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Vault.sol#L1250-L1251 https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Vault.sol#L1262-L1263

Vulnerability details

Impact

Since the daily increase limit for borrowing and depositing is calculated as 110% of total assets, the intention to apply daily lend/debt limit would be broken.

Proof of Concept

In this protocol, they introduced the idea of Max Daily Debt Increase(MDDI) that ensures that maximum loan amount can only grow by 10% of the total deposited assets in 24-hour period. Following is the statements and example of protocol's whitepaper which explain MDDI.

The Max Daily Debt Increase (MDDI) determines the maximum increase in the total value of loans issued by the protocol within a 24-hour period. Set at 10%, this cap ensures that the maximum loan amount provided can only grow by 10% of the total stablecoin deposits in any given 24-hour period. MDDI functions as an emergency backstop to protect in case of a catastrophic exploit, whether due to unforeseen errors in the protocol’s logic or the exploitation of counterparty risk in one of the collateral tokens. For instance, consider a scenario where a malicious actor discovers a vulnerability allowing them to manipulate the price of a collateral token. With MDDI in place, the maximum that the total loan amount could increase due to such an exploit is capped. For example, if the total lending pool is 25 million USD, the MDDI restricts any increase in loans to a maximum of 2.5 million USD (10% of the total lending deposits) in a 24-hour period. In the event of such a catastrophic exploit, this means the potential loss for the lending pool is limited to the MDDI amount.

However, the actual calculated increase limit is different from above content.

        if (force || time > dailyDebtIncreaseLimitLastReset) {
@>          uint256 debtIncreaseLimit = _convertToAssets(totalSupply(), newLendExchangeRateX96, Math.Rounding.Up)
@>              * (Q32 + MAX_DAILY_DEBT_INCREASE_X32) / Q32; // @audit calced as 110%
            dailyDebtIncreaseLimitLeft =
                dailyDebtIncreaseLimitMin > debtIncreaseLimit ? dailyDebtIncreaseLimitMin : debtIncreaseLimit;
            dailyDebtIncreaseLimitLastReset = time;
        }

Hence, the maximum limit that owners of LP positions can borrow everyday is calculated as 110% of actual total assets of vault. This limit is meaningless indeed. (dailyDebtIncreaseLimitMin is the minimum limit to enable borrowers can borrow assets in extreme situation that total assets is too small, so cannot affect this issue.) Calculating lendIncreaseLimit has similar problem. This breaks the protocol's intention to prevent exploitation in lending and borrowing.

Tools Used

Manual Review

Recommended Mitigation Steps

debtIncreaseLimit and lendIncreaseLimit should be calculated correctly as whitepaper explained:

        if (force || time > dailyDebtIncreaseLimitLastReset) {
            uint256 debtIncreaseLimit = _convertToAssets(totalSupply(), newLendExchangeRateX96, Math.Rounding.Up)
-               * (Q32 + MAX_DAILY_DEBT_INCREASE_X32) / Q32;
+               * MAX_DAILY_DEBT_INCREASE_X32 / Q32;
            dailyDebtIncreaseLimitLeft =
                dailyDebtIncreaseLimitMin > debtIncreaseLimit ? dailyDebtIncreaseLimitMin : debtIncreaseLimit;
            dailyDebtIncreaseLimitLastReset = time;
        }

        if (force || time > dailyLendIncreaseLimitLastReset) {
            uint256 lendIncreaseLimit = _convertToAssets(totalSupply(), newLendExchangeRateX96, Math.Rounding.Up)
-               * (Q32 + MAX_DAILY_LEND_INCREASE_X32) / Q32;
+               * MAX_DAILY_LEND_INCREASE_X32 / Q32;
            dailyLendIncreaseLimitLeft=
                dailyLendIncreaseLimitMin > lendIncreaseLimit ? dailyLendIncreaseLimitMin : lendIncreaseLimit;
            dailyLendIncreaseLimitLastReset = time;
        }

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/automators/AutoExit.sol#L154-L157 https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/automators/Automator.sol#L193-L215

Vulnerability details

Impact

Since the feeAmount also includes assets from removed liquidity, the reward calculated from feeAmount is incorrect when onlyFees of config is true.

Proof of Concept

When the onlyFees is true for active position in AutoExit, the reward should be calculated only from fees.

    // decrease full liquidity for given position - and return fees as well
@>  (state.amount0, state.amount1, state.feeAmount0, state.feeAmount1) = _decreaseFullLiquidityAndCollect(
        params.tokenId, state.liquidity, params.amountRemoveMin0, params.amountRemoveMin1, params.deadline
    );
    if (state.isSwap) {
        if (params.swapData.length == 0) {
            revert MissingSwapData();
        }

        // reward is taken before swap - if from fees only
        if (config.onlyFees) {
@>          state.amount0 -= state.feeAmount0 * params.rewardX64 / Q64;
@>          state.amount1 -= state.feeAmount1 * params.rewardX64 / Q64;
        }

However, above feeAmount0 and feeAmount1 are including not only fees but also token assets by decreasing the liquidity from position.

    function _decreaseFullLiquidityAndCollect(
        uint256 tokenId,
        uint128 liquidity,
        uint256 amountRemoveMin0,
        uint256 amountRemoveMin1,
        uint256 deadline
    ) internal returns (uint256 amount0, uint256 amount1, uint256 feeAmount0, uint256 feeAmount1) {
        if (liquidity > 0) {
            // store in temporarely "misnamed" variables - see comment below
@>          (feeAmount0, feeAmount1) = nonfungiblePositionManager.decreaseLiquidity(
                INonfungiblePositionManager.DecreaseLiquidityParams(
                    tokenId, liquidity, amountRemoveMin0, amountRemoveMin1, deadline
                )
            );
        }
@>      (amount0, amount1) = nonfungiblePositionManager.collect(
            INonfungiblePositionManager.CollectParams(tokenId, address(this), type(uint128).max, type(uint128).max)
        );

        // fee amount is what was collected additionally to liquidity amount
@>      feeAmount0 = amount0 - feeAmount0;
@>      feeAmount1 = amount1 - feeAmount1;
    }

The feeAmount represents unpaid fees excluding assets derived from the current liquidity but comprises the owned amount. Meanwhile, the owned amount encompasses unpaid assets acquired not merely from fees but also from a prior decreaseLiquidity() call. In the situation where the owner had invoked decreaseLiquidity() earlier, the unpaid assets may include some of the withdrawn liquidity, possibly accounting for a majority. Consequently, the reward of onlyFees config is not correct and more reward than expected will be collected when the operator calls execute().

Tools Used

Manual Review

Recommended Mitigation Steps

feeAmount should includes only fees calculated from feeGrowthInside.

Assessed type

Other