Revert Lend - kennedy1030's results

Lines of code

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

Vulnerability details

Impact

Users are able to deposit more asset tokens than the protocol intended within a day.

Proof of Concept

In V3Vault._resetDailyLendIncreaseLimit(), there is a computation for lendIncreaseLimit at L1250.

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

In actuality, lendIncreaseLimit should be 10%(MAX_DAILY_LEND_INCREASE_X32) of the total amount of lent asset tokens.

This problem also occurs in V3Vault._resetDailyDebtIncreaseLimit().

Tools Used

Manual review

Recommended Mitigation Steps

L1251 of V3Vault._resetDailyLendIncreaseLimit() should be modified as follows.

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

L1263 of V3Vault._resetDailyDebtIncreaseLimit() should be also modified as follows.

    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/main/src/V3Oracle.sol#L272-L326 https://github.com/code-423n4/2024-03-revert-lend/blob/main/src/V3Oracle.sol#L95-L131 https://github.com/code-423n4/2024-03-revert-lend/blob/main/src/V3Vault.sol#L1270-L1279

Vulnerability details

Impact

Integer overflow occurs in V3Oracle._getReferenceTokenPriceX96() for specific tokens.

So this protocol might not be compatible with specific tokens.

Proof of Concept

At L304 of V3Oracle._getReferenceTokenPriceX96(), there is a computation for the chainlinkPriceX96 which can make an integer overflow.

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

V3Oracle._getReferenceTokenPriceX96() is invoked by V3Oracle.getValue().

V3Oracle.getValue() is directly called by V3Vault._checkLoanIsHealthy(), and indirectly by V3Vault._requireLoanIsHealthy(), V3Vault.liquidate(), V3Vault.decreaseLiquidityAndCollect(), V3Vault.borrow(), and V3Vault.transform().

Let’s take a simple scenario to highlight this issue: referenceToken = DAI, token = WETH

In this scenario, referenceTokenDecimals = 18 and chainlinkPriceX96 is approximately 4000 * 2 ** 96, leading to an integer overflow.

Tools Used

Manual review

Recommended Mitigation Steps

L304 of V3Oracle._getReferenceTokenPriceX96() should be fixed to prevent integer overflow.

Assessed type

Under/Overflow

Lines of code

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

Vulnerability details

Impact

The total amount of lent asset tokens could exceed the globalLendLimit.

Proof of Concept

In V3Vault._deposit(), there is a validation for the totalSupply at L906.

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

In fact, globalLendLimit is the maximum limit for the total amount of lent asset tokens, not for the totalSupply.

Tools Used

Manual review

Recommended Mitigation Steps

L906 of V3Vault._deposit() should be modified as follows.

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

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

        [...]
    }

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/main/src/automators/AutoExit.sol#L100-L214 https://github.com/code-423n4/2024-03-revert-lend/blob/main/src/automators/Automator.sol#L193-L215

Vulnerability details

Impact

The owner of the NFT could end up paying more rewards to AutoExit than anticipated when onlyFee is set to true.

Proof of Concept

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

    function execute(ExecuteParams calldata params) external {
        [...]

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

        [...]
    }

https://github.com/code-423n4/2024-03-revert-lend/blob/main/src/automators/Automator.sol#L208

    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
202         (feeAmount0, feeAmount1) = nonfungiblePositionManager.decreaseLiquidity(
                INonfungiblePositionManager.DecreaseLiquidityParams(
                    tokenId, liquidity, amountRemoveMin0, amountRemoveMin1, deadline
                )
            );
        }
208     (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;
    }

As seen at L208, feeAmount represents the uncollected fees excluding assets from the current liquidity. However, it includes the owed amount, which comprises uncollected assets not just from fees but also from nonfungiblePositionManager.decreaseLiquidity() called earlier at L202. If the owner has already executed nonfungiblePositionManager.decreaseLiquidity(), the uncollected assets would consist of some assets withdrawn from their liquidity, possibly a significant portion. This implies that onlyFee configuration is not functioning effectively.

Here is a simple scenario to highlight this issue:

1. The owner invokes nonfungiblePositionManager.approve(address(autoExit), NFT) and sets onlyFee to true.
2. The owner then calls nonfungiblePositionManager.decreaseLiquidity() to withdraw the majority of their liquidity..
3. Subsequently, an operator calls autoExit.execute() and receives more rewards than anticipated.

Tools used

Manual Review

Recommended Mitigation Steps

In Automator._decreaseFullLiquidityAndCollect(), feeAmount0, feeAmount1 must only include the amount calculated from the feeGrowthInside of UniswapV3 position .

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/main/src/transformers/LeverageTransformer.sol#L123-L175 https://github.com/code-423n4/2024-03-revert-lend/blob/main/src/V3Vault.sol#L497-L545 https://github.com/code-423n4/2024-03-revert-lend/blob/main/src/V3Vault.sol#L954-L1014 https://github.com/code-423n4/2024-03-revert-lend/blob/main/src/V3Vault.sol#L1077-L1085

Vulnerability details

Impact

The tokenOwner[tokenId] is unable to execute a full settlement using LeverageTransformer.leverageDown().

Proof of Concept

At L532 of V3Vault.transform(), there is a validation for the owner of the 'tokenId'.

    function transform(uint256 tokenId, address transformer, bytes calldata data)
        external
        override
        returns (uint256 newTokenId)
    {
        [...]

        // give access to transformer
        nonfungiblePositionManager.approve(transformer, tokenId);

        (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)
        address owner = nonfungiblePositionManager.ownerOf(tokenId);
532     if (owner != address(this)) {
            revert Unauthorized();
        }

        [...]
    }

In the event of a full settlement, V3Vault._repay() calls the function _cleanupLoan(), resulting in the alteration of the owner of the tokenId from V3Vault to tokenOwner[tokenId].

As a consequence, the validation for the owner of the 'tokenId' fails in V3Vault.transform() when the tokenOwner[tokenId] attempts a full settlement through LeverageTransformer.leverageDown().

Tools used

Manual Review

Recommended Mitigation Steps

The validation for the owner of the 'tokenId' at V3Vault.transform() should be as follows.

    function transform(uint256 tokenId, address transformer, bytes calldata data)
        external
        override
        returns (uint256 newTokenId)
    {
        [...]

        // check owner not changed (NEEDED because token could have been moved somewhere else in the meantime)
        address owner = nonfungiblePositionManager.ownerOf(tokenId);
-       if (owner != address(this)) {
+       if (owner != address(this) && loans[tokenId].debtShares != 0) {
            revert Unauthorized();
        }

        [...]
    }

Assessed type

Other