c4-bot-1 commented 8 months ago

Lines of code

https://github.com/code-423n4/2023-12-autonolas/blob/2a095eb1f8359be349d23af67089795fb0be4ed1/lockbox-solana/solidity/liquidity_lockbox.sol#L277

Vulnerability details

Impact

Vanilla example of missing slippage protection.

Proof of Concept

As any AMM, Orca implements slippage protections by letting the caller specify the minimum amount of input and output tokens to be used in a given operation, so that if the transaction outputs a value less than the expected ones, it will revert preventing users from incurring in unacceptable losses.

In our situation, we are interested in the decreaseLiquidity handler:

decrease_liquidity, lines 18 and 19

pub fn handler(
    ctx: Context<ModifyLiquidity>,
    liquidity_amount: u128,
    token_min_a: u64,
    token_min_b: u64,
) -> Result<()> {

    ...

    let (delta_a, delta_b) = calculate_liquidity_token_deltas(
        ctx.accounts.whirlpool.tick_current_index,
        ctx.accounts.whirlpool.sqrt_price,
        &ctx.accounts.position,
        liquidity_delta,
    )?;

    if delta_a < token_min_a {
        return Err(ErrorCode::TokenMinSubceeded.into());
    } else if delta_b < token_min_b {
        return Err(ErrorCode::TokenMinSubceeded.into());
    }

    ...
}

The code snippet above calculates the delta variations of the two tokens amount in the given pool and reverts the whole transaction if the calculated amounts are less than the specified ones. However, if we go to

liquidity_lockbox, function withdraw

        ...

        whirlpool.decreaseLiquidity{accounts: metasDecreaseLiquidity, seeds: [[pdaProgramSeed, pdaBump]]}(amount, 0, 0);

        ...

we see that token_min_a and token_min_b are hard-coded to $0$, meaning that the liquidity decrease operation will accept ANY amount in exchange, even $0$, leading to a loss of funds as MEV exists in Solana too (see here). The mathematical reason is that

$$x >= 0, \forall x \in [0, 2⁶⁴-1]$$

so the errors

    if delta_a < token_min_a {
        return Err(ErrorCode::TokenMinSubceeded.into());
    } else if delta_b < token_min_b {
        return Err(ErrorCode::TokenMinSubceeded.into());
    }

won't be triggered as

    if delta_a < token_min_a { // @audit delta_a < 0 => false for all delta_a, so no error
        return Err(ErrorCode::TokenMinSubceeded.into());
    } else if delta_b < token_min_b { // @audit delta_b < 0 => false for al delta_b, so no error
        return Err(ErrorCode::TokenMinSubceeded.into());
    }

    // @audit if token_min_a = token_min_b = 0, then delta_a and delta_b can be ANY value, 
    // @audit even 0, which means total loss of funds is possible

meaning there is no slippage protection at all if called with token_min_a = token_min_b = 0 (as it's the case with liquidity_lockbock::withdraw).

Recommended Mitigation Steps

Let users provide the minimum amount of tokens they are willing to use as function arguments and pass them straight to decreaseLiquidity like:

-   function withdraw(uint64 amount) external {
+   function withdraw(uint64 amount, uint64 minA, uint64 minB) external {

        ...

-       whirlpool.decreaseLiquidity{accounts: metasDecreaseLiquidity, seeds: [[pdaProgramSeed, pdaBump]]}(amount, 0, 0);
+       whirlpool.decreaseLiquidity{accounts: metasDecreaseLiquidity, seeds: [[pdaProgramSeed, pdaBump]]}(amount, minA, minB);

        ...
}

Assessed type

Other

c4-pre-sort commented 8 months ago

alex-ppg marked the issue as primary issue

c4-pre-sort commented 8 months ago

alex-ppg marked the issue as sufficient quality report

c4-sponsor commented 8 months ago

mariapiamo (sponsor) confirmed

c4-sponsor commented 8 months ago

kupermind marked the issue as disagree with severity

c4-judge commented 7 months ago

dmvt changed the severity to 2 (Med Risk)