decr_position_09293696() is incorrectly implemented in the current version of the protocol

[c4-bot-9]

Lines of code

https://github.com/code-423n4/2024-08-superposition/blob/main/pkg/seawater/src/lib.rs#L938

Vulnerability details

Impact

In the current implementation of the "update_positions" feature, the function decr_position_09293696() that decreases the liquidity is flawed. The problem is that it the function uses the same functionality for increasing / decreasing position that is not correct. Moreover, the function does not check if the provided liquidity is no greater than the current position liquidity.

Proof of Concept

Currently both functions for increasing / decreasing liquidity call the same function - adjust_position_internal() where we calculate the liquidity and update the position. The problem is that the functions should be mirrored - users should provide the amount of liquidity and get the calculated corresponding amounts in return:

https://github.com/code-423n4/2024-08-superposition/blob/main/pkg/seawater/src/lib.rs#L938-958

  pub fn decr_position_09293696(
        &mut self,
        pool: Address,
        id: U256,
        amount_0_min: U256,
        amount_1_min: U256,
        amount_0_max: U256,
        amount_1_max: U256,
    ) -> Result<(U256, U256), Revert> {
        self.adjust_position_internal(
            pool,
            id,
            amount_0_min,
            amount_1_min,
            amount_0_max,
            amount_1_max,
            true,
            None,
        )
    }
}

Now take a look at the Uniswap implementation:

https://github.com/Uniswap/v3-periphery/blob/main/contracts/NonfungiblePositionManager.sol#L257-264

function decreaseLiquidity(DecreaseLiquidityParams calldata params)
        external
        payable
        override
        isAuthorizedForToken(params.tokenId)
        checkDeadline(params.deadline)
        returns (uint256 amount0, uint256 amount1)
    {

There are currently no any checks that check for the requested liquidity to be greater or equal than the liquidity inside of the position as it's done in UniswapV3:

https://github.com/Uniswap/v3-periphery/blob/main/contracts/NonfungiblePositionManager.sol#L268-269

        uint128 positionLiquidity = position.liquidity;
        require(positionLiquidity >= params.liquidity);

Now, as we compare the implementation with the impl of UniswapV3, let's take a look at how amounts are calculated:

https://github.com/Uniswap/v3-periphery/blob/main/contracts/NonfungiblePositionManager.sol#L273

  (amount0, amount1) = pool.burn(position.tickLower, position.tickUpper, params.liquidity);

But in the protocol everything is done the same way as for liquidity increase operation:

https://github.com/code-423n4/2024-08-superposition/blob/main/pkg/seawater/src/pool.rs#L265-277

     let sqrt_ratio_x_96 = tick_math::get_sqrt_ratio_at_tick(self.get_cur_tick().as_i32())?;
        let sqrt_ratio_a_x_96 = tick_math::get_sqrt_ratio_at_tick(position.lower.get().as_i32())?;
        let sqrt_ratio_b_x_96 = tick_math::get_sqrt_ratio_at_tick(position.upper.get().as_i32())?;

        let mut delta = sqrt_price_math::get_liquidity_for_amounts(
            sqrt_ratio_x_96,   // cur_tick
            sqrt_ratio_a_x_96, // lower_tick
            sqrt_ratio_b_x_96, // upper_tick
            amount_0,          // amount_0
            amount_1,          // amount_1
        )?
        .to_i128()
        .map_or_else(|| Err(Error::LiquidityAmountTooWide), Ok)?;

So it's the same algorithm as with increasing liquidity on UniswapV3:

https://github.com/Uniswap/v3-periphery/blob/main/contracts/base/LiquidityManagement.sol#L51-78

function addLiquidity(AddLiquidityParams memory params)
        internal
        returns (
            uint128 liquidity,
            uint256 amount0,
            uint256 amount1,
            IUniswapV3Pool pool
        )
    {
        PoolAddress.PoolKey memory poolKey =
            PoolAddress.PoolKey({token0: params.token0, token1: params.token1, fee: params.fee});

        pool = IUniswapV3Pool(PoolAddress.computeAddress(factory, poolKey));

        // compute the liquidity amount
        {
            (uint160 sqrtPriceX96, , , , , , ) = pool.slot0();
            uint160 sqrtRatioAX96 = TickMath.getSqrtRatioAtTick(params.tickLower);
            uint160 sqrtRatioBX96 = TickMath.getSqrtRatioAtTick(params.tickUpper);

            liquidity = LiquidityAmounts.getLiquidityForAmounts(
                sqrtPriceX96,
                sqrtRatioAX96,
                sqrtRatioBX96,
                params.amount0Desired,
                params.amount1Desired
            );
        }

https://github.com/Uniswap/v3-periphery/blob/main/contracts/NonfungiblePositionManager.sol#L273

  (amount0, amount1) = pool.burn(position.tickLower, position.tickUpper, params.liquidity);

https://github.com/Uniswap/v3-core/blob/main/contracts/UniswapV3Pool.sol#L522-530

 _modifyPosition(
                ModifyPositionParams({
                    owner: msg.sender,
                    tickLower: tickLower,
                    tickUpper: tickUpper,
                    liquidityDelta: -int256(amount).toInt128()
                })
            );

As you can see here, the liquidityDelta in UniV3 is not calculated via get_liquidity_for_amounts() but with amount supplied by the user.

Tools Used

Manual review.

Recommended Mitigation Steps

Consider implementing decrease liquidity functionality correctly including check for the liquidity requested to be greater or equal to the current position liquidity to prevent users from getting more liquidity than they should.

Assessed type

Other