Revert Lend - KupiaSec's results

Lines of code

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

Vulnerability details

Impact

_resetDailyLendIncreaseLimit() function will return incorrect dailyLendIncreaseLimitLeft and _resetDailyDebtIncreaseLimit function will return incorrect dailyDebtIncreaseLimitLeft and users deposit and lend more allowed within a day than intended

Proof of Concept

    function _resetDailyLendIncreaseLimit(uint256 newLendExchangeRateX96, bool force) internal {
        // daily lend limit reset handling
        uint256 time = block.timestamp / 1 days;
        if (force || time > dailyLendIncreaseLimitLastReset) {
1250        uint256 lendIncreaseLimit = _convertToAssets(totalSupply(), newLendExchangeRateX96, Math.Rounding.Up)
                * (Q32 + MAX_DAILY_LEND_INCREASE_X32) / Q32;
            dailyLendIncreaseLimitLeft =
                dailyLendIncreaseLimitMin > lendIncreaseLimit ? dailyLendIncreaseLimitMin : lendIncreaseLimit;
            dailyLendIncreaseLimitLastReset = time;
        }
    }

As you can see in the L1250, the limit becomes 110%, not 10% and I guess it is not intended.

This goes the same for _resetDailyDebtIncreaseLimit() function

Tools Used

Manual Review

Recommended Mitigation Steps

    function _resetDailyLendIncreaseLimit(uint256 newLendExchangeRateX96, bool force) internal {
        // daily lend limit reset handling
        uint256 time = block.timestamp / 1 days;
        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;
        }
    }

    function _resetDailyDebtIncreaseLimit(uint256 newLendExchangeRateX96, bool force) internal {
        // daily debt limit reset handling
        uint256 time = block.timestamp / 1 days;
        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;
        }
    }

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Oracle.sol#L304

Vulnerability details

Impact

In the _getReferenceTokenPriceX96() function of the contract, at L304 overflow could occur and this will the function revert.

Proof of Concept

    function _getReferenceTokenPriceX96(address token, uint256 cachedChainlinkReferencePriceX96)
        internal
        view
        returns (uint256 priceX96, uint256 chainlinkReferencePriceX96)
    {
        if (token == referenceToken) {
            return (Q96, chainlinkReferencePriceX96);
        }

        TokenConfig memory feedConfig = feedConfigs[token];

        if (feedConfig.mode == Mode.NOT_SET) {
            revert NotConfigured();
        }

        uint256 verifyPriceX96;

        bool usesChainlink = (
            feedConfig.mode == Mode.CHAINLINK_TWAP_VERIFY || feedConfig.mode == Mode.TWAP_CHAINLINK_VERIFY
                || feedConfig.mode == Mode.CHAINLINK
        );
        bool usesTWAP = (
            feedConfig.mode == Mode.CHAINLINK_TWAP_VERIFY || feedConfig.mode == Mode.TWAP_CHAINLINK_VERIFY
                || feedConfig.mode == Mode.TWAP
        );

        if (usesChainlink) {
            uint256 chainlinkPriceX96 = _getChainlinkPriceX96(token);
            chainlinkReferencePriceX96 = cachedChainlinkReferencePriceX96 == 0
                ? _getChainlinkPriceX96(referenceToken)
                : cachedChainlinkReferencePriceX96;

304         chainlinkPriceX96 = (10 ** referenceTokenDecimals) * chainlinkPriceX96 * Q96 / chainlinkReferencePriceX96
                / (10 ** feedConfig.tokenDecimals);

            if (feedConfig.mode == Mode.TWAP_CHAINLINK_VERIFY) {
                verifyPriceX96 = chainlinkPriceX96;
            } else {
                priceX96 = chainlinkPriceX96;
            }
        }

        if (usesTWAP) {
            uint256 twapPriceX96 = _getTWAPPriceX96(feedConfig);
            if (feedConfig.mode == Mode.CHAINLINK_TWAP_VERIFY) {
                verifyPriceX96 = twapPriceX96;
            } else {
                priceX96 = twapPriceX96;
            }
        }

        if (feedConfig.mode == Mode.CHAINLINK_TWAP_VERIFY || feedConfig.mode == Mode.TWAP_CHAINLINK_VERIFY) {
            _requireMaxDifference(priceX96, verifyPriceX96, feedConfig.maxDifference);
        }
    }

At L304, if the referenceToken = ETH and token = BTC for example, the chainlinkPriceX96 is 70,000 * 2 ** 96, and referenceTokenDecimal = 18 and this is enough for overflow and this function will revert.

Plus other functions which is calling this function directly or indirectly will revert.

• V3Oracle.getValue().
• V3Oracle.getValue()
• V3Vault._checkLoanIsHealthy()
• V3Vault._requireLoanIsHealthy()
• V3Vault.liquidate()
• V3Vault.decreaseLiquidityAndCollect()
• V3Vault.borrow()
• V3Vault.transform()
• V3Vault.loanInfo()

Tools Used

Manual Review

Recommended Mitigation Steps

You can use FullMath library to prevent overflow here.

Assessed type

Under/Overflow

Lines of code

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

Vulnerability details

Impact

In the overall codebase totalSupply is for shares and globalLendLimit is for assets. But in the _deposit() function at L906 totalSupply is compared to the globalLendLimit and this should not happen and assets can be lent more than the globalLendLimit.

Proof of Concept

    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);

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

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

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

totalSupply stands for shares and globalLendLimit stands for assets and they are completely different terms. They must not be compared together or assets could be lent more than globalLendLimit.

Tools Used

Manual Review

Recommended Mitigation Steps

Should convert totalSupply to assets and then compare with globalLendLimit

    function _deposit(address receiver, uint256 amount, bool isShare, bytes memory permitData)
        internal
        returns (uint256 assets, uint256 shares)
    {
        /// lines of code

        _mint(receiver, shares);

-       if (totalSupply() > globalLendLimit) {
+       if (_convertToAssets(totalSupply(), lendExchangeRateX96, Math.Rounding.Down) > globalLendLimit) {
            revert GlobalLendLimit();
        }

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

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

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/automators/AutoExit.sol#L143

Vulnerability details

Impact

If onlyFee is set as false, the protocol should collect reward from only the fees. However if the borrowers(token owners) decrease the liquity for some reason, the decreased amount is added to the feeAmount0 and feeAmount1 and it is treated and fee. And this can be included in the calculation for reward, if the borrowers doesn't take the amount in time.

Proof of Concept

When execute function of the AutoExit contract is called, at [L143] some values are calculated and later used for reward.

        (state.amount0, state.amount1, state.feeAmount0, state.feeAmount1) = _decreaseFullLiquidityAndCollect(
            params.tokenId, state.liquidity, params.amountRemoveMin0, params.amountRemoveMin1, params.deadline
        );

If onlyFee is set True, reward is calculated as follows.

    if (config.onlyFees) {
            state.amount0 -= state.feeAmount0 * params.rewardX64 / Q64;
            state.amount1 -= state.feeAmount1 * params.rewardX64 / Q64;
        }

Above state.feeAmount0 and state.feeAmount1 is calculated like following.

    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;
    }

feeAmount0 and feeAmount1 are the amounts for the uncollected ones. But if the liquidity owner decreases the liquidity for some reason, the feeAmount0 and feeAmount1 contains not only but also the decreased the liquidity amount. So in case of the owner doesn't collect this amount Automator.execute is called, the liquidity owner should pay more reward than expected without even knowing

Tools Used

Manual Review

Recommended Mitigation Steps

feeAmount0 and feeAmount1 should be calculated correctly not to include the decreased liquidity. Maybe feeGrowthInside in the UniswapV3, could be used

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

There are mainly two ways for borrowers to repay debt fully.

• Call V3Vault.repay() or V3Vault.repay()
• Or use the LeverageTransformer.leverageDown. The second method is to use method V3Vault.transform to call the LeverageTransformer.leverageDown but if borrowers use this method to repay debt fully it will always revert.

Proof of Concept

Borrowers are able to call LeverageTransformer.leverageDown function using V3Vault.transform function.

And if they try to repay debt fully using the LeverageTransformer.leverageDown function, [V3Vault.repay] function is called.

    SafeERC20.safeApprove(IERC20(token), msg.sender, amount);
    IVault(msg.sender).repay(params.tokenId, amount, false);    

_cleanupLoan() function is called and the NFT token is return to the owner and vault is no longer the owner of the token. However in the transform function, after making the call there is a check for ownership of the issued NFT token.

    (bool success,) = transformer.call(data);
    if (!success) {
        revert TransformFailed();
    }

    // may have changed in the meantime
    tokenId = transformedTokenId;

    // check owner not changed (NEEDED because token could have been moved somewhere else in the meantime)
531 address owner = nonfungiblePositionManager.ownerOf(tokenId);
532 if (owner != address(this)) {
        revert Unauthorized();
    }

At L531, the owner is not the vault itself and the owner is the original token owner. And this will make revert in the following check at L532

Tools Used

Manual Review

Recommended Mitigation Steps

After call in the transform function, it would be good to add check for the NFT token debt and skip the check at At L531-534, if possible. But I am not sure it is not good way and it would be good for borrowers to use V3Vault.repay for repaying debt fully.

Assessed type

ERC721