Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Usage of slot0's information is prone to manipulation attack #12

Open
c4-bot-4 opened this issue Dec 18, 2023 · 4 comments
Open

Usage of slot0's information is prone to manipulation attack #12

c4-bot-4 opened this issue Dec 18, 2023 · 4 comments
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue duplicate-23 grade-a Q-05 QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax

Comments

@c4-bot-4
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2023-12-particle/blob/main/contracts/libraries/Base.sol#L183
https://github.com/code-423n4/2023-12-particle/blob/main/contracts/libraries/Base.sol#L326

Vulnerability details

Impact

slot0 is the most recent data point that can be manipulated trough swap and flash loan. Operations that rely in this data are susceptible to price manipulation attack.

Proof of Concept

It can be observed that slot0 price is used when calculating getRequiredRepay, this will calculate amount need to be repaid when closing or liquidating traders position.

https://github.com/code-423n4/2023-12-particle/blob/main/contracts/libraries/Base.sol#L163-L192

   function getRequiredRepay(
        uint128 liquidity,
        uint256 tokenId
    ) internal view returns (uint256 amount0, uint256 amount1) {
        DataCache.RepayCache memory repayCache;
        (
            ,
            ,
            repayCache.token0,
            repayCache.token1,
            repayCache.fee,
            repayCache.tickLower,
            repayCache.tickUpper,
            ,
            ,
            ,
            ,

        ) = UNI_POSITION_MANAGER.positions(tokenId);
        IUniswapV3Pool pool = IUniswapV3Pool(UNI_FACTORY.getPool(repayCache.token0, repayCache.token1, repayCache.fee));
>>>     (repayCache.sqrtRatioX96, , , , , , ) = pool.slot0();
        repayCache.sqrtRatioAX96 = TickMath.getSqrtRatioAtTick(repayCache.tickLower);
        repayCache.sqrtRatioBX96 = TickMath.getSqrtRatioAtTick(repayCache.tickUpper);
        (amount0, amount1) = LiquidityAmounts.getAmountsForLiquidity(
            repayCache.sqrtRatioX96,
            repayCache.sqrtRatioAX96,
            repayCache.sqrtRatioBX96,
            liquidity
        );
    }

Traders can leverage flash loan to sandwich the operation so it become profitable for them when calculating the amount that need to be provided back to the LPs liquidity position.

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

Tools Used

Manual review.

Recommended Mitigation Steps

Use TWAP for the price inside the operation.

Assessed type

Uniswap

@c4-bot-4 c4-bot-4 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Dec 18, 2023
c4-bot-8 added a commit that referenced this issue Dec 18, 2023
@c4-judge
Copy link
Contributor

0xleastwood marked the issue as duplicate of #23

@romeroadrian
Copy link

The LP owner is interesting in getting the liquidity back, the underlying amounts of each token are meaningless to this operation. The attacker only looses flash loan and trading fees.

@c4-judge c4-judge added downgraded by judge Judge downgraded the risk level of this issue QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax and removed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels Dec 23, 2023
@c4-judge
Copy link
Contributor

0xleastwood changed the severity to QA (Quality Assurance)

@c4-judge
Copy link
Contributor

0xleastwood marked the issue as grade-a

@C4-Staff C4-Staff reopened this Jan 3, 2024
@C4-Staff C4-Staff added the Q-05 label Jan 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue duplicate-23 grade-a Q-05 QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax
Projects
None yet
Development

No branches or pull requests

4 participants