c4-bot-4 commented 9 months ago

Lines of code

https://github.com/code-423n4/2023-12-particle/blob/main/contracts/protocol/ParticlePositionManager.sol#L423-L439

Vulnerability details

Impact

When the position is closed by the traders or due to liquidation, the required amount of tokens to be paid is calculated and added back to the LP Uniswap V3 positions. However, the leftover tokens from the increasing liquidity operation are not accounted properly.

Proof of Concept

When closePosition or liquidatePosition is called, it will eventually trigger _closePosition, to calculate the cache.amountToAdd and cache.amountFromAdd and increase the LP uniswap v3 position by providing those value.

https://github.com/code-423n4/2023-12-particle/blob/main/contracts/protocol/ParticlePositionManager.sol#L410-L439

    function _closePosition(
        DataStruct.ClosePositionParams calldata params,
        DataCache.ClosePositionCache memory cache,
        Lien.Info memory lien,
        address borrower
    ) internal {
        // optimistically use the input numbers to swap for repay
        /// @dev amountSwap overspend will be caught by refundWithCheck step in below
        (cache.amountSpent, cache.amountReceived) = Base.swap(
            cache.tokenFrom,
            cache.tokenTo,
            params.amountSwap,
            0, /// @dev we check cache.amountReceived is sufficient to repay LP in below
            DEX_AGGREGATOR,
            params.data
        );

        // based on borrowed liquidity, compute the required return amount
        /// @dev the from-to swapping direction is reverted compared to openPosition
>>>     (cache.amountToAdd, cache.amountFromAdd) = Base.getRequiredRepay(lien.liquidity, lien.tokenId);
        if (!lien.zeroForOne) (cache.amountToAdd, cache.amountFromAdd) = (cache.amountFromAdd, cache.amountToAdd);

        // the liquidity to add must be no less than the available amount
        /// @dev the max available amount contains the tokensOwed, will have another check in below at refundWithCheck
        if (
            cache.amountFromAdd > cache.collateralFrom + cache.tokenFromPremium - cache.amountSpent ||
            cache.amountToAdd > cache.amountReceived + cache.tokenToPremium
        ) {
            revert Errors.InsufficientRepay();
        }

        // add liquidity back to borrower
        if (lien.zeroForOne) {
>>>         (cache.liquidityAdded, cache.amountToAdd, cache.amountFromAdd) = LiquidityPosition.increaseLiquidity(
                cache.tokenTo,
                cache.tokenFrom,
                lien.tokenId,
                cache.amountToAdd,
                cache.amountFromAdd
            );
        } else {
>>>         (cache.liquidityAdded, cache.amountFromAdd, cache.amountToAdd) = LiquidityPosition.increaseLiquidity(
                cache.tokenFrom,
                cache.tokenTo,
                lien.tokenId,
                cache.amountFromAdd,
                cache.amountToAdd
            );
        }

        // obtain the position's latest FeeGrowthInside after increaseLiquidity
        (, , , , , , , , cache.feeGrowthInside0LastX128, cache.feeGrowthInside1LastX128, , ) = Base
            .UNI_POSITION_MANAGER
            .positions(lien.tokenId);

        // caculate the amounts owed since last fee collection during the borrowing period
        (cache.token0Owed, cache.token1Owed) = Base.getOwedFee(
            cache.feeGrowthInside0LastX128,
            cache.feeGrowthInside1LastX128,
            lien.feeGrowthInside0LastX128,
            lien.feeGrowthInside1LastX128,
            lien.liquidity
        );

        // calculate the the amounts owed to LP up to the premium in the lien
        // must ensure enough amount is left to pay for interest first, then send gains and fund left to borrower
        ///@dev refundWithCheck ensures actual cannot be more than expected, since amount owed to LP is in actual,
        ///     it ensures (1) on the collateralFrom part of refund, tokenOwed is covered, and (2) on the amountReceived
        ///      part, received is no less than liquidity addback + token owed.
        if (lien.zeroForOne) {
            cache.token0Owed = cache.token0Owed < cache.tokenToPremium ? cache.token0Owed : cache.tokenToPremium;
            cache.token1Owed = cache.token1Owed < cache.tokenFromPremium ? cache.token1Owed : cache.tokenFromPremium;
            Base.refundWithCheck(
                borrower,
                cache.tokenFrom,
                cache.collateralFrom + cache.tokenFromPremium,
                cache.amountSpent + cache.amountFromAdd + cache.token1Owed
            );
            Base.refundWithCheck(
                borrower,
                cache.tokenTo,
                cache.amountReceived + cache.tokenToPremium,
                cache.amountToAdd + cache.token0Owed
            );
        } else {
            cache.token0Owed = cache.token0Owed < cache.tokenFromPremium ? cache.token0Owed : cache.tokenFromPremium;
            cache.token1Owed = cache.token1Owed < cache.tokenToPremium ? cache.token1Owed : cache.tokenToPremium;
            Base.refundWithCheck(
                borrower,
                cache.tokenFrom,
                cache.collateralFrom + cache.tokenFromPremium,
                cache.amountSpent + cache.amountFromAdd + cache.token0Owed
            );
            Base.refundWithCheck(
                borrower,
                cache.tokenTo,
                cache.amountReceived + cache.tokenToPremium,
                cache.amountToAdd + cache.token1Owed
            );
        }

        // pay for interest
        lps.addTokensOwed(lien.tokenId, cache.token0Owed, cache.token1Owed);
    }

https://github.com/code-423n4/2023-12-particle/blob/main/contracts/libraries/LiquidityPosition.sol#L178-L204

    function increaseLiquidity(
        address token0,
        address token1,
        uint256 tokenId,
        uint256 amount0,
        uint256 amount1
    ) internal returns (uint128 liquidity, uint256 amount0Added, uint256 amount1Added) {
        // approve spending for uniswap's position manager
        TransferHelper.safeApprove(token0, Base.UNI_POSITION_MANAGER_ADDR, amount0);
        TransferHelper.safeApprove(token1, Base.UNI_POSITION_MANAGER_ADDR, amount1);

        // increase liquidity via position manager
        (liquidity, amount0Added, amount1Added) = Base.UNI_POSITION_MANAGER.increaseLiquidity(
            INonfungiblePositionManager.IncreaseLiquidityParams({
                tokenId: tokenId,
                amount0Desired: amount0,
                amount1Desired: amount1,
                amount0Min: 0,
                amount1Min: 0,
                deadline: block.timestamp
            })
        );

        // reset approval
        TransferHelper.safeApprove(token0, Base.UNI_POSITION_MANAGER_ADDR, 0);
        TransferHelper.safeApprove(token1, Base.UNI_POSITION_MANAGER_ADDR, 0);
    }

It can be observed that instead of tracking the actual amounts used for the increaseLiquidity liquidity operation, it directly update cache.amountFromAdd and cache.amountToAdd instead. so the unused amount (desired vs actual) is not tracked and stuck inside the contract.

Tools Used

Manual review

Recommended Mitigation Steps

Instead of directly updating cache.amountFromAdd and cache.amountToAdd using the actual amount from increaseLiquidity, track the unsused amount and add it to the token owed for the LP.

    function _closePosition(
        DataStruct.ClosePositionParams calldata params,
        DataCache.ClosePositionCache memory cache,
        Lien.Info memory lien,
        address borrower
    ) internal {
        // optimistically use the input numbers to swap for repay
        /// @dev amountSwap overspend will be caught by refundWithCheck step in below
        (cache.amountSpent, cache.amountReceived) = Base.swap(
            cache.tokenFrom,
            cache.tokenTo,
            params.amountSwap,
            0, /// @dev we check cache.amountReceived is sufficient to repay LP in below
            DEX_AGGREGATOR,
            params.data
        );

        // based on borrowed liquidity, compute the required return amount
        /// @dev the from-to swapping direction is reverted compared to openPosition
        (cache.amountToAdd, cache.amountFromAdd) = Base.getRequiredRepay(lien.liquidity, lien.tokenId);
        if (!lien.zeroForOne) (cache.amountToAdd, cache.amountFromAdd) = (cache.amountFromAdd, cache.amountToAdd);

        // the liquidity to add must be no less than the available amount
        /// @dev the max available amount contains the tokensOwed, will have another check in below at refundWithCheck
        if (
            cache.amountFromAdd > cache.collateralFrom + cache.tokenFromPremium - cache.amountSpent ||
            cache.amountToAdd > cache.amountReceived + cache.tokenToPremium
        ) {
            revert Errors.InsufficientRepay();
        }

        // add liquidity back to borrower
+        uint256 amountToAddActual;
+        uint256 amountFromAddActual;
+        uint128 extraToken0Owed;
+        uint128 extraToken1Owed;
        if (lien.zeroForOne) {
-            (cache.liquidityAdded, cache.amountToAdd, cache.amountFromAdd) = LiquidityPosition.increaseLiquidity(
-                cache.tokenTo,
-                cache.tokenFrom,
-                lien.tokenId,
-                cache.amountToAdd,
-                cache.amountFromAdd
-            );
+            (cache.liquidityAdded, amountToAddActual, amountFromAddActual) = LiquidityPosition.increaseLiquidity(
+                cache.tokenTo,
+                cache.tokenFrom,
+                lien.tokenId,
+                cache.amountToAdd,
+                cache.amountFromAdd
+            );
+            if (cache.amountToAdd > amountToAddActual) {
+                extraToken0Owed += uint128(cache.amountToAdd - amountToAddActual);
+            }
+
+           if (cache.amountFromAdd > amountFromAddActual) {
+                extraToken1Owed += uint128(cache.amountFromAdd - amountFromAddActual);
+            }
+            cache.amountToAdd = amountToAddActual;
+            cache.amountFromAdd = amountFromAddActual;
        } else {
-            (cache.liquidityAdded, cache.amountFromAdd, cache.amountToAdd) = LiquidityPosition.increaseLiquidity(
-                cache.tokenFrom,
-                cache.tokenTo,
-                lien.tokenId,
-                cache.amountFromAdd,
-                cache.amountToAdd
-            );
+            (cache.liquidityAdded, amountFromAddActual, amountToAddActual) = LiquidityPosition.increaseLiquidity(
+                cache.tokenFrom,
+                cache.tokenTo,
+                lien.tokenId,
+                cache.amountFromAdd,
+                cache.amountToAdd
+            );
+
+           if (cache.amountToAdd > amountToAddActual) {
+                extraToken1Owed += uint128(cache.amountToAdd - amountToAddActual);
+            }
+
+            if (cache.amountFromAdd > amountFromAddActual) {
+                extraToken0Owed += uint128(cache.amountFromAdd - amountFromAddActual);
+            }
+            cache.amountToAdd = amountToAddActual;
+            cache.amountFromAdd = amountFromAddActual; 
        }

        // obtain the position's latest FeeGrowthInside after increaseLiquidity
        (, , , , , , , , cache.feeGrowthInside0LastX128, cache.feeGrowthInside1LastX128, , ) = Base
            .UNI_POSITION_MANAGER
            .positions(lien.tokenId);

        // caculate the amounts owed since last fee collection during the borrowing period
        (cache.token0Owed, cache.token1Owed) = Base.getOwedFee(
            cache.feeGrowthInside0LastX128,
            cache.feeGrowthInside1LastX128,
            lien.feeGrowthInside0LastX128,
            lien.feeGrowthInside1LastX128,
            lien.liquidity
        );

        // calculate the the amounts owed to LP up to the premium in the lien
        // must ensure enough amount is left to pay for interest first, then send gains and fund left to borrower
        ///@dev refundWithCheck ensures actual cannot be more than expected, since amount owed to LP is in actual,
        ///     it ensures (1) on the collateralFrom part of refund, tokenOwed is covered, and (2) on the amountReceived
        ///      part, received is no less than liquidity addback + token owed.
        if (lien.zeroForOne) {
            cache.token0Owed = cache.token0Owed < cache.tokenToPremium ? cache.token0Owed : cache.tokenToPremium;
            cache.token1Owed = cache.token1Owed < cache.tokenFromPremium ? cache.token1Owed : cache.tokenFromPremium;
            Base.refundWithCheck(
                borrower,
                cache.tokenFrom,
                cache.collateralFrom + cache.tokenFromPremium,
                cache.amountSpent + cache.amountFromAdd + cache.token1Owed
            );
            Base.refundWithCheck(
                borrower,
                cache.tokenTo,
                cache.amountReceived + cache.tokenToPremium,
                cache.amountToAdd + cache.token0Owed
            );
        } else {
            cache.token0Owed = cache.token0Owed < cache.tokenFromPremium ? cache.token0Owed : cache.tokenFromPremium;
            cache.token1Owed = cache.token1Owed < cache.tokenToPremium ? cache.token1Owed : cache.tokenToPremium;
            Base.refundWithCheck(
                borrower,
                cache.tokenFrom,
                cache.collateralFrom + cache.tokenFromPremium,
                cache.amountSpent + cache.amountFromAdd + cache.token0Owed
            );
            Base.refundWithCheck(
                borrower,
                cache.tokenTo,
                cache.amountReceived + cache.tokenToPremium,
                cache.amountToAdd + cache.token1Owed
            );
        }
+        cache.token0Owed += extraToken0Owed;
+        cache.token1Owed += extraToken1Owed;
        // pay for interest
        lps.addTokensOwed(lien.tokenId, cache.token0Owed, cache.token1Owed);
    }

Assessed type

Uniswap

c4-judge commented 9 months ago

0xleastwood marked the issue as primary issue

romeroadrian commented 9 months ago

Aren't these leftovers refunded to the borrower during refundWithCheck()?

wukong-particle commented 9 months ago

I agree with @romeroadrian, these amounts should be gracefully handled by refundWithCheck and will be sent to the borrower. The LPs should only be entitled cache.amountFromAdd and cache.amountToAdd (the actual amount, overwritten at the liquidity increasing step), as it repays the borrowed liquidity amount exactly (experiencing no higher impermanent loss). As it stands, I tend to vote for invalidating this finding, but will let the judge to decide.

0xleastwood commented 9 months ago

It does seem that refundWithCheck would send back excess funds to the borrower. Will invalidate but let @said017 comment if they disagree with this and have a POC to show otherwise.