Spline

Liquidity Provider

6.1 Initial locked liquidity can be withdrawn by anyone which causes permanent DoS for the pool

When a new pool is created, initial liquidity is provided. The corresponding LP shares representing this initial liquidity are minted to the LP contract itself and are intended to be locked indefinitely. This is shown in the create_and_initialize_pool(...) function:

fn create_and_initialize_pool(...) {
    // ...
    let shares = initial_liquidity_factor.try_into().unwrap();
    call_core_with_callback::<
        (PoolKey, i129, i129, u256, ContractAddress), (),
    >(core, @(pool_key, liquidity_factor_delta, Zero::<i129>::zero(), shares, caller));

    // lock initial minted lp tokens forever in this contract
    let pool_token = self.pool_tokens.read(pool_key);
    // @audit-issue Initial LP tokens, representing the first liquidity, are minted to this contract.
    ILiquidityProviderTokenDispatcher { contract_address: pool_token }
        .mint(get_contract_address(), shares);
}

This initial locked liquidity ensures that the pool’s total_factor (total liquidity factor) is non-zero. The total_factor is crucial for share calculations, as demonstrated in the calculate_shares(...) function, which asserts total_factor > 0:

fn calculate_shares(...) -> u256 {
    // @audit-issue This assertion requires total_factor to be positive. If it becomes '0', subsequent liquidity additions will fail.
    assert(total_factor > 0, 'Total factor is 0');
    let denom: u256 = total_factor.try_into().unwrap();
    let num: u256 = factor.try_into().unwrap();
    let shares: u256 = muldiv(total_shares, num, denom);
    shares
}

The problem arises because the LP contract embeds a SweepableComponent which exposes a sweep(...) function. This function allows anyone to withdraw any ERC20 tokens held by the LP contract.

#[embeddable_as(Sweepable)]
impl SweepableImpl<...> of super::ISweepable<ComponentState<TContractState>> {
    fn sweep(...) {
        let balance = IERC20Dispatcher { contract_address: token }
            .balance_of(get_contract_address());
        // @audit-issue The function checks for minimum balance but does not restrict which token can be swept or who can call it.
        assert(balance >= amount_min, 'Insufficient balance');
        IERC20Dispatcher { contract_address: token }.transfer(recipient, balance);
    }
}

Crucially, the sweep(...) function does not prevent the pool’s own LP tokens (those representing the initial locked liquidity) from being swept. An attacker can call sweep(...), specifying the pool’s LP token address as the token and their own address as the recipient. This allows them to take ownership of the initially locked LP shares.

Once the attacker possesses these LP shares, they can call remove_liquidity(...) to withdraw the actual underlying initial liquidity from the pool. This action reduces the pool’s liquidity_factor_delta and subsequently the total_factor to 0. As a result, the assert(total_factor > 0, ’Total factor is 0’) in calculate_shares(...) will fail for all future attempts to add liquidity, effectively causing a permanent Denial of Service (DoS) for the affected pool. All subsequent operations that depend on adding liquidity will become impossible.

The following Proof of Concept demonstrates the attack:

#[test]
#[fork("mainnet")]
#[should_panic(expected: ('Total factor is 0',))]
fn test_poc_foo() {
    let (pool_key, lp, _, _, default_profile_params, token0, token1) = setup_add_liquidity();
    let initial_liquidity_factor = lp.pool_liquidity_factor(pool_key);
    assert_eq!(initial_liquidity_factor, 1000000000000000000);

    // @audit Attacker (bob) identifies the pool's LP token and its balance held by the LP contract.
    let pool_token = IERC20Dispatcher { contract_address: lp.pool_token(pool_key) };
    let locked_shares = pool_token.balance_of(lp.contract_address);
    let lp_sweep = ISweepableDispatcher { contract_address: lp.contract_address };

    // @audit Attacker calls sweep(...) to transfer the locked LP shares to themselves.
    cheat_caller_address(lp.contract_address, 123.try_into().unwrap(), CheatSpan::TargetCalls(1));
    lp_sweep.sweep(pool_token.contract_address, 123.try_into().unwrap(), locked_shares);

    // @audit Attacker calls remove_liquidity(...) with the stolen LP shares, draining initial liquidity.
    cheat_caller_address(lp.contract_address, 123.try_into().unwrap(), CheatSpan::TargetCalls(1));
    lp.remove_liquidity(pool_key, locked_shares);

    // @audit A legitimate user attempts to add liquidity to the pool.
    let step = *default_profile_params[2];
    let n = *default_profile_params[3];
    let factor = 100000000000000000000; // 100 * 1e18
    let amount: u128 = (step.mag * n.mag * (factor)) / (1900000);
    token0.transfer(lp.contract_address, amount.into());
    token1.transfer(lp.contract_address, amount.into());

    // @audit-issue This call will now panic because total_factor is '0', causing a DoS.
    lp.add_liquidity(pool_key, factor);
}

Recommendation

Consider preventing the sweeping of the initial, locked LP shares specific to any pool managed by this contract.

Resolution

Fixed in e423418, 1993c7f and 7b390f5

Update from Nethermind Security

The safe_transfer_from(...) in shared_locker doesn’t handle the case for the tokens which returns false and doesn’t revert in the case of error.

Update from the client

Fixed in 29844dc

6.2 Lack of slippage protection in add_liquidity and remove_liquidity

The add_liquidity(...) function enables users to deposit assets into the underlying liquidity pool. In return, users receive pool shares proportional to their contributed liquidity relative to the total pool size.

fn add_liquidity(ref self: ContractState, pool_key: PoolKey, factor: u128) -> u256 {
    // compound fees if possible. also checks pool key and pool initialized
    self.compound_fees(pool_key);

    // calculate shares to mint
    let liquidity_factor_delta = i129 { mag: factor, sign: false };
    self.check_liquidity_factor_delta(pool_key, liquidity_factor_delta);

    let pool_token = self.pool_tokens.read(pool_key);
    let total_shares = IERC20Dispatcher { contract_address: pool_token }.total_supply();

    let liquidity_factor = self.pool_liquidity_factors.read(pool_key);
    let shares = self.calculate_shares(total_shares, factor, liquidity_factor);

    // add amount to liquidity factor in storage
    let new_liquidity_factor = liquidity_factor + factor;
    self.pool_liquidity_factors.write(pool_key, new_liquidity_factor);

    // obtain core lock. should also effectively lock this contract for unique pool key
    let core = self.core.read();
    let caller = get_caller_address();
    call_core_with_callback::<
        (PoolKey, i129, i129, u256, ContractAddress),
        ()
    >(
        core,
        @(
            pool_key,
            liquidity_factor_delta,
            Zero::<i129>::zero(),
            shares,
            caller
        )
    );

    // mint pool token shares to caller
    ILiquidityProviderTokenDispatcher { contract_address: pool_token }.mint(caller, shares);

    shares
}

The current implementation lacks any form of slippage protection. In the add_liquidity(...) function, the number of pool shares a user receives is determined by the factor they provide and the pool’s state (specifically total_shares and current liquidity_factor) at the time of execution via the self.calculate_shares(...) call. However, this state can change due to other transactions (e.g., deposits or withdrawals by other users) that are processed between the user submitting their transaction and its actual execution on-chain. This timing difference exposes the user to potential slippage: they might receive fewer pool shares than they expected for their deposited liquidity if the pool’s total liquidity or composition changed unfavorably in the interim.

A similar vulnerability exists in the remove_liquidity(...) function. When a user removes liquidity, the amount of underlying assets they receive is calculated based on the pool’s state at that moment. If other users’ transactions alter the pool composition or value just before the withdrawal is processed, the withdrawing user might receive fewer underlying assets than anticipated for their redeemed pool shares.

Recommendation

Consider allowing users to specify a minimum acceptable number of shares (for add_liquidity(...)) or a minimum amount of underlying assets (for remove_liquidity(...)) they are willing to receive. The functions should then revert if the calculated amount falls below this user-defined threshold due to market movements.

Resolution

Fixed in d3ff60e

6.3 Not enough validation on params for _set_grid_for_bounds(...)

When a pool is created and initialized, the _set_grid_for_bounds(...) is used to set the parameters for calculating bounds for a symmetric liquidity profile.

fn _set_grid_for_bounds(
    ref self: ComponentState<TContractState>, pool_key: PoolKey, params: Span<i129>,
) {
    assert(params.len() == 4, 'Invalid params length');
    assert(!*params[0].sign, 'Invalid grid s');
    assert(!*params[1].sign, 'Invalid grid resolution');

    let (s, res, tick_start, tick_max) = (
        *params[0].mag, *params[1].mag, *params[2], *params[3],
    );

    // @audit-issue It doesn't ensure res to be power of 2 rather it ensures res to be an even number.
    assert((res > 0 && (res % 2 == 0)), 'resolution must be power of 2');
    assert(tick_start < tick_max, 'tick_start must be < tick_max');
    assert(s > 0 && (s % pool_key.tick_spacing == 0), 's must divide by tick_spacing');
    // @audit-issue step is not 2*s/res rather step is s/res, so this check is incorrect.
    assert((2 * s) / res != 0, 'step must be non-zero');

    self.grid.write(pool_key, (s, res, tick_start, tick_max));
}

The _set_grid_for_bounds(...) function contains several asserts to ensure that the params are valid. However, some of these asserts are incorrect, and additional validations are missing.

Specifically, the following asserts are incorrect:

• To assert that "resolution must be power of 2", the current check res > 0 (res % 2 == 0) only ensures res is an even number greater than 0. A correct check for res being a power of 2 and greater than 0 would be res > 0 (res (res - 1)) == 0;
• To assert that "step must be non-zero", the current check uses (2 * s) / res != 0. The actual step in the calculation is s / res. Therefore, even if the current assert passes, the true step (s / res) could still be 0, leading to incorrect behavior;

Furthermore, the following crucial conditions are not asserted:

• The tick_start and tick_max values should be valid ticks, meaning they must be divisible by tick_spacing;
• The bounds calculated by get_bounds_for_liquidity_updates(...) should always fall within the valid tick range of [MIN_TICK, MAX_TICK], i.e., [-88722883, 88722883]. The current logic does not prevent a combination of s / res (step), tick_start, and tick_max from resulting in bounds outside this valid range;
• The s should be divisible by res else the segments won’t be divided into equally spaced bins resulting in the case where the check ticks.upper == next.upper can be missed in the get_bounds_for_liquidity_updates(...);
• The step, which is s / res, should also be divisible by tick_spacing to ensure that the calculated bounds align with valid tick positions. If it’s not, the bounds could point to invalid ticks;

Recommendation

Consider fixing the incorrect asserts and adding the missing asserts to ensure the params lead to a valid grid configuration.

Resolution

Fixed in 5796e4e

Update from Nethermind Security

There are 2 specific checks to ensure that tick_start and tick_max are within the range of [MIN_TICK, MAX_TICK]. These 2 checks aren’t enough. It should be ensured that all the ticks calculated in the get_bounds_for_liquidity_updates(...) by adding/subtracting with steps in the while loop are valid ticks.

Update from the client

Fixed in ae0ed13

Update from Nethermind Security

This will revert even for certain valid configurations. For example, if the configurations are as follows:

• TICK RANGE = [-460, 460];
• dt = 4;
• tick_start = -100;
• tick_max = 280;
• s = 32;
• res = 8;

Then, the last bound will be [-452, 256]. But tick_min will be -480, which is less than -460. Thus, it will revert even though the lower tick of the last bound, i.e., -452, is a valid tick.

Update from the client

This is fine since it wouldn’t revert when the pool is live, so basically just excludes certain configs.

6.4 Pool initialization initial_tick may mismatch profile’s tick_start

The create_and_initialize_pool(...) function is responsible for setting up a new liquidity pool. It accepts initial_tick and profile_params as parameters. The initial_tick is used to initialize the pool on the Ekubo core. The profile_params array is used to configure the liquidity profile associated with the pool, where profile_params[2] represents tick_start and profile_params[5] represents mu for the Cauchy distribution.

The cauchy::set_liquidity_profile(...) function, called during pool creation, correctly asserts that mu (profile_params[5]) is equal to tick_start (profile_params[2]).

fn set_liquidity_profile(ref self: ContractState, pool_key: PoolKey, params: Span<i129>) {
    // ...
    // @audit This asserts that mu (params[5]) equals tick_start (params[2]).
    assert(
        *params[5].mag == *params[2].mag && *params[5].sign == *params[2].sign,
        'mu != symmetric::tick_start',
    );
    // ...
}

However, there is no corresponding validation in lp::create_and_initialize_pool(...) to ensure that the initial_tick (passed as a direct argument) is equal to tick_start (i.e., profile_params[2]).

fn create_and_initialize_pool(
    ref self: ContractState,
    pool_key: PoolKey,
    initial_tick: i129,         // @audit Used to initialize the pool on core.
    profile_params: Span<i129>, // @audit profile_params[2] is tick_start.
) {
    // ...
    // @audit profile_params are set, which includes tick_start (profile_params[2]).
    profile.set_liquidity_profile(pool_key, profile_params);
    // ...
    // @audit Pool is initialized with initial_tick.
    // @audit-issue No check ensures initial_tick matches profile_params[2] (tick_start).
    core.initialize_pool(pool_key, initial_tick);

    // ...
}

This omission means that the pool can be initialized on the Ekubo core with an initial_tick that is different from the tick_start value defined in its liquidity profile parameters.

Recommendation

Consider adding a validation step within the lp::create_and_initialize_pool(...) function to ensure that the provided initial_tick argument is equal to profile_params[2] (the tick_start value from the profile parameters).

Resolution

The initial tick mismatch with tick start should be fine since it leaves flexibility to initialize pool not at center of liquidity distribution (although unlikely we’ll ever do that).

6.5 The try_call_core_with_callback(...) function reverts on deserialization failure, defeating its purpose of safely handling call errors

The try_call_core_with_callback(...) function is intended to make a call to an external core contract and handle potential errors without reverting the entire transaction. It uses a match statement on the call_result to differentiate between successful calls (Result::Ok) and failed calls (Result::Err). In the case of a failed call, it correctly returns Option::None.

However, if the external call to core.contract_address is successful (i.e., Result::Ok), the function then attempts to deserialize the output_span using Serde::deserialize(ref output_span).expect(’DESERIALIZE_RESULT_FAILED’). If this deserialization fails for any reason (e.g., unexpected data format returned by the core contract), the .expect(...) call will cause the entire transaction to revert with the message ’DESERIALIZE_RESULT_FAILED’. This behavior contradicts the function’s apparent goal of catching errors from the external call and its aftermath without causing a full revert.

The problem lies in this line:

pub fn try_call_core_with_callback<TInput, TOutput, +Serde<TInput>, +Serde<TOutput>>(
    core: ICoreDispatcher, input: @TInput,
) -> Option<TOutput> {
    let data = serialize(input).span();
    let mut calldata = serialize(@data).span();
    // TODO: is this valid for ekubo core with callback into lp? to bypass full tx reverts
    let call_result = call_contract_syscall(core.contract_address, selector!("lock"), calldata);
    match call_result {
        Result::Ok(mut output_span) => {
            // @audit-issue If Serde::deserialize fails, the .expect() call will revert the transaction.
            Option::Some(Serde::deserialize(ref output_span).expect('DESERIALIZE_RESULT_FAILED'))
        },
        Result::Err(_) => { Option::None(()) },
    }
}

This means that even if the call to the core contract itself does not revert, a subsequent failure in processing its return data can still lead to a revert, which might be an unintended behavior for a "try_call" type of function.

Recommendation

Consider modifying the deserialization step to handle potential errors gracefully.

Resolution

The try/call reverting on deserialization should be ok for spline purposes given more so intended usage is to catch any rounding issues on harvest fees. Deserialization errors on our end should revert as a bug.