Positions can be re-initialized

[c4-bot-5]

Lines of code

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

Vulnerability details

Description

In position.rs the function pub fn new is used the first time a position is created, users normally will call mint_position_B_C5_B086_D in lib.rs to handle the logic of position id so that we dont override an already initialized position.

In the current design, functions like new in position.rs and create_position in pool.rs are marked as pub fn, meaning they are publicly accessible and can be called by any external source they are also without any initialization checks . Unlike private functions, which are restricted to internal use, public functions lack this protection and are exposed to potential misuse. Because these public functions don't include initialization they allow anyone to directly interact with sensitive parts of the contract. This means that users can bypass the intended logic in lib.rs, where position creation is handled correctly through the mint_position_B_C5_B086_D function

Impact

Users can re-initialize positions using the create_position function directly from pool.rs

Proof of Concept

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

#[test]
fn test_pool_position_re_initalization() -> Result<(), Vec<u8>> {
    test_utils::with_storage::<_, StoragePool, _>(None, None, None, None, |pool| {
        let id = uint!(2_U256);
        let low = tick_math::get_tick_at_sqrt_ratio(test_utils::encode_sqrt_price(50, 1))?;
        let up = tick_math::get_tick_at_sqrt_ratio(test_utils::encode_sqrt_price(170, 1))?;
        pool.init(
            test_utils::encode_sqrt_price(100, 1), // price
            0,
            10,
            u128::MAX,
        )?;
        pool.enabled.set(true);
        pool.create_position(id, low - low % 10, up - up % 10)?;
        let position_saved = pool.positions.positions.get(id);
        println!(
            "Position saved: lower = {}, upper = {}", 
            position_saved.lower.get().as_i32(), 
            position_saved.upper.get().as_i32()
        );
        let different_low = low + 490; // Adding to ensure it's different
        let different_up = up + 850; // Adding to ensure it's different
        pool.create_position(id, different_low - different_low % 10, different_up - different_up % 10)?;
        let position_saved2 = pool.positions.positions.get(id);
        println!(
            "Position saved 2: lower = {}, upper = {}", 
            position_saved2.lower.get().as_i32(), 
            position_saved2.upper.get().as_i32()
        );
        Ok(())
    })
}

Run the test with

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

Tools Used

Manual Analysis

Recommended Mitigation Steps

One way to solve this is to explicitly check in the new fn in position.rs that low and high must be 0 the time we call this function otherwise this position is initialized.

    pub fn new(&mut self, id: U256, low: i32, up: i32) {
        // Check if the position has already been initialized
        let info = self.positions.get(id);
        assert!(info.lower.get() == I32::ZERO && info.upper.get() == I32::ZERO, "Position is already initialized.");
        let mut info = self.positions.setter(id);
        info.lower.set(I32::lib(&low));
        info.upper.set(I32::lib(&up));
    }

Assessed type

Invalid Validation