omnipool: LP can get zero asset when withdraw liquidity with one share

[c4-bot-5]

Lines of code

https://github.com/code-423n4/2024-02-hydradx/blob/main/HydraDX-node/math/src/omnipool/math.rs#L349

Vulnerability details

Impact

When LP calls the remove_liquidity() with one share to remove, he will get zero assets if the current asset is less than the position price, which is a loss for him.

Proof of Concept

In calculate_remove_liquidity_state_changes, when we calculate the protocol shares, we add one share on the result in favor of the protocol.

    // Protocol shares update
    let delta_b_hp = if current_price < position_price {
        let numer = p_x_r
            .checked_sub(current_hub_reserve_hp)?
            .checked_mul(shares_removed_hp)?;
        let denom = p_x_r.checked_add(current_hub_reserve_hp)?;
        numer.checked_div(denom)?.checked_add(U256::one())? // round up
    } else {
        U256::from(Balance::zero())
    };

This is fine for pools where the shares are sufficient or the asset price is not high. But for other cases where the asset price is high, the LP may try to withdraw only one share, in which case the asset withdrawn to the LP will become zero, and his share will be burned.

Basically, the LP burns one share and gets nothing.

PoC:

fn calculate_remove_liquidity_should_work_when_current_price_is_smaller_than_position_price() {
    let asset_state = AssetReserveState {
        reserve: 10 * UNIT,
        hub_reserve: 20 * UNIT,
        shares: 10 * UNIT,
        protocol_shares: 0u128,
    };

    let amount_to_remove = 1;

    let imbalance = I129 {
        value: UNIT,
        negative: true,
    };
    let total_hub_reserve = 22 * UNIT;

    let position = Position {
        amount: 3 * UNIT,
        shares: 3 * UNIT,
        price: (FixedU128::from_float(2.23).into_inner(), 1_000_000_000_000_000_000),
    };

    let state_changes = calculate_remove_liquidity_state_changes(
        &asset_state,
        amount_to_remove,
        &position,
        imbalance,
        total_hub_reserve,
        FixedU128::zero(),
    );

    assert!(state_changes.is_some());

    let state_changes = state_changes.unwrap();

    assert!(
        *state_changes.asset.delta_reserve > 0 // <-- fail
    );

    assert_eq!(state_changes.lp_hub_amount, 0u128);
}

Tools Used

Manual Review.

Recommended Mitigation Steps

In calculate_remove_liquidity_state_changes, we should make sure delta_reserve is not zero.

Assessed type

Context

[c4-pre-sort]

0xRobocop marked the issue as insufficient quality report

[OpenCoreCH]

Intended behaviour that is industry standard (rounding in favor of the protocol) with extremely negligible monetary impact.

[c4-judge]

OpenCoreCH marked the issue as nullified