c4-bot-6 commented 8 months ago

Lines of code

https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/omnipool/src/traits.rs#L164-L190

Vulnerability details

Impact

The ensure_price function in omnipool is a way to control slippage in the omnipool during liquidity addition and removal. The function takes in two assets, typically LRNA and the asset being added/removed, and the current spot price (reserves and hub reserves).

T::PriceBarrier::ensure_price(
        &who,
        T::HubAssetId::get(),
        asset,
        EmaPrice::new(asset_state.hub_reserve, asset_state.reserve),
    )

The function itself then compares the spot price (calculated from the reserves) and an oracle price, and makes sure the spot price is within certain bounds of the oracle price. If it is not, the function returns an error.

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(())?;
// diff is calculated as the absolute difference
ensure!(
    diff.checked_mul(&FixedU128::from(2)).ok_or(())? <= max_allowed_difference,
    ()
);

The issue is that this only deals with the price of the token with LRNA. Omnipools can have multiple tokens in them, not just the asset whose price is being checked here.

Since the only requirement is to match the tokenA - LRNA price (say tokenA is being deposited/removed here), users can swap tokenA to tokenB, and then sell LRNA for tokenA to skew the prices while keeping the LRNA-tokenA price the same. This is further discussed in the later section with step by step walkthrough of the math. Thus even though the protocol implements a 1% slippage through this function, the actual slippage can be forced to be much much higher.

Proof of Concept

The attack can be carried out in the following steps:

ALICE sells tokenA for tokenB.
ALICE sells LRNA for tokenA.
tokenA-LRNA price is shown to be the same as before ALICE's interactions.
BOB adds liquidity and loses out on liquidity shares. Since LRNA-tokenA price is the same, this addition does not revert.

For simplicity, lets assume that all tokens (tokenA, tokenB, LRNA) are equal and worth 1 usd.

Initially, the pool is assumed to have 20 tokenB, and 20 LRNA tokens corresponding to the 20 tokenB since they are all valued at the same price. Similarly the pool also has 20 tokenA and 20 LRNA tokens corresponding to the 20 tokenA.

In the below table, the assets of ALICE are tracked in columns 2-4, and the assets of the pool are tracked in columns 5-8. The pool's LRNA tokens are tracked in columns 6 and 7, and they contain the corresponding LRNA tokens for tokenA and tokenB respectively.

pA is the spot price of LRNA-tokenA, calculated by column_6/column_5. pB is the spot price of LRNA-tokenB, calculated by column_7/column_8. pAB is the spot price of tokenA-tokenB, calculated pB/pA.

The initial condition of the pool looks like so: Assuming there are 20 tokenB in the pool, and no tokenA.

Operation	ALICE tokenA	ALICE LRNA	ALICE tokenB	Pool tokenA	Pool LRNA for tokenA	Pool LRNA for tokenB	Pool tokenB	pA	pB	pAB
Initial	20	20	20	20	20	20	20	1	1	1

Now, ALICE swaps 5 tokenA for tokenB. The result of the swap is calculated according to the x*y=k invariant. ALICE gets back 4 tokenB from this swap.

Operation	ALICE tokenA	ALICE LRNA	ALICE tokenB	Pool tokenA	Pool LRNA for tokenA	Pool LRNA for tokenB	Pool tokenB	pA	pB	pAB
Initial	20	20	20	20	20	20	20	1	1	1
swap A->B	15	20	24	25	20	20	16	0.8	1.25	1.5625

The price pA has now dropped to 0.8. For ensure_price to pass, we want pA to be the same as the oracle price. So ALICE can now sell LRNA tokens to make pA 1 again.

For pA to be 1, tokenA = hub_tokenA, so tokenA_in_pool = sqrt(25*20)=22.36. So ALICE swaps 2.36 LRNA tokens and gets back 2.64 tokenA. The composition now looks like this:

Operation	ALICE tokenA	ALICE LRNA	ALICE tokenB	Pool tokenA	Pool LRNA for tokenA	Pool LRNA for tokenB	Pool tokenB	pA	pB	pAB
Initial	20	20	20	20	20	20	20	1	1	1
swap A->B	15	20	24	25	20	20	16	0.8	1.25	1.5625
swap LRNA->A	17.64	17.64	24	22.36	22.36	20	16	1.0	1.25	1.25

Thus ALICE has managed to skew the A-B price to 1.25, but since the LRNA-A price is still 1.0, the ensure_price check will pass. Now if BOB adds liquidity, he will receive less tokens than desired.

This is easily evident by looking at the state of the pool. If BOB adds 22.36 tokenA liquidity, BOB would get 50% of the shares of the pool, since the current state also has 22.36 tokenA reserves. However if this frontrunning attack never happened, BOB would have been received more than 50% shares of the pool, since originally the pool reserves only had 20 tokenA.

Thua ALICE was able to cause BOB losses by manipulating the price of tokenA, and bypass the ensure_price check, by making sure the LRNA-A price remained the same. Since this leads to unbounded losses due to slippage, this is a high severity issue.

The model here ignores fees. The fees only affect ALICE's profit chances. BOB will always eat a loss no matter what the fees are set to.

Tools Used

Manual Review

Recommended Mitigation Steps

ensure_price is not sufficient for checking slippage since it only checks LRNA-tokenA price, and not the prices between the tokens of the pool. Consider adding price checks of the end assets, i.e. tokenA to tokenB, since this is where the real manipulation happens and the MEV is extracted from. However in pools with n assets, this constitutes to checking n different prices, which might increase gas usage.

Assessed type

Invalid Validation

c4-pre-sort commented 8 months ago

0xRobocop marked the issue as primary issue

c4-pre-sort commented 8 months ago

0xRobocop marked the issue as sufficient quality report

c4-sponsor commented 8 months ago

enthusiastmartin (sponsor) disputed

enthusiastmartin commented 8 months ago

It is not obvious what is the problem here.

We dont believe this demonstrate any possible exploit.

Manipulating price of tokenB by selling LRNA does not have impact on tokenA.

OpenCoreCH commented 7 months ago

The main argument here is:

This is easily evident by looking at the state of the pool. If BOB adds 22.36 tokenA liquidity, BOB would get 50% of the shares of the pool, since the current state also has 22.36 tokenA reserves. However if this frontrunning attack never happened, BOB would have been received more than 50% shares of the pool, since originally the pool reserves only had 20 tokenA.

However, the fact that bob owns a smaller percentage than in an alternative scenario does not constitute slippage. What does Bob receive in both scenarios when he would immediately withdraw after providing liquidity, i.e. how has his net worth measured in tokenA changed? The finding does not show that this decreased in any way (and it should not with constant A/LRNA prices).

c4-judge commented 7 months ago

OpenCoreCH marked the issue as unsatisfactory: Insufficient proof

carrotsmuggler commented 7 months ago

The loss to the user comes after the frontrunner closes the price gap they created. After the three transactions above, the pAB is 1.25, which is above the market rate. So the frontrunner can then backrun to close this price gap by selling 1.888 B tokens for A tokens.

Operation	ALICE tokenA	ALICE LRNA	ALICE tokenB	Pool tokenA	Pool LRNA for tokenA	Pool LRNA for tokenB	Pool tokenB	pA	pB	pAB
Initial	20	20	20	20	20	20	20	1	1	1
swap A->B	15	20	24	25	20	20	16	0.8	1.25	1.5625
swap LRNA->A	17.64	17.64	24	22.36	22.36	20	16	1.0	1.25	1.25
Add liquidity				44.72	44.72	20	16	1.0	1.25	1.25
swap B->A	22.36	17.64	22.112	40	44.72	20	17.888	1.12	1.12	1.0

Now the price difference has been arbitraged away by Alice.

Now Bob supplied liquidity at a price pA of 1.0, but is now removing liquidity at the price pA of 1.12. Due to this, Bob will incur an impermanent loss according to the graph here. part of the total loss will be offset by minting Bob LRNA tokens, but this wont cover the entire loss. This loss is captured by Alice, who starts with 20+20+20=60 usd of tokens assuming all tokens are 1 usd, and ends up with 22.36+17.64+22.112=62.112 usd of tokens.

So there is a loss, and the ensure_price function cannot do anything to mitigate this. This last part was not included in the report since the focus was on the price manipulation part, and this same loss realization step was used in my report #116 and also explained in my Analysis report #136.

The issue I want to focus on here is that while A is not related to B, devaluing the LRNA token via swapping B does affect A. This basically removes all price protection from the system. I would even argue this as a valid high, since the other slippage related issues have been judged a medium because the system always had ensure_price in play which capped the losses to 1%. But this issue shows that ensure_price itself can be broken, and shows uncapped losses. the price is shown to move from 1.0 to 1.12, which is a 12% difference already and Bob eats this IL. This is equivalent to having no price protection at all.

carrotsmuggler commented 7 months ago

Attached is a running POC demonstrating the issue in the code. The POC follows the same 4 steps as above as well as liquidity removal:

A->B swap by BOB
LRNA -> A swap by BOB
Alice liquidity addition
B -> A swap by BOB
Alice liquidity removal

Test

#[test]
fn test_Attack_slippage() {
    ExtBuilder::default()
        .with_endowed_accounts(vec![
            (Omnipool::protocol_account(), DAI, 1000 * ONE),
            (Omnipool::protocol_account(), HDX, NATIVE_AMOUNT),
            (LP1, 1_000, 5000 * ONE),
            (LP1, LRNA, 5000 * ONE),
            (LP1, DAI, 5000 * ONE),
            (LP2, 1_000, 2000 * ONE),
            (LP2, DAI, 2000 * ONE),
            (LP3, 1_000, 1000 * ONE),
        ])
        .with_initial_pool(FixedU128::from_float(1.0), FixedU128::from(1))
        .with_token(1_000, FixedU128::from_float(1.0), LP2, 1000 * ONE)
        .with_min_withdrawal_fee(Permill::from_float(0.01))
        .build()
        .execute_with(|| {
            let current_position_id = <NextPositionId<Test>>::get();

            let omni_lrna = Tokens::free_balance(LRNA, &Omnipool::protocol_account());
            let omni_token = Tokens::free_balance(1000, &Omnipool::protocol_account());
            let omni_dai = Tokens::free_balance(DAI, &Omnipool::protocol_account());
            println!("omni_lrna : {}", omni_lrna);
            println!("omni_token: {}", omni_token);
            println!("omni_dai  : {}", omni_dai);
            let init_token_state = Omnipool::load_asset_state(1000).unwrap();
            let init_dai_state = Omnipool::load_asset_state(DAI).unwrap();
            println!("LRNA_token  : {}", init_token_state.hub_reserve);
            println!("LRNA_dai  : {}", init_dai_state.hub_reserve);
            println!("pA: {}", (init_token_state.hub_reserve as f64) / (omni_token as f64) );
            println!("");

            println!("Swap token -> DAI");
            println!("");
            let swap_amount = 200*ONE;
            assert_ok!(Omnipool::buy(
                RuntimeOrigin::signed(LP1),
                DAI,
                1_000,
                swap_amount,
                500000 * ONE
            ));

            let omni_lrna = Tokens::free_balance(LRNA, &Omnipool::protocol_account());
            let omni_token = Tokens::free_balance(1000, &Omnipool::protocol_account());
            let omni_dai = Tokens::free_balance(DAI, &Omnipool::protocol_account());
            println!("omni_lrna : {}", omni_lrna);
            println!("omni_token: {}", omni_token);
            println!("omni_dai  : {}", omni_dai);
            let sold_token_state = Omnipool::load_asset_state(1000).unwrap();
            let sold_dai_state = Omnipool::load_asset_state(DAI).unwrap();
            println!("LRNA_token: {}", sold_token_state.hub_reserve);
            println!("LRNA_dai  : {}", sold_dai_state.hub_reserve);
            let price_a = (sold_token_state.hub_reserve as f64) / (omni_token as f64);
            let price_b = (sold_dai_state.hub_reserve as f64) / (omni_dai as f64);
            println!("pA: {}", price_a );
            println!("pB: {}", price_b );
            println!("");

            let lpproduct_root = ((sold_token_state.hub_reserve * omni_token) as f64).sqrt() as i64 + 1;
            let to_sell = lpproduct_root as u128 - sold_token_state.hub_reserve;

            println!("Swap LRNA -> token");
            println!("");
            assert_ok!(Omnipool::sell(
                RuntimeOrigin::signed(LP1),
                LRNA,
                1_000,
                to_sell as u128,
                0
            ));

            let lrna_amt_init = Tokens::free_balance(LRNA, &LP3);
            let token_amt_init = Tokens::free_balance(1000, &LP3);
            println!("lrna_alice_init : {}", lrna_amt_init);
            println!("token_alice_init: {}", token_amt_init);
            let omni_lrna = Tokens::free_balance(LRNA, &Omnipool::protocol_account());
            let omni_token = Tokens::free_balance(1000, &Omnipool::protocol_account());
            let omni_dai = Tokens::free_balance(DAI, &Omnipool::protocol_account());
            println!("omni_lrna : {}", omni_lrna);
            println!("omni_token: {}", omni_token);
            println!("omni_dai  : {}", omni_dai);
            let sold_token_state = Omnipool::load_asset_state(1000).unwrap();
            let sold_dai_state = Omnipool::load_asset_state(DAI).unwrap();
            println!("LRNA_token: {}", sold_token_state.hub_reserve);
            println!("LRNA_dai  : {}", sold_dai_state.hub_reserve);
            let price_a = (sold_token_state.hub_reserve as f64) / (omni_token as f64);
            let price_b = (sold_dai_state.hub_reserve as f64) / (omni_dai as f64);
            println!("pA: {}", price_a );
            println!("pB: {}", price_b );
            println!("pAB: {}", price_b/price_a );
            println!("");

            println!("Add liquidity");
            println!("");
            let current_position_id = <NextPositionId<Test>>::get();
            assert_ok!(Omnipool::add_liquidity(RuntimeOrigin::signed(LP3), 1_000, 1000*ONE));
            let position = Positions::<Test>::get(current_position_id).unwrap();

            let omni_lrna = Tokens::free_balance(LRNA, &Omnipool::protocol_account());
            let omni_token = Tokens::free_balance(1000, &Omnipool::protocol_account());
            let omni_dai = Tokens::free_balance(DAI, &Omnipool::protocol_account());
            println!("omni_lrna : {}", omni_lrna);
            println!("omni_token: {}", omni_token);
            println!("omni_dai  : {}", omni_dai);
            let sold_token_state = Omnipool::load_asset_state(1000).unwrap();
            let sold_dai_state = Omnipool::load_asset_state(DAI).unwrap();
            println!("LRNA_token: {}", sold_token_state.hub_reserve);
            println!("LRNA_dai  : {}", sold_dai_state.hub_reserve);
            let price_a = (sold_token_state.hub_reserve as f64) / (omni_token as f64);
            let price_b = (sold_dai_state.hub_reserve as f64) / (omni_dai as f64);
            println!("pA: {}", price_a );
            println!("pB: {}", price_b );
            println!("pAB: {}", price_b/price_a );
            println!("");

            let dai_final_p1 = ((omni_token*omni_dai) as f64).sqrt();
            let dai_final_p2 = (sold_dai_state.hub_reserve as f64 / sold_token_state.hub_reserve as f64).sqrt();
            let dai_final = (dai_final_p1 * dai_final_p2) as u128;
            // some withcraft numbers from manual linear interp to make pAB=1 again
            let to_sell = (((dai_final - omni_dai) as f64)*0.75*0.873*0.9573) as u128;

            println!("Swap DAI->token");
            println!("");
            assert_ok!(Omnipool::sell(
                RuntimeOrigin::signed(LP1),
                DAI,
                1_000,
                to_sell,
                0
            ));

            let lrna_amt = Tokens::free_balance(LRNA, &LP3);
            let token_amt = Tokens::free_balance(1000, &LP3);
            println!("lrna_alice : {}", lrna_amt);
            println!("token_alice: {}", token_amt);
            let omni_lrna = Tokens::free_balance(LRNA, &Omnipool::protocol_account());
            let omni_token = Tokens::free_balance(1000, &Omnipool::protocol_account());
            let omni_dai = Tokens::free_balance(DAI, &Omnipool::protocol_account());
            println!("omni_lrna : {}", omni_lrna);
            println!("omni_token: {}", omni_token);
            println!("omni_dai  : {}", omni_dai);
            let sold_token_state = Omnipool::load_asset_state(1000).unwrap();
            let sold_dai_state = Omnipool::load_asset_state(DAI).unwrap();
            println!("LRNA_token: {}", sold_token_state.hub_reserve);
            println!("LRNA_dai  : {}", sold_dai_state.hub_reserve);
            let price_a = (sold_token_state.hub_reserve as f64) / (omni_token as f64);
            let price_b = (sold_dai_state.hub_reserve as f64) / (omni_dai as f64);
            println!("pA: {}", price_a );
            println!("pB: {}", price_b );
            println!("pAB: {}", price_b/price_a );
            println!("");

            println!("remove liquidity");
            println!("");
            assert_ok!(Omnipool::remove_liquidity(
                RuntimeOrigin::signed(LP3),
                current_position_id,
                position.shares
            ));

            let lrna_amt = Tokens::free_balance(LRNA, &LP3);
            let token_amt = Tokens::free_balance(1000, &LP3);
            println!("lrna_alice_final : {}", lrna_amt);
            println!("token_alice_final: {}", token_amt);
            let omni_lrna = Tokens::free_balance(LRNA, &Omnipool::protocol_account());
            let omni_token = Tokens::free_balance(1000, &Omnipool::protocol_account());
            let omni_dai = Tokens::free_balance(DAI, &Omnipool::protocol_account());
            println!("omni_lrna : {}", omni_lrna);
            println!("omni_token: {}", omni_token);
            println!("omni_dai  : {}", omni_dai);
            let sold_token_state = Omnipool::load_asset_state(1000).unwrap();
            let sold_dai_state = Omnipool::load_asset_state(DAI).unwrap();
            println!("LRNA_token: {}", sold_token_state.hub_reserve);
            println!("LRNA_dai  : {}", sold_dai_state.hub_reserve);
            let price_a = (sold_token_state.hub_reserve as f64) / (omni_token as f64);
            let price_b = (sold_dai_state.hub_reserve as f64) / (omni_dai as f64);
            println!("pA: {}", price_a );
            println!("pB: {}", price_b );
            println!("pAB: {}", price_b/price_a );
            println!("");
        });
}

Output

running 1 test                                                                                                                                                                                                                                                                                                                                                                              
omni_lrna : 12000000000000000                                                                                                                                                                                                                                                                                                                                                               
omni_token: 1000000000000000                                                                                                                                                                                                                                                                                                                                                                
omni_dai  : 1000000000000000                                                                                                                                                                                                                                                                                                                                                                
LRNA_token  : 1000000000000000                                                                                                                                                                                                                                                                                                                                                              
LRNA_dai  : 1000000000000000                                                                                                                                                                                                                                                                                                                                                                
pA: 1                                                                                                                                                                                                                                                                                                                                                                                       

Swap token -> DAI                                                                                                                                                                                                                                                                                                                                                                           

omni_lrna : 12000000000000000                                                                                                                                                                                                                                                                                                                                                               
omni_token: 1333333333333336                                                                                                                                                                                                                                                                                                                                                                
omni_dai  : 800000000000000                                                                                                                                                                                                                                                                                                                                                                 
LRNA_token: 749999999999999                                                                                                                                                                                                                                                                                                                                                                 
LRNA_dai  : 1250000000000001                                                                                                                                                                                                                                                                                                                                                                
pA: 0.5624999999999981                                                                                                                                                                                                                                                                                                                                                                      
pB: 1.5625000000000013                                                                                                                                                                                                                                                                                                                                                                      

Swap LRNA -> token                                                                                                                                                                                                                                                                                                                                                                          

lrna_alice_init : 0                                                                                                                                                                                                                                                                                                                                                                         
token_alice_init: 1000000000000000                                                                                                                                                                                                                                                                                                                                                          
omni_lrna : 12250000000000002                                                                                                                                                                                                                                                                                                                                                               
omni_token: 1000000000000000                                                                                                                                                                                                                                                                                                                                                                
omni_dai  : 800000000000000                                                                                                                                                                                                                                                                                                                                                                 
LRNA_token: 1000000000000001                                                                                                                                                                                                                                                                                                                                                                
LRNA_dai  : 1250000000000001                                                                                                                                                                                                                                                                                                                                                                
pA: 1.000000000000001                                                                                                                                                                                                                                                                                                                                                                       
pB: 1.5625000000000013                                                                                                                                                                                                                                                                                                                                                                      
pAB: 1.5624999999999996                                                                                                                                                                                                                                                                                                                                                                     

Add liquidity                                                                                                                                                                                                                                                                                                                                                                               

omni_lrna : 13250000000000003                                                                                                                                                                                                                                                                                                                                                               
omni_token: 2000000000000000                                                                                                                                                                                                                                                                                                                                                                
omni_dai  : 800000000000000                                                                                                                                                                                                                                                                                                                                                                 
LRNA_token: 2000000000000002                                                                                                                                                                                                                                                                                                                                                                
LRNA_dai  : 1250000000000001                                                                                                                                                                                                                                                                                                                                                                
pA: 1.000000000000001                                                                                                                                                                                                                                                                                                                                                                       
pB: 1.5625000000000013                                                                                                                                                                                                                                                                                                                                                                      
pAB: 1.5624999999999996                                                                                                                                                                                                                                                                                                                                                                     

Swap DAI->token                                                                                                                                                                                                                                                                                                                                                                             

lrna_alice : 0                                                                                                                                                                                                                                                                                                                                                                              
token_alice: 0                                                                                                                                                                                                                                                                                                                                                                              
omni_lrna : 13250000000000003                                                                                                                                                                                                                                                                                                                                                               
omni_token: 1843880761593751                                                                                                                                                                                                                                                                                                                                                                
omni_dai  : 925358434999999                                                                                                                                                                                                                                                     
LRNA_token: 2169337672650058                                                                                                                                                                                                                                                    
LRNA_dai  : 1080662327349945                                                                                                                                                                                                                                                    
pA: 1.1765064844946913                                                                                                                                                                                                                                                          
pB: 1.1678310657533977                                                                                                                                                                                                                                                          
pAB: 0.992626119060432                                                                                                                                                                                                                                                          

remove liquidity                                                                                                                                                                                                                                                                

lrna_alice_final : 87082934812975                                                                                                                                                                                                                                               
token_alice_final: 912720976988906                                                                                                                                                                                                                                              
omni_lrna : 12176177852038226                                                                                                                                                                                                                                                   
omni_token: 931159784604845                                                                                                                                                                                                                                                     
omni_dai  : 925358434999999                                                                                                                                                                                                                                                     
LRNA_token: 1095515524688281                                                                                                                                                                                                                                                    
LRNA_dai  : 1080662327349945                                                                                                                                                                                                                                                    
pA: 1.1765064844946922                                                                                                                                                                                                                                                          
pB: 1.1678310657533977                                                                                                                                                                                                                                                          
pAB: 0.9926261190604312                                                                                                                                                                                                                                                         

test tests::remove_liquidity::test_Attack_slippage ... ok

Analysis

From this output, we see that Alice started with 1000 A tokens, and ended up with 912.7 A tokens and 87.08 LRNA tokens. This is because the price was manipulated, bypassing the ensure_price check.

The values of pA, pB and pAB can be followed through the output to get a better picture. After the first 2 steps (token -> DAI swap and LRNA -> token swap), the pA goes back to 1.0. This will ensure that ensure_price doesn't reject the liquidity addition.

Step 3 (liquidity addition) proceeds smoothly, but here alive is adding liquidity at a price point pAB of 1.562 instead of 1! But this is allowed since pA is still ~1.0

Step 4 (B->A swap) sees the frontrunner closing the arbitrage position and resetting the price pAB to 0.993, or close to 1. Due to this, the token-LRNA price changes and is now 1.176! Since the pA has increased, Alice has suffered impermanent loss and part of her los will be subsidised with LRNA tokens.

After Step 5 (Liquidity removal), we see Alice did not get back the full 1000 A tokens. Instead she gets credited with some LRNA tokens to compensate her loss. The main issue is that the attacker can manipulate the price as much as they want, so they can dictate how much of Alice's holdings will be paid out in LRNA tokens. In this example, the manipulation magnitude was small since only 10% of the pool is used to imbalance it (100 tokens are sold into a liquidity of 1000) but this magnitude can be larger and force Alice to eat a much larger impermanent loss.

The protocol has no way of defending against this, since the ensure_price module is being broken.

carrotsmuggler commented 7 months ago

I understand there are regulations regarding how much supplementary material can be added in the end of a contest for PJQA, however I would like to stress a few key points.

The core attack algorithm / strategy remains exactly the same. This material only serves to better prove the correctness of the report.
This kind of spreadsheet analysis was already accepted for another of my reports. However it proved to be too complicated to word properly in this scenario. I apologise for that.
This is a unique issue. So there is no chance of me plagiarising this from any other person's report.
This is a live protocol with an active BBP. So there is no chance of me plagiarising this from any other sources either.

Thank you for your consideration.

poliwop commented 7 months ago

The loss to the user comes after the frontrunner closes the price gap they created. After the three transactions above, the pAB is 1.25, which is above the market rate. So the frontrunner can then backrun to close this price gap by selling 1.888 B tokens for A tokens.

Operation ALICE tokenA ALICE LRNA ALICE tokenB Pool tokenA Pool LRNA for tokenA Pool LRNA for tokenB Pool tokenB pA pB pAB Initial 20 20 20 20 20 20 20 1 1 1 swap A->B 15 20 24 25 20 20 16 0.8 1.25 1.5625 swap LRNA->A 17.64 17.64 24 22.36 22.36 20 16 1.0 1.25 1.25 Add liquidity 44.72 44.72 20 16 1.0 1.25 1.25 swap B->A 22.36 17.64 22.112 40 44.72 20 17.888 1.12 1.12 1.0 Now the price difference has been arbitraged away by Alice.

This is not a correct analysis of closing the arb opportunity/price gap.

Alice's initial swap A->B and swap LRNA -> A effectively netted to a swap LRNA->B that changed the LRNA/B price but left the LRNA/A price unchanged. Note this also means the price of C/A for every other token C in the Omnipool was unchanged. So at the point when Bob adds liquidity to asset A, the price of LRNA/B has been manipulated, but nothing else. A swap B-> A will therefore not close the arb, because it will change the C/A price for every other asset C in Omnipool.

carrotsmuggler commented 7 months ago

When Bob adds liquidity to the pool, the price of A-B has been manipulated. This is where the arb opportunity comes from. From the spreadsheet as well as the POC, the pAB during liquidity addition is not 1 anymore. After liquidity addition, the pAB is changed back to close the arb by Alice. This is because pAB is 1 in other exchanges, but is more than 1 in this exchange due to the swaps.

Once the arb gap is closed and pAB returns back to 1, we see that pA goes up. This change in pA is the impermanent loss faced by the user.

In the POC itself you can see clearly how the protocol behaves. Instead of giving Bob back the 1000 tokens they provided, instead they are partially payed back in LRNA tokens. If there was no manipulation involved, Bob would have received tokens back with the same value. However that is not the case anymore, and the amount of money bob gets back depends heavily on the spot price of LRNA at that moment.

The POC clearly demonstrates that a malicious user can force a user's token A deposit to be paid back in tokenA + LRNA. This happens even if there is no apparent change in the price of the tokens in outside markets. Thus this is an issue.

Also this analysis is done for a pool with only 2 assets A and B. For larger pools, the manipulation would require more steps, since closing the arb would require swapping each component against A. However I believe analyzing the situation with only 2 assets will reflect the situation with more assets as well and will be easier to understand and analyze.

Frankcastleauditor commented 7 months ago

Hi @OpenCoreCH . I think this report has the same invalid assumption as the report number #116 , the report assumed that no change in the LRNA in the pools of the traded asset , which is not correct and the LRNA changed , and affect all the price calculations .
As shown here Screenshot_20240315_000604_Chrome The swap done by the report did not change the LRNA reserve of the pool . as shown here the amount of LRNA in the pool of asset A wrongly calculated since it should be equal to 16 in the second row instead of 20 , and since Alice only provided LRNA to get assetA in a trade , this wrong calculation lead to different prices of the final state after Alice added liquidity in wrong prices of assetA and assetB .

carrotsmuggler commented 7 months ago

This report also has a valid POC demonstrating the issue. Also the LRNA token count does not matter for this, only the change in the pA.

The POC results show LRNA_token, the LRNA corresponding to token A, and LRNA_DAI, LRNA corresponding to token B. And omni_token and omni_dai show the reserve values of the two tokens. pA is calculated as LRNA_token/omni_token. We can clearly see pA increase after the DAI->token (B->A) swap, which leads to the impermanent loss of the user.

Some numbers on the spreadsheet are incorrect since i didn't update the hub components, but the result of the POC is irrefutable proof that the system still works the same they the report describes the problem.

OpenCoreCH commented 7 months ago

Thanks for the PoC @carrotsmuggler.

If we leave out pB and pAB for a moment and only focus on pA, the attack essentially involves manipulating the pA price directly after a user has added liquidity. This will lead to IL in most AMMs, you could perform steps 3 - 5 also on Uniswap (with IL to the LP). However, steps 1 and 2 ensure that B is mispriced (which can only be done risk free if the front and back running is guaranteed, because someone else could intervene otherwise, but let's assume this for the moment), which ensures that the step 4 rebalances the pool again.

Of course, if there were more tokens, there would still be an arb opportunity after step 4 as mentioned by @poliwop. But if we assume that there are only two tokens in the pool, this seems to be a valid sandwich attack to me, at least I cannot find an issue in the PoC and could not find an arb opportunity after step 4 (if there were one, it would not really be valid, because it would be stupid to withdraw the liquidity in this state and someone would close it). What do you think @enthusiastmartin @poliwop ? You definitely know the math behind the system better than me, so would love your input.

poliwop commented 7 months ago

I think the confusion here is coming from using the wrong baseline for comparison. In the POC, Bob is selling LRNA into the Omnipool. That changes the price of LRNA, which gives LPs impermanent loss. This is expected.

Here is a simplified version of the suggested manipulation, as demonstrated in the POC:

Bob swaps LRNA -> DAI*
Alice adds liquidity in TKN
Bob arbitrages DAI/TKN price back to initial value
Alice withdraws liquidity

*(note that I'm combining swapping TKN -> DAI and LRNA -> TKN, because ignoring fees, they are equivalent to simply swapping LRNA -> DAI, if the TKN quantity swapped matches, as it does in the POC)

Notice that the first transaction happens only in the DAI part of Omnipool, while the second only affects the TKN part of Omnipool. So these two transactions can be flipped with no change in the outcome. Thus this matches

Alice adds liquidity in TKN
Bob swaps LRNA -> DAI
Bob arbitrages DAI/TKN price back to initial value
Alice withdraws liquidity

This is business-as-usual, and this is the correct baseline to compare to. Alice added TKN liquidity, and when Bob sold LRNA to the Omnipool, the LRNA/TKN price changed, causing impermanent loss for Alice. The "sandwich" in the first sequence of transactions doesn't actually do anything, it doesn't benefit Bob or hurt Alice.

Typically we would consider something price manipulation only if the price is moved away from the "market price". But the change in LRNA price here is a real change in the market price: LRNA is sold to the pool, which makes it less valuable, and that isn't reversed (it cannot be reversed, since LRNA cannot be bought from the Omnipool). So the fact that it causes the LP impermanent loss is the correct outcome.

The ensure_price check referenced here is in place to protect against specific exploits which cannot be executed via the strategy outlined in this issue primarily because LRNA cannot be bought from the Omnipool.

OpenCoreCH commented 7 months ago

Thanks for your comment @poliwop Indeed, the order of the operations does not actually matter here, the result should be the same if Bob performs his actions between the liquidity addition. So ultimately, the finding displays a scenario where the LPs incur impermanent loss, which is the intended behaviour in this scenario. Therefore keeping the initial judging.

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

[H03] `ensure_price` slippage control in omnipools can be bypassed with swaps #114

Lines of code

Vulnerability details

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps

Assessed type

Test

Output

Analysis