Fix for fees calculations

[c4-bot-7]

Lines of code

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

Vulnerability details

Description

If we executed the test function incr_position_fee_growth_tick as the team said in Contest description, it will revert with called Result::unwrap() on an Err value: [70, 101, 101, 32, 103, 114, 111, 119, 116, 104, 32, 115, 117, 98, 32, 111, 118, 101, 114, 102, 108, 111, 119, 32, 116, 105, 99, 107] when we decode this error it will give us the message Fee growth sub overflow tick.This means the calculation of the fee growth isnt working correctly, aftrer some digging i found that Uniswap V3 code is with solidity < 0.8.0 so that overflows can happen, this design of uni isnt wrong and the fees calculated are correct because of this overflow. If we look at the function responsible for calculating feees in the context of superposition get_fee_growth_inside in tick.rs all substraction operations in this function are used with checked_sub. The function checked_sub in rust performs the subtraction and also checks if an overflow or underflow occurs. If the subtraction would result in an overflow or underflow, checked_sub returns None instead of wrapping around like in the uniswap v3 code version. We can use instead wrapping_sub to mimic exactly how uniswap handles the fee calculation and see if it will solve the issue.

Proof of Concept

To test our POC below please copy the function get_fee_growth_inside I put in Recommended Mitigation Steps and paste it instead of the incorrect one in tick.rs.

You can add the test function below in pkg/seawater/tests/lib.rs

fn test_incr_position_fee_growth_tick_working() {

    test_utils::with_storage::<_, Pools, _>(
            Some(address!("feb6034fc7df27df18a3a6bad5fb94c0d3dcb6d5").into_array()), // sender
            Some(hashmap! {
                    "0x0000000000000000000000000000000000000000000000000000000000000000" => "0x000000000000000000000000feb6034fc7df27df18a3a6bad5fb94c0d3dcb6d5",
                    "0x2094fc11ba78df2b7ed9c7631680af7cf7bd4803bac5c7331fb2686e5c11e38e" => "0x00000000000000000000000000000000d3c21bcecceda10000003c00000bb801",
                    "0x2094fc11ba78df2b7ed9c7631680af7cf7bd4803bac5c7331fb2686e5c11e38f" => "0x000000000000000000000000000002946e618fc1c100eb2ece23c766dc7fe332",
                    "0x2094fc11ba78df2b7ed9c7631680af7cf7bd4803bac5c7331fb2686e5c11e390" => "0x00000000000000000000000000000000000003e430416229f385847e3438b6a0",
                    "0x2094fc11ba78df2b7ed9c7631680af7cf7bd4803bac5c7331fb2686e5c11e392" => "0x000000000000000000000000fffc729800000000000000000e000843c6b7e857",
                    "0x3c79da47f96b0f39664f73c0a1f350580be90742947dddfa21ba64d578dfe600" => "0x0000000000000000000000000000000000000000000000000000000000000000",
                    "0x441db1f4a15f63dd7988db6d518ae69b05a4bd5f528ae8589d82253ea85c9bcb" => "0x000000000000000000000000000000000000000000000000fffc7660fffc6e68",
                    "0x606b7cbac0ee9fcaadc6dc1a873e9053536063080567030e6f1bbeeecc7c5b99" => "0x0000000000000000000000000000000000000000000000000000000000000000",
                    "0x606b7cbac0ee9fcaadc6dc1a873e9053536063080567030e6f1bbeeecc7c5b9d" => "0x0000000000000000000000000000000000000000000000000000000000000000",
                    "0x81e9c7c70971b5eb969cec21a82e6deed42e7c6736e0e83ced66d72297d9f1d7" => "0x000000000000000000000000eb365e1d8113e2dc89eaffeb0eb8655de541e068",
                    "0x9082b893d81e13a22d3a20bb475d762420aa82b1b423048886c8649938325d80" => "0x0000000000000000000000000000000000000000000000000000000000000000",
                    "0xc26292c271d836cef11934ee150114f8ac724da089d6e3a3a515a1943495adf9" => "0x0000000000000000000258aa211fc79e0000000000000000000258aa211fc79e",
                    "0xc26292c271d836cef11934ee150114f8ac724da089d6e3a3a515a1943495adfa" => "0x000000000000000000000000000002340f77a54003eed89b1a97894b15c82a7c",
                    "0xc26292c271d836cef11934ee150114f8ac724da089d6e3a3a515a1943495adfb" => "0x00000000000000000000000000000000000003ccff159d3a26244013f0917ad0",
                    "0x09082b893d81e13a22d3a20bb475d762420aa82b1b423048886c8649938325d8" => "0x000000000000000000000000feb6034fc7df27df18a3a6bad5fb94c0d3dcb6d5",
            }),
            None,
            None,
            |contract| {
                let token = address!("22b9fa698b68bba071b513959794e9a47d19214c");
                let fee_global_0 = contract.fee_growth_global_0_38_B5665_B(token);
                let fee_global_1 = contract.fee_growth_global_1_A_33_A_5_A_1_B(token);
                let starting_fee = contract.fee_B_B_3_C_F_608(token);

                match (starting_fee, fee_global_0, fee_global_1) {
                    (Ok(fee), Ok(fee_0), Ok(fee_1)) => {
                        eprintln!(
                            "starting fee: {}, token: {}, fee global 0: {}, fee global 1: {}",
                            fee, token, fee_0, fee_1
                        );
                    }
                    (Err(e), _, _) | (_, Err(e), _) | (_, _, Err(e)) => {
                        eprintln!("Error fetching fees: {:?}", e);
                    }
                }

            let id = U256::from(22);
            contract.mint_position_B_C5_B086_D(token, -887272, 887272);
             contract.incr_position_C_3_A_C_7_C_A_A(
                token,
                id,
                U256::zero(),
                U256::zero(),
                U256::from(100_000),
                U256::from(100_000),
            );
            }
        );
}

Run the test with

cargo test --features testing --package seawater -- test_incr_position_fee_growth_tick_working --nocapture

As you can see the test passes and we will see the fees in the logs.

starting fee: 3000, token: 0x22b9fa698b68bBA071B513959794E9a47d19214c, fee global 0: 224733083815886344915083562155014453453618, fee global 1: 78926184188422759162812938368672

NOTE: The test function incr_position_fee_growth_tick is now reverting with "Sqrt price is 0" and not "Fee growth sub overflow tick" i am not sure why it is reverting with price is 0 but the test function i gave is working correctly.

Impact

Incorrect calculations of postion fees.

Tools Used

Manual Analysis and also this report from late 2023 about the same issue happening in solidity > 0.8.0

Recommended Mitigation Steps

One way to solve this is to use the function wrapping_sub instead of checked_sub.

    pub fn get_fee_growth_inside(
        &mut self,
        lower_tick: i32,
        upper_tick: i32,
        cur_tick: i32,
        fee_growth_global_0: &U256,
        fee_growth_global_1: &U256,
    ) -> Result<(U256, U256), Error> {
        // the fee growth inside this tick is the total fee
        // growth, minus the fee growth outside this tick
        let lower = self.ticks.get(lower_tick);
        let upper = self.ticks.get(upper_tick);

        let (fee_growth_below_0, fee_growth_below_1) = if cur_tick >= lower_tick {
            #[cfg(feature = "testing-dbg")]
            dbg!((
                "cur_tick >= lower_tick",
                current_test!(),
                lower.fee_growth_outside_0.get().to_string(),
                lower.fee_growth_outside_1.get().to_string()
            ));

            (
                lower.fee_growth_outside_0.get(),
                lower.fee_growth_outside_1.get(),
            )
        } else {
            #[cfg(feature = "testing-dbg")]
            dbg!((
                "cur_tick < lower_tick",
                current_test!(),
                fee_growth_global_0,
                fee_growth_global_1,
                lower.fee_growth_outside_0.get().to_string(),
                lower.fee_growth_outside_1.get().to_string()
            ));

            (
                // Change: Replace checked_sub with wrapping_sub
                fee_growth_global_0
                    .wrapping_sub(lower.fee_growth_outside_0.get()),

                // Change: Replace checked_sub with wrapping_sub
                fee_growth_global_1
                    .wrapping_sub(lower.fee_growth_outside_1.get()),
            )
        };

        let (fee_growth_above_0, fee_growth_above_1) = if cur_tick < upper_tick {
            #[cfg(feature = "testing-dbg")]
            dbg!((
                "cur_tick < upper_tick",
                current_test!(),
                upper.fee_growth_outside_0.get().to_string(),
                upper.fee_growth_outside_1.get().to_string()
            ));

            (
                upper.fee_growth_outside_0.get(),
                upper.fee_growth_outside_1.get(),
            )
        } else {
            #[cfg(feature = "testing-dbg")]
            dbg!((
                "cur_tick >= upper_tick",
                current_test!(),
                fee_growth_global_0,
                fee_growth_global_1,
                upper.fee_growth_outside_0.get(),
                upper.fee_growth_outside_1.get()
            ));

            (
                // Change: Replace checked_sub with wrapping_sub
                fee_growth_global_0
                    .wrapping_sub(upper.fee_growth_outside_0.get()),

                // Change: Replace checked_sub with wrapping_sub
                fee_growth_global_1
                    .wrapping_sub(upper.fee_growth_outside_1.get()),
            )
        };

        #[cfg(feature = "testing-dbg")] // REMOVEME
        {
            if *fee_growth_global_0 < fee_growth_below_0 {
                dbg!((
                    "fee_growth_global_0 < fee_growth_below_0",
                    current_test!(),
                    fee_growth_global_0.to_string(),
                    fee_growth_below_0.to_string()
                ));
            }
            let fee_growth_global_0 = fee_growth_global_0.wrapping_sub(fee_growth_below_0);  // Change: Replace checked_sub with wrapping_sub
            if fee_growth_global_0 < fee_growth_above_0 {
                dbg!((
                    "fee_growth_global_0 < fee_growth_above_0",
                    current_test!(),
                    fee_growth_global_0.to_string(),
                    fee_growth_above_0.to_string()
                ));
            }
        }

        #[cfg(feature = "testing-dbg")]
        dbg!((
            "final stage wrapping sub below",
            current_test!(),
            fee_growth_global_0
                .wrapping_sub(fee_growth_below_0)
                .and_then(|x| x.wrapping_sub(fee_growth_above_0))
        ));

        Ok((
            // Change: Replace checked_sub with wrapping_sub
            fee_growth_global_0
                .wrapping_sub(fee_growth_below_0)
                .wrapping_sub(fee_growth_above_0),

            // Change: Replace checked_sub with wrapping_sub
            fee_growth_global_1
                .wrapping_sub(fee_growth_below_1)
                .wrapping_sub(fee_growth_above_1),
        ))
    }

Assessed type

Uniswap