search

code-423n4 / 2024-02-hydradx-findings

1 stars 0 forks source link

Missing slippage checks on Omnipool.add_liquidity() #55

Closed c4-bot-10 closed 7 months ago

c4-bot-10 commented 8 months ago

Lines of code

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

Vulnerability details

Bug Description

HydraDx Protocol's Automated Market Maker (AMM), known as the Omnipool, enables user transactions like adding liquidity. To combat potential front-running of add_liquidity() calls, the protocol uses the T::PriceBarrier::ensure_price() mechanism. This function checks if the current price, which may have been manipulated by front-running, deviates from the oracle's average price by no more than 1%, a limit set in the runtime configuration. 

fn ensure_price(who: &AccountId, asset_a: AssetId, asset_b: AssetId, current_price: EmaPrice) -> Result<(), ()> {
    if WhitelistedAccounts::contains(who) {
        return Ok(());
    }

    let max_allowed = FixedU128::from(MaxAllowed::get());

    let oracle_price = ExternalOracle::get_price(asset_a, asset_b).map_err(|_| ())?;
    let external_price = FixedU128::checked_from_rational(oracle_price.n, oracle_price.d).ok_or(())?;
    let current_spot_price = FixedU128::checked_from_rational(current_price.n, current_price.d).ok_or(())?;

    let max_allowed_difference = max_allowed
        .checked_mul(&current_spot_price.checked_add(&external_price).ok_or(())?)
        .ok_or(())?;

    let diff = if current_spot_price >= external_price {
        current_spot_price.saturating_sub(external_price)
    } else {
        external_price.saturating_sub(current_spot_price)
    };

    ensure!(
        diff.checked_mul(&FixedU128::from(2)).ok_or(())? <= max_allowed_difference,
        ()
    );
    Ok(())
}

However, this safeguard only protects against price manipulations that exceed a 1% deviation, leaving a gap for smaller front-running attacks to occur without detection.

Impact

This vulnerability allows adversaries to execute front-running attacks on add_liquidity() transactions that result in less than a 1% loss for the targeted users, thereby exposing them to repeated minor financial losses that are not mitigated by the current protocol defenses.

Proof of Concept

The proof of concept demonstrates a scenario where a transaction to add_liquidity() is successfully front-run, leading to a slight price change within the 1% tolerance. The protocol fails to prevent the completion of the transaction, showcasing its susceptibility to such front-running strategies.

#[test]
fn add_liquidity_should_pass_when_frontrun_below() {
    ExtBuilder::default()
        .with_endowed_accounts(vec![
            (Omnipool::protocol_account(), DAI, 1000 * ONE),
            (Omnipool::protocol_account(), HDX, NATIVE_AMOUNT),
            (LP2, 1_000, 2000 * ONE),
            (LP1, 1_000, 5000 * ONE),
        ])
        .with_initial_pool(FixedU128::from_float(0.5), FixedU128::from(1))
        .with_token(1_000, FixedU128::from_float(0.65), LP2, 2000 * ONE)
        .with_max_allowed_price_difference(Permill::from_percent(1))
        .build()
        .execute_with(|| {
            let current_position_id = <NextPositionId<Test>>::get();
            assert_ok!(Omnipool::add_liquidity(RuntimeOrigin::signed(LP1), 1_000, 400 * ONE));

            //Minor adjustment of 99/100 
            EXT_PRICE_ADJUSTMENT.with(|v| {
                *v.borrow_mut() = (99, 100, true);
            });

            assert_noop!(
                Omnipool::add_liquidity(RuntimeOrigin::signed(LP1), current_position_id, 200 * ONE,),
                Error::<Test>::PriceDifferenceTooHigh
            );
        });
}

To run the test it needs to be added to the omnipool/src/tests/remove_liquidity.rs file.

Tools Used

Manual Review

Recommended Mitigation Steps

A practical solution involves allowing users to set their own slippage tolerance level. By adopting a customizable slippage parameter, similar to the approach used in StableSwap's liquidity withdrawal functions, users can better control their acceptable loss margins, potentially avoiding the default up to 1% loss currently in place.

Assessed type

MEV

c4-pre-sort commented 7 months ago

0xRobocop marked the issue as duplicate of #158

c4-judge commented 7 months ago

OpenCoreCH marked the issue as satisfactory

c4-judge commented 7 months ago

OpenCoreCH changed the severity to 2 (Med Risk)

J4X-98 commented 7 months ago

Hi @OpenCoreCH,

thank you very much for reviewing this contest. I would like to add that the primary issue of this only describes a part of the actual problem. In the protocol, there are 3 functions that are missing slippage control.

The current primary issue only describes the missing slippage parameters for Omnipoool.remove_liquidity() but not the other 2 problems. In the past, the C4 judging on similar situations was to split multiple slippage problems in one protocol into separate issues. Examples of this are:

Vader Protocol:

BadgerDAO

Asymmetry Finance:

In the C4 judging documentation duplicates are defined as follows:

Neither of those is true for the 3 issues described above. They all result out of different parts of the code, and adding slippage protection to one of the functions will not fix the other issues.

Additionally, I want to add that #177 is actually a duplicate of #84 and not of #93

OpenCoreCH commented 7 months ago

I would argue that they conceptually have the same root cause (missing slippage check) and are resolved by the same solution. Of course, this would affect different lines of code / functions, but if we make this the single criteria for duplicates, it would imply that a codebase with 20 mint functions could have 20 different slippage issues, which seems not ideal.

J4X-98 commented 7 months ago

Hey @OpenCoreCH,

I understand and agree with you. Nevertheless I would kindly ask you to reconsider why a warden/issue that only found 1/3 problems in the codebase is chosen as the primary issue and also why wardens that only found 1 or 2 out of the 3 issues are awarded fully and not partially.

I think it makes more sense to make me (or another warden) that found all of the issues as the prime issue and also only partially award wardens that did not find all issues.

OpenCoreCH commented 7 months ago

At first, another issue (#158) that mentions all three functions was chosen as the primary one. While the current only mentions one function, I still think that it provides the most value because it analyzes the issue in detail with a numerical example and directly highlights the potential impact.

carrotsmuggler commented 7 months ago

Hi @J4X-98 .Since you are suggesting rewarding a warden who found all the issues as the primary report, i would like to point out I did find all the issues: #93, #85, #92. They are all duped with #93 now.

J4X-98 commented 7 months ago

Hey @carrotsmuggler,

did not see that. Congrats on also finding all issues. Consider my comment above void.