An easily manipulated tick data is being used when minting the full range liquidity

[c4-bot-4]

Lines of code

https://github.com/code-423n4/2024-04-panoptic/blob/833312ebd600665b577fbd9c03ffa0daf250ed24/contracts/PanopticFactory.sol#L335-L412

Vulnerability details

Proof of Concept

Protocol in multiple instances uses data returned from uniswapV3 pool's slot0 as can be confirmed using this search command: https://github.com/search?q=repo%3Acode-423n4%2F2024-04-panoptic+slot0+NOT+language%3AMarkdown&type=code, now whereas it's right and logical for this to be used in some instances like say if we are to check the current solvency state or what not, using this in some instances is wrong, see reasoning below.

Take a look at https://github.com/code-423n4/2024-04-panoptic/blob/833312ebd600665b577fbd9c03ffa0daf250ed24/contracts/PanopticFactory.sol#L335-L412

    function _mintFullRange(
        IUniswapV3Pool v3Pool,
        address token0,
        address token1,
        uint24 fee
    ) internal returns (uint256, uint256) {
        //@audit 
        (uint160 currentSqrtPriceX96, , , , , , ) = v3Pool.slot0();

        // For full range: L = Δx * sqrt(P) = Δy / sqrt(P)
        // We start with fixed token amounts and apply this equation to calculate the liquidity
        // Note that for pools with a tickSpacing that is not a power of 2 or greater than 8 (887272 % ts != 0),
        // a position at the maximum and minimum allowable ticks will be wide, but not necessarily full-range.
        // In this case, the `fullRangeLiquidity` will always be an underestimate in respect to the token amounts required to mint.
        uint128 fullRangeLiquidity;
        unchecked {
            // Since we know one of the tokens is WETH, we simply add 0.1 ETH + worth in tokens
            if (token0 == WETH) {
                fullRangeLiquidity = uint128(
                    Math.mulDiv96RoundingUp(FULL_RANGE_LIQUIDITY_AMOUNT_WETH, currentSqrtPriceX96)
                );
            } else if (token1 == WETH) {
                fullRangeLiquidity = uint128(
                    Math.mulDivRoundingUp(
                        FULL_RANGE_LIQUIDITY_AMOUNT_WETH,
                        Constants.FP96,
                        currentSqrtPriceX96
                    )
                );
            } else {
                // Find the resulting liquidity for providing 1e6 of both tokens
                uint128 liquidity0 = uint128(
                    Math.mulDiv96RoundingUp(FULL_RANGE_LIQUIDITY_AMOUNT_TOKEN, currentSqrtPriceX96)
                );
                uint128 liquidity1 = uint128(
                    Math.mulDivRoundingUp(
                        FULL_RANGE_LIQUIDITY_AMOUNT_TOKEN,
                        Constants.FP96,
                        currentSqrtPriceX96
                    )
                );

                // Pick the greater of the liquidities - i.e the more "expensive" option
                // This ensures that the liquidity added is sufficiently large
                fullRangeLiquidity = liquidity0 > liquidity1 ? liquidity0 : liquidity1;
            }
        }

        // The maximum range we can mint is determined by the tickSpacing of the pool
        // The upper and lower ticks must be divisible by `tickSpacing`, so
        // tickSpacing = 1: tU/L = +/-887272
        // tickSpacing = 10: tU/L = +/-887270
        // tickSpacing = 60: tU/L = +/-887220
        // tickSpacing = 200: tU/L = +/-887200
        int24 tickLower;
        int24 tickUpper;
        unchecked {
            int24 tickSpacing = v3Pool.tickSpacing();
            tickLower = (Constants.MIN_V3POOL_TICK / tickSpacing) * tickSpacing;
            tickUpper = -tickLower;
        }

        bytes memory mintCallback = abi.encode(
            CallbackLib.CallbackData({
                poolFeatures: CallbackLib.PoolFeatures({token0: token0, token1: token1, fee: fee}),
                payer: msg.sender
            })
        );

        return
            IUniswapV3Pool(v3Pool).mint(
                address(this),
                tickLower,
                tickUpper,
                fullRangeLiquidity,
                mintCallback
            );
    }

This function is used to seed the Uniswap V3 pool with a full-tick-range liquidity deployment using funds from caller, however it uses the currentSqrtPriceX96 from slot0 to do this calculation.

Now Uniswap.slot0 is the most recent data point and can be manipulated easily via MEV bots and Flashloans with sandwich attacks; which can cause the loss of funds or leak of value to protocol when users are interacting with this function.

If the currentSqrtPriceX96 is higher, it means the price of the pool's tokens is higher. Consequently, the amount of liquidity minted will be lower because the same amount of assets (in terms of token0 and token1) will provide less liquidity at a higher price.

Conversely, if the currentSqrtPriceX96 is lower, it means the price of the pool's tokens is lower. In this case, the amount of liquidity minted will be higher because the same amount of assets will provide more liquidity at a lower price. This is because liquidity is more valuable when the price of the tokens is lower, as it covers a wider price range.

Impact

An easily manipulated tick data could be used when minting the positions since the value of currentSqrtPriceX96 directly influences the amount of liquidity that can be minted in the _mintFullRange function.

Recommended Mitigation Steps

Reimplement some core functionalities where slot0 is being implemented like shown in this report, cases where current data needs to be strictly used can have this implementation however if there is a valid case to use a twapTick then this should be used as it way more harder to manipulate the data returned from this.

Assessed type

Context

[c4-judge]

Picodes marked the issue as duplicate of #537

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes marked the issue as partial-50

[Picodes]

Giving partial credit because the impact and this part "If the currentSqrtPriceX96 is higher, it means the price of the pool's tokens is higher. Consequently, the amount of liquidity minted will be lower because the same amount of assets (in terms of token0 and token1) will provide less liquidity at a higher price." is unclear.

[Bauchibred]

Hi @Picodes, thanks for judging, however I believe this report explains the issue sufficiently and would like you to consider reassigning it's full duplicate status, cause without the current primary report, this report does a good job explaining the root cause of the issue, going section to section in both report we can see that:

The first part of the report, i.e impact section reads:

When deployNewPool is called it uses the spot price of the pool, which can be manipulated through a flashloan and thus could return a highly inaccurate result.
The price is used when deciding how much liquidity should be minted for each token, so this can result in an unbalanced pool.
In other parts of the code, this is not an issue as there are oracles that prevent price manipulations, but in case there aren't any checks to avoid so.

The first part of this report also hints how using the spot price is not problematic in other instances, but not when minting the full range liquidity, i.e

Protocol in multiple instances uses data returned from uniswapV3 pool's slot0 as can be confirmed using this search command: https://github.com/search?q=repo%3Acode-423n4%2F2024-04-panoptic+slot0+NOT+language%3AMarkdown&type=code, now whereas it's right and logical for this to be used in some instances like say if we are to check the current solvency state or what not, using this in some instances is wrong, see reasoning below.

Additionally, the impact section in this also grows on this fact:

An easily manipulated tick data could be used when minting the positions since the value of currentSqrtPriceX96 directly influences the amount of liquidity that can be minted in the _mintFullRange function.

Now the current primary report, asides the aforementioned impact section, the code snippet is the same as is attached in this report, the recommended mitigation step is also the same as is in this report, the only other statement is one under the POC section, which is also covered in this report, i.e

Under POC:

But unlike other parts of the code, the PanopticFactory doesn't have any checks against the price (it doesn't use any oracles nor the TWAP), so each token liquidity is manipulable through flash loans.

In this report:

This function is used to seed the Uniswap V3 pool with a full-tick-range liquidity deployment using funds from caller, however it uses the currentSqrtPriceX96 from slot0 to do this.
Now Uniswap.slot0 is the most recent data point and can be manipulated easily via MEV bots and Flashloans with sandwich attacks; which can cause the loss of funds or leak of value to protocol when users are interacting with this function.

Now the confusing section you've hinted is just a generic comment on issues arising from price manipulations, and I believe even without that section this report sufficiently hints where the issue is and how to fix it, considering these arguments, I believe the additional 30% for being selected for report on the primary is already a sufficient addition considering it's short and straight to the point, however I also believe a full duplication should be applied to this report, since tagging this as partial 50 means this report ends up being rewarded ~38% of what’s been rewarded to the primary _(50/ (100+30))_ , which imo is unfair as they both do a good job of explaining the core issue and suggesting the fix.

[Picodes]

@Bauchibred I still believe that the part I highlighted above - the part that is trying to explain what the issue would be in this report - is unclear, as there is no "higher price" or "lower price" but only a relative price between the 2 tokens, but do agree with you that you have correctly flagged the issue and this seems to be a wording mistake so I'll give back full credit.

[c4-judge]

Picodes marked the issue as full credit

[c4-judge]

Picodes marked the issue as satisfactory