HydraDX - carrotsmuggler's results

Lines of code

https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/stableswap/src/lib.rs#L677-L681

Vulnerability details

Impact

The stableswap pool allows users to send in any tokens to the pool, as confirmed by the dev on the discord channel. This opens it up to a potential attack vector, where initial user can deposit a small amount of tokens, and then "donate" a large amount of tokens such that the share ratio is set really high. This will lead to rounding losses and potentially block other users from making small sized deposits. The protocol prevents this attack vector by instituting a T::MinPoolLiquidity check, ensuring that the amount of LP shares minted is always either 0 or above this minimum threshold.

This is reflected in the liquidity addition functions, where the minimum amount check is enforced.

ensure!(
        asset.amount >= T::MinTradingLimit::get(),
        Error::<T>::InsufficientTradingAmount
    );

This is also implemented in the function remove_liquidity_one_asset.

let share_issuance = T::Currency::total_issuance(pool_id);

    ensure!(
        share_issuance == share_amount
            || share_issuance.saturating_sub(share_amount) >= T::MinPoolLiquidity::get(),
        Error::<T>::InsufficientLiquidityRemaining
    );

Where share_issuance checks the total supply of the pool token. However, the check in withdraw_asset_amount is faulty.

let current_share_balance = T::Currency::free_balance(pool_id, &who);
    ensure!(
        current_share_balance == shares
            || current_share_balance.saturating_sub(shares) >= T::MinPoolLiquidity::get(),
        Error::<T>::InsufficientShareBalance
    );

Instead of checking against the total issuance, here the contract checks against the user's balance. Thus, users are allowed to redeem all the LP tokens in their account, even if that leads to the situation where the total issuance is below the threshold, allowing an inflation attack. Users can just send 1 wei of LP token to some other account, and then the current_share_balance == shares will pass when redeeming the remaining balance, allowing LP removal and leaving 1 wei of LP still existing.

Since inflation attacks and DOS attacks can be carried out due to this valid pool state, this is a high severity issue. The recent Wise lending hack was also due to such inflation attacks.

Proof of Concept

The user can trigger this situation by following these steps.

1. Alice deposits liquidity and gets 1*ONE LP tokens.
2. Alice sends 1 wei of that LP otken to BOB. ALice now has 1*ONE-1 LP tokens.
3. Alice calls withdraw_asset_amount function with all her LP token balance. Since current_share_balance == shares check passes, all LP tokens are redeemed, except for the 1 wei in BOB's account
4. Pool now has only 1 wei of LP tokens issued. Users can "donate" tokens to the pool to raise the share ratio.

Tools Used

Manual Review

Recommended Mitigation Steps

Check with share_issuance instead of the user's balance, just like in remove_liquidity_one_asset function.

Assessed type

Math

Lines of code

https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/stableswap/src/lib.rs#L475-L492

Vulnerability details

Impact

Liquidity is added to the stableswap pools by calling the add_liquidity function.

pub fn add_liquidity(
        origin: OriginFor<T>,
        pool_id: T::AssetId,
        assets: Vec<AssetAmount<T::AssetId>>,
    ) -> DispatchResult

The issue is that there is no minimum_amount_out parameter in this function, so the liquidity provider has no control over how many tokens get minted.

In curve-style pools, the amount of shares received for adding liquidity to a pool depends on the price of the pool. So a malicious user can do a large swap and then add liquidity to the pool, causing lesser number of shares to be minted to the liquidity provider. To defend against this attack vector, the curve protocol has a slippage control parameter in its contracts.

def add_liquidity(amounts: uint256[N_COINS], min_mint_amount: uint256) -> uint256:
# ...
assert mint_amount >= min_mint_amount, "Slippage screwed you"

Proof of Concept

A malicious user can frontrun the liquidity addition with a large swap. This would cause the amount of shares to be minted to be skewed, and depending on the price and ratio of deposited assets, the liquidity provider can end up with lesser shares than expected.

Tools Used

Manual Review

Recommended Mitigation Steps

Add a min_mint_amount parameter to ensure the liquidity provider does not miss out on shares due to frontrunning. A similar measure is done on the add_liquidity_shares function, but is missing on this function.

Assessed type

MEV

Lines of code

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

Vulnerability details

Impact

The functions add_liquidity in the omnipool pallet does not check for slippage. There is no minimum_out parameter which makes sure that no other users can frontrun the transaction and cause the liquidity provider to lose tokens. This can lead to slippage losses for liquidity providers.

During addition of liquidity, LP shares are minted to the liquidity provider, based on the fraction of liquidity they provide. This is calculated in the calculate_add_liquidity_state_changes function in the math pallet.

let delta_shares_hp = shares_hp
    .checked_mul(amount_hp)
    .and_then(|v| v.checked_div(reserve_hp))?;

Thus the calculation is based only on the reserve amount of that asset.

Let's assume the state of the liquidity pool is such that the price is 1 all around, and the pool has 1*ONE token A, 1*ONE corresponding LRNA, 1*ONE token B and 1*ONE corresponding LRNA for token B. Say 1*ONE amount of LP shares have been minted out.

Say a user wants to add liquidity equal to 1*ONE token A. They should therefore get 1*ONE liquidity shares as well, since

shares = total_shares * amount / reserve = 1*ONE * 1*ONE / 1*ONE = 1*ONE

However, a malicious user can now come in and carry out a swap A->B, devaluing the price of A, so now there are more A tokens in the pool. Say now there are 1.2*ONE token A, and the amount of token B (which is not important here) has gone down.

Now, the user gets minted 0.83*ONE LP shares, since

shares = total_shares * amount / reserve = 1*ONE * 1*ONE / 1.2*ONE = 0.83*ONE

So the user has been made to eat a loss of 0.17*ONE due to the frontrunning attack.

This is an exaggerated scenario. There is an ensure_price check in the add_liquidity function, which checks if the price of the asset has not changed by more than 1% from the oracle price. Thus the maximum price deviation that can happen is 2% (if the spot price was changed from +1% to -1%). 2% slippage is already unacceptable for a number of cases, since the industry standard for swaps has been 0.5%, and even lower for liquidity additions.

While the frontrunning/sandwich attack might not be profitable, it can still be used to cause losses to the liquidity provider.

Proof of Concept

Users can be made to eat a loss during liquidity addition by following the steps below:

1. User wants to add liquidity equal to 1*ONE token A. They should therefore get 1*ONE liquidity shares as well assuming the pool has 1*ONE token A in reserve.
2. Malicious frontrunner does a swap on the pool, devaluing the price of A. Now there is 1.2*ONE token A in the pool and more token B than before.
3. User adds liquidity. User gets minted 0.83*ONE LP shares instead of 1*ONE LP shares.
4. Malicious user swaps back, recovering most of their investment.

Tools Used

Manual Review

Recommended Mitigation Steps

Add a minimum_shares_out parameter for liquidity addition.

Assessed type

MEV

Lines of code

https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/omnipool/src/lib.rs#L716-L867

Vulnerability details

Impact

The liquidity removal function in the omnipool pallet lacks slippage control. There is no minimum_amount_out parameter to ensure that the user gets out atleast a certain amount of tokens. This can lead to slippage losses for liquidity providers if malicious users frontrun the liquidity withdrawer.

During liquidity removal, since there are lots of different fees involved, the scenario gets complicated and a POC is used to study the effect further. A POC is presented in the next section, which has ALICE depositing LP of token_1000 to the pool, the actor LP3 carrying out a swap, and then ALICE removing liquidity immediately after. In case ALICE receives any LRNA tokens, she swaps them out to token_1000. We compare the amount of token_1000 ALICE would end up with in different scenarios.

In all scenarios, ALICE is assumed to remove liquidity at the same price she put in. However the bad actor LP3 frontruns her removal, and we want to study the effect of her losses. In scenario 1, there is no action by LP3, and ALICE deposits and withdraws, to get a baseline measurement.

Scenario 1- Liq Add - Liq remove

Here there is no frontrunner in order to get a baseline measurement.

running 1 test
lrna_init : 2000000000000000
token_init: 5000000000000000

lrna_add : 2000000000000000
token_add: 4000000000000000

lrna_remove : 2000000000000000
token_remove: 4990000000000000

Here we see ALICE started with 5000*ONE token_1000s, and ends up with 4990*ONE token_1000s. This is due to withdrawal fees, and is the acceptable baseline. Any lower amounts due to frontrunning would be unacceptable. This is a 0.1% loss.

Scenario 2- Liq Add - token->DAI swap - Liq remove

Here, the frontrunner devalues token_1000 by selling a bunch of it for DAI. Since the price is now lower, some of Alice's shares will be burnt.

running 1 test
lrna_init : 2000000000000000
token_init: 5000000000000000

lrna_add : 2000000000000000
token_add: 4000000000000000

lrna_remove : 2000000000000000
token_remove: 4961892744479493

In this scenario, ALICE ends up with 4961.89*ONE token_1000s. This is nearly a 1% loss. Since some of her share tokens are burnt, the other liquidity providers profit from this, since their liquidity positions are now worth more.

Scenario 3- Liq Add - DAI->token swap - Liq remove

Here, the frontrunner buys up token_1000 increasing its price. Alice gets minted LRNA tokens to compensate the increase in price, but she swaps them out to token_1000 immediately. We then check her token_1000 balance and compare it to the beginning.

running 1 test
lrna_init : 2000000000000000
token_init: 5000000000000000

lrna_add : 2000000000000000
token_add: 4000000000000000

lrna_remove : 2187637667548876
token_remove: 4841500000000000

lrna_end : 2000000000000000
token_end: 4958579804661321

Here ALICE ends up with 4958.57*ONE token_1000s. This is again a 1% loss. The frontrunner can even sandwich the LRNA->token_1000 swap and even profit in this scenario.

Thus in all frontrunning scenarios, ALICE realizes a slippage loss due to insufficient parameters. The losses will be capped to 2%, since the ensure_price check in the remove_liquidity function checks if the price of the asset has not changed by more than 1% from the oracle price. Thus the maximum price deviation that can happen is 2% (if the spot price was changed from +1% to -1%). 2% slippage is already unacceptable for a number of cases, since the industry standard for swaps has been 0.5%, and even lower for liquidity removals.

Proof of Concept

The following test was run to generate the figures above. For the different scenarios, the buy was commented (scenario 1), used to swap token->DAI (scenario 2), and used to swap DAI->token (scenario 3).

#[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, DAI, 5000 * ONE),
			(LP1, LRNA, 2000 * ONE),
			(LP2, 1_000, 2000 * ONE),
			(LP2, DAI, 2000 * ONE),
			(LP3, 1_000, 5000 * ONE),
			(LP3, LRNA, 2000 * ONE),
			(LP3, DAI, 2000 * 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 lrna_amt = Tokens::free_balance(LRNA, &LP3);
            let token_amt = Tokens::free_balance(1000, &LP3);
            let dai_amt = Tokens::free_balance(DAI, &LP3);
            println!("lrna_init : {}", lrna_amt);
            println!("token_init: {}", token_amt);
            println!("dai_init  : {}", dai_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);
            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 lrna_amt_add = Tokens::free_balance(LRNA, &LP3);
            let token_amt_add = Tokens::free_balance(1000, &LP3);
            let dai_amt_add = Tokens::free_balance(DAI, &LP3);
            println!("lrna_add : {}", lrna_amt_add);
            println!("token_add: {}", token_amt_add);
            println!("dai_add  : {}", dai_amt_add);
            let omni_lrna_add = Tokens::free_balance(LRNA, &Omnipool::protocol_account());
            let omni_token_add = Tokens::free_balance(1000, &Omnipool::protocol_account());
            let omni_dai_add = Tokens::free_balance(DAI, &Omnipool::protocol_account());
            println!("omni_lrna_add : {}", omni_lrna_add);
            println!("omni_token_add: {}", omni_token_add);
            println!("omni_dai_add  : {}", omni_dai_add);
            println!("");

            let swap_amount = 300*ONE;
            assert_ok!(Omnipool::buy(
                RuntimeOrigin::signed(LP1),
                1_000,
                LRNA,
                swap_amount,
                500000 * ONE
            ));

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

            let lrna_amt_remove = Tokens::free_balance(LRNA, &LP3);
            let token_amt_remove = Tokens::free_balance(1000, &LP3);
            let dai_amt_remove = Tokens::free_balance(DAI, &LP3);
            println!("lrna_remove : {}", lrna_amt_remove);
            println!("token_remove: {}", token_amt_remove);
            println!("dai_remove  : {}", dai_amt_remove);
            let omni_lrna_remove = Tokens::free_balance(LRNA, &Omnipool::protocol_account());
            let omni_token_remove = Tokens::free_balance(1000, &Omnipool::protocol_account());
            let omni_dai_remove = Tokens::free_balance(DAI, &Omnipool::protocol_account());
            println!("omni_lrna_remove : {}", omni_lrna_remove);
            println!("omni_token_remove: {}", omni_token_remove);
            println!("omni_dai_remove  : {}", omni_dai_remove);
            println!("");

            if lrna_amt_remove > lrna_amt{
                let diff = lrna_amt_remove - lrna_amt;
                assert_ok!(Omnipool::sell(
                    RuntimeOrigin::signed(LP3),
                    LRNA,
                    1_000,
                    diff,
                    0
                ));
            }

            let lrna_amt_end = Tokens::free_balance(LRNA, &LP3);
            let token_amt_end = Tokens::free_balance(1000, &LP3);
            let dai_amt_end = Tokens::free_balance(DAI, &LP3);
            println!("lrna_end : {}", lrna_amt_end);
            println!("token_end: {}", token_amt_end);
            println!("dai_end  : {}", dai_amt_end);
            let omni_lrna_end = Tokens::free_balance(LRNA, &Omnipool::protocol_account());
            let omni_token_end = Tokens::free_balance(1000, &Omnipool::protocol_account());
            let omni_dai_end = Tokens::free_balance(DAI, &Omnipool::protocol_account());
            println!("omni_lrna_end : {}", omni_lrna_end);
            println!("omni_token_end: {}", omni_token_end);
            println!("omni_dai_end  : {}", omni_dai_end);
            println!("");

		});
}

Tools Used

Substrate

Recommended Mitigation Steps

Add a slippage limit for liquidity removal. The in-built limit of 2% is too large for most use cases.

Assessed type

MEV

Lines of code

https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/stableswap/src/lib.rs#L638 https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/stableswap/src/lib.rs#L551

Vulnerability details

Impact

The contracts for stableswap has 2 functions dealing with removal of liquidity: remove_liquidity_one_asset and withdraw_asset_amount. However, both these functions allow redeeming LP tokens and pay out in only one token. Critically, this contract is missing Curve protocol's remove_liquidity function, which allows redeeming LP tokens for all the different tokens in the pool.

The result of this decision is that when the complete liquidity of a pool is to be removed, the contract reverts with an arithmetic overflow. In curve protocol, when removing the complete liquidity, the composing tokens are removed from the pool. However here, they also need to be converted to a single token, using a liqudity which wont exist anymore. This leads to an issue somewhere in the mathematics of the curve liquidity calculation, and thus reverts.

Proof of Concept

A simple POC to remove the complete liquidity is coded up below. This POC reverts when the entire amount of shares is being redeemed.

#[test]
fn test_Attack_min_shares() {
	let asset_a: AssetId = 1;
	let asset_b: AssetId = 2;
	let asset_c: AssetId = 3;

	ExtBuilder::default()
		.with_endowed_accounts(vec![
			(BOB, asset_a, 2*ONE),
			(ALICE, asset_a, 1*ONE),
			(ALICE, asset_b, 1*ONE),
			(ALICE, asset_c, 1*ONE),
		])
		.with_registered_asset("one".as_bytes().to_vec(), asset_a, 18)
		.with_registered_asset("two".as_bytes().to_vec(), asset_b, 6)
		.with_registered_asset("three".as_bytes().to_vec(), asset_c, 6)
		.with_pool(
			ALICE,
			PoolInfo::<AssetId, u64> {
				assets: vec![asset_a, asset_b, asset_c].try_into().unwrap(),
				initial_amplification: NonZeroU16::new(2000).unwrap(),
				final_amplification: NonZeroU16::new(2000).unwrap(),
				initial_block: 0,
				final_block: 0,
				fee: Permill::zero(),
			},
			InitialLiquidity {
				account: ALICE,
				assets: vec![
					AssetAmount::new(asset_a, 1*ONE),
					AssetAmount::new(asset_b, 1*ONE),
					AssetAmount::new(asset_c, 1*ONE),
				],
			},
		)
		.build()
		.execute_with(|| {
			let pool_id = get_pool_id_at(0);

			let received = Tokens::free_balance(pool_id, &ALICE);
            println!("LP tokens received: {}", received);


            assert_ok!(Stableswap::remove_liquidity_one_asset(
                RuntimeOrigin::signed(ALICE),
                pool_id,
                asset_a,
                received,
                0
            ));

            let asset_a_remliq_bal = Tokens::free_balance(asset_a, &ALICE);
            println!("asset a rem: {}", asset_a_remliq_bal);
		});
}

Here ALICE adds liquidity, and is trying to redeem all her LP tokens. This reverts with the following:

running 1 test
LP tokens received: 23786876415280195891619
thread 'tests::add_liquidity::test_Attack_min_shares' panicked at 'Expected Ok(_). Got Err(
    Arithmetic(
        Overflow,
    ),
)', pallets/stableswap/src/tests/add_liquidity.rs:889:13
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
test tests::add_liquidity::test_Attack_min_shares ... FAILED

This is because the internal math of the stableswap algorithm fails when there is no more liquidity.

Tools Used

Substrate

Recommended Mitigation Steps

Allow multi-token liquidity withdrawal, which would allow complete redeeming of all LP tokens.

Assessed type

Under/Overflow

Lines of code

https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/omnipool/src/lib.rs#L1442-L1525

Vulnerability details

Impact

The sacrifice_position function can be used by any liquidity provider to hand over their liquidity position to the protocol. The protocol can then choose to remove this liquidity via the withdraw_protocol_liquidity function. This is similar to the remove_liquidity function, but with one key difference. The remove_liquidity function has a safe_withdrawal option, where if trading is ongoing, the price difference is limited to 1% via the ensure_price function. This is not present in the withdraw_protocol_liquidity function.

// remove_liquidity
if !safe_withdrawal {
        T::PriceBarrier::ensure_price(
            &who,
            T::HubAssetId::get(),
            asset_id,
            EmaPrice::new(asset_state.hub_reserve, asset_state.reserve),
        )
        .map_err(|_| Error::<T>::PriceDifferenceTooHigh)?;
    }

Thus when the admin decides to call withdraw_protocol_liquidity to remove the liquidity, they can be frontrun to eat slippage loss. The admin has to pass in a price parameter, and if the frontrunner manipulates the spot price to be different from the price passed in, the admin will eat losses. A deeper dive and simulation of losses has been done in another issue titled No slippage check in remove_liquidity function in omnipool can lead to slippage losses during liquidity withdrawal. where the losses are limited to 2% due to the ensure_price check. However, the losses here can be much higher due to the lack of this check altogether.

Since higher losses can be possible, this is a high severity issue.

Proof of Concept

The ensure_price check is missing from the withdraw_protocol_liquidity function.

Tools Used

Substrate

Recommended Mitigation Steps

Add a safe_withdrawal parameter, or add a minimum_out parameter to limit slippage losses.

Assessed type

MEV

Table of contents

Low severity issues

Informational

Low

L01 MinimumPoolLiquidity can be violated in omnipool contract

Description

The add_liquidity function in the omnipool contract enforces a minimum deposit amount of the liquidity token as shown below.

ensure!(
        amount >= T::MinimumPoolLiquidity::get(),
        Error::<T>::InsufficientLiquidity
    );

However, there is no such check during removal of liquidity when calling the remove_liquidity function. Thus a user can add liquidity of 100 tokens (assume 100 > MinimumPoolLiquidity) and then remove 99 tokens, leaving 1 token in the pool which is less than the MinimumPoolLiquidity requirement.

Since the developer confirmed that the runtime forbids users from sending registered assets to the omnipool, this cannot be used to carry out an inflation attack, and is thus a low severity issue by itself.

Mitigation

Consider adding a check that the remaining amount of liquidity after removal is above the MinimumPoolLiquidity requirement.

L02 remove_token feature can be blocked easily

Description

The Omnipool contract has a remove_token feature that allows the owner to remove a token from the pool. However, this requires the protocol to own every share of the liquidity provided. Thus if any user has even a single share of liquidity with them, this feature cannot be used.

ensure!(
        asset_state.shares == asset_state.protocol_shares,
        Error::<T>::SharesRemaining
    );

It is quite impractical to expect every user in a decentralised system to sacrifice or remove their liquidity to allow the owner to remove a token. Users can just be offline with a small dust amount of liquidity and that would be enough to block this feature from being used.

Since there is no material impact on the pool as the asset can be frozen and the pool can continue to operate, this is a low severity issue.

Mitigation

Consider adding a force removal option that allows the owner to remove a token after a certain time period.

L03 Omnipool NFTs vulnerable in flashloanable protocols

Description

The Omnipool contract mints users position NFTs when they add liquidity to the pool. When they remove liquidity, the same NFT is modified instead of destroying and minting a new one. This can be a problem if the NFT is used in other defi protocols which allows flashloaning of NFTs.

If such a liquidity provision NFT ends up in such a defi protocol, any user can flashloan the NFT, take out all the liquidity from the pool, and then return the NFT back to the original owner. Since the tokenId remains the same, this is allowed by virtually all NFT handling protocols.

Mitigation

Consider burning and re-creating the NFT when a user removes liquidity. Otherwise, this will be an acceptable risk for the protocol.

L04 Missing InsufficientLiquidity check in omnipool sell

Description

The sell function in the omnipool contract does not check if it has enough asset_out tokens in the pool to pay out the user after the sale. This is present in the buy function, but is absent in the sell function.

// buy function
ensure!(asset_out_state.reserve >= amount, Error::<T>::InsufficientLiquidity);

This can lead to incorrect errors being thrown when the pool does not have enough tokens to pay out the user.

Mitigation

Consider adding this check before transferring the tokens to the user.

L05 LRNA tokens can still be bought from omnipool by manipulating price

Description

Buying of LRNA tokens from the omnipool is forbidden by the code. But LRNA tokens are still paid out to the liquidity provider if they remove liquidity at a higher price than they put in. This is because due to the increase in price there are fewer asset tokens to give out, and thus some LRNA tokens are given out to the user to compensate them.

So a user can add liquidity in asset_a, then buy up asset_a using asset_b. Then when they remove liquidity, they get back asset_a as well as LRNA tokens. Assuming 0 slippage, from the user's perspective, they have just swapped in asset_as for LRNA tokens.

This is not exploitable since the price ratios in the pool still remain the same, which would have been the same in a direct buy. However this is still possible while being explicitly blocked by the protocol.

Informational

I01 Inconsistent hooks call position

Ideally, hooks should be called at the very end of the function operation, after all the variables have been updated. This is because if the hooks try to query the contract, they should be accessing the latest updated values. While this is done in some functions, hooks are called before the end in certain functions.

List of functions where hooks are called before the end/update of storage variables:

• add_token in Omnipool (hook called before asset update)
• sell in Omnipool
• buy in Omnipool
• update_hdx_subpool_hub_asset in Omnipool (hook called before processing trade fee)
• sell_hub_asset
• buy_asset_for_hub_asset

I02 Incorrect Natspec

Incorrect/Missing/Inconsistent Natspec in certain areas of the code.

Missing input parameter documentation:

• https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/omnipool/src/lib.rs#L869-L875
• https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/omnipool/src/lib.rs#L1362-L1370
• https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/omnipool/src/lib.rs#L1428-L1438
• https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/stableswap/src/lib.rs#L847

Natspec order of input parameters different from the function's order of input parameters:

• https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/omnipool/src/lib.rs#L437-L440
• https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/omnipool/src/lib.rs#L1127-L1130

Incorrect Natspec:

• https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/stableswap/src/lib.rs#L399-L402

Natspec uses ' instead of `:

• https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/stableswap/src/lib.rs#L543-L544
• https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/stableswap/src/lib.rs#L630-L631

I03 InsufficientLiquidity check in buy function too low

The InsufficientLiquidity check in the omnipool buy function is present here. This is below the buy_asset_for_hub_asset call, and thus functions will revert with incorrect error messages, probably due to underflow in the math functions. Consider moving this up higher in the function. Also do the same for the sell function.

I04 Missing 0 amount check in withdraw_protocol_liquidity

The withdraw_protocol_liquidity function in omnipool does check if the admin is passing in 0 as the amount to withdraw. Consider adding this check.

I05 Unused pallet indices

The omnipool contract has some missing/unused pallet indices. pallet::call_index(0) is not used. pallet::call_index(10) is skipped over while both the indexes 9 and 11 are used.

• 1. System Overview
  • Stableswap
  • Omnipool
• 2. System Mechanics
  • Stableswap
  • Omnipool
    • A-B swaps
    • LRNA-A swaps
• 3. Threat Modelling
• 4. Centralization Risks
• 5. Further Security Advice
  • Stableswap
  • Omnipool

1. System Overview

HydraDx is an automated market maker in the Polkadot ecosystem and is deployed as a parachain designed to bring liquidity to the Polkadot ecosystem. For this purpose there are two separate AMMs in the scope of this audit: Stableswap and Omnipool.

Stableswap

Stableswap is a fork of the Curve protocol, re-written in rust and designed to be deployed on the Polkadot ecosystem. Similar to the curve protocol, it is a liquidity pool with variable slippage, and is better for assets with correlated values, such as stablecoins.

Omnipool

Omnipool is a new take on constant product market maker (CPMM) algorithms, and acts similar to the Uniswap protocol, but supports multiple tokens and allows single sided liquidity deposits. The aim of the system is to provide more liquidity between assets within a pool which have beend etermined to have similar risk factors/volatility. This way the system can provide liquidity to multiple pairs simultaneously without fracturing the liquidity.

The system is designed on substrate with pallets, with separate pallets responsible for the different AMMs, tokens as well as the runtimes. Using this architecture allows the protocol to finely control the intricacies of the blockchain environment itself.

In this audit, the following pallets are being Audited:

• stableswap
• omnipool
• math
• ema-oracle
• circuit-breaker

The first three pallets make up the two different AMMs described above. ema-oracle is an implementation of an exponential moving average oracle to evaluate time-weighted prices in the pool and act as an oracle. circuit-breaker is a pallet that allows the system to pause operations if the system goes outside the defined limits for trading, liquidity etc.

Here we will focus on the two AMMs for the most part, as they are the most complex and critical parts of the system.

2. System Mechanics

Stableswap

Stableswap is a curve protocol fork written in rust for the Polkadot ecosystem. Similar to curve, the stableswap AMM is basically a linear combination of a constant product market maker (CPMM) and a constant sum market maker (CSMM). The system is designed to be better for assets with correlated values, such as stablecoins. The amplification factor A determines the weight of the CPMM or the CSMM part, and thus determines the slippage of the pool.

Stableswap also implements some extra functions on top of the curve protocol, allowing liquidity addition in terms of shares and liquidity removal in terms of asset amount.

Stableswap removes some functionality from the curve protocol as well, mainly disallowing the ability to remove liquidity in multiple tokens at once. The protocol only allows for single asset removals. This creates an issue since single asset removals always changes the price in the pool and creates MEV opportunity, giving worse rates to the liquidity provider. This is because on curve, single sided liquidity addition or removal is equivalent to a trade where the single asset is traded for the other assets in the pool incurring some slippage and then adding liquidity. For removals the opposite is done. Thus there is a large swap involved, which creates MEV opportunity and is simply inefficient.

While the protocol supports multi-asset liquidity addition, it does not support multi-asset liquidity removal. This is a design decision, but it is important to note that this is a significant difference from the curve protocol and leads to inefficiencies.

Omnipool

The omnipool is a CPMM similar to uniswap or balancer, with the main difference being that the liquidity addition and removal happen in a single asset. This asset when deposited is paired with some LRNA tokens which are minted on demand at a specified price. So the liquidity pool holds multiple tokens, as well as LRNA or hub tokens which have been minted on demand and each token has an amount of hub tokens associated with it.

The immediate advantage of this design is that it allows for single sided liquidity operations, and increases the depth of liquidity in the pools. Instead of splitting liquidity of token A into an A-B pool and an A-C pool, thus leading to higher slippage, they can all be combined in a single A-B-C pool with the LRNA token acting as a bridge between the different assets.

A number of issues were discovered in this model, mainly relating to MEV and have been reported. In this section, I would like to present a spreadsheet based model for rough calculations of swaps in the omnipool, which can better help understand the core mechanics of the pool, as well as better explain some of the POCs in the submitted reports.

Lets assume tokens A and B are both priced at 1 usd, and the hub token LRNA is also priced at 1 usd. If we assume that the protocol starts with 20 A tokens and 20 B tokens, the protocol will mint 20 LRNA tokens corresponding to the 20 A tokens and similarly 20 LRNA tokens corresponding to the 10 B tokens. The initial liquidity composition of the pool will look like so:

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

Different swaps have different effects on the pool. Since buying LRNA tokens from the pool is not allowed, there are only two possible swaps: LRNA-A swaps and A-B swaps.

A-B swaps

In this scenario, Alice sells B token for A token. The price of A is raised. Now, no of A tokens and no of B tokens must maintain the invariant, while the corresponding hub token amounts remain untouched. Alice sells 5 B tokens, so the reserves grow to 25 B tokens and 16 A tokens.

Some observations:

1. pAB has gone down. Since A is being bought, the price of A has gone up wrt B.
2. pA has gone up while pB has gone down. This isbecause the LRNA tokens are untouched while one external token is devalued and the other is bought up. An important point to note, is that in A-B swaps, pA * pB is an invariant, neglecting fees. This is evident from this scenario, since 1.25*0.8=1.0. This is a direct product of the x*y=k invariant.
3. An arbitrage opportunity is created. Since pAB is not zero, it means the pool pays out more tokenA when tokenB is sold.

LRNA-A swaps

In this scenario, LRNA is sold for token A. Since this is a CPMM, it means LRNA corresponding to A and the number of token A will be related by the x*y=k invariant. Let's assume Alice sells 5 LRNA tokens and ignore any fees. Thus Pool LRNA for tokenA becomes 25. Similarly, pool's tokenA then becomes =20*20/25=16 following the invariant. So Alice receives 4 A tokens from the pool.

Some observations:

1. pAB has gone down. By buying up A, we have increased its worth wrt B. So the price of B has gone down in terms of A.
2. pA has gone up. This is because LRNA is sold and thus devalued.
3. An arbitrage opportunity is created. Since pAB is not zero, it means the pool pays out more tokenB when tokenA is sold.

Evaluating the arbitrage opportunity is quite simple. The arbitrager will sell token A and buy token B. As shown in point 2 in the A-B swap, in A-B swaps the pA*pB stays the same. For the final price pAB to equal 1, pA'=pB'. So the final prices pA' and pB' will be =sqrt(1.5625*1)=1.25. Since LRNA tokens remain untouched in A-B swaps, the arbitraged state is shown below. The return

These swap tables were used to simulate transactions on excel (sans fees) and is also used in the POCs of a couple submissions.

3. Threat Modelling

Mainly these threats were investigated in the audit:

1. Bugs / faulty math in the AMMs
2. Programming issues
3. Economic vulnerabilities
4. MEV / frontrunning / sandwich attacks

Some Bugs and programming issue were found, which have been reported.

Economic vulnerabilities were studied using the excel table method described above. This method removes fees from the calculations and is thus an optimistic scenario for value extraction.

The results who that if users are able to token->LRNA swaps, they can extract value from the pool. This is currently turned off in the system, but if it is turned on in the future for any reason, the algorithm needs to be thoroughly verified to prevent abuse of the system.

This method also showed that the ensure_price method in the code can be violated. This was an important finding since this allowed arbitrary amount of slippage losses to the user. This was discovered based on a few observations:

1. ensure_Price only looked at pA from the table above
2. pA increases in LRNA-A swaps, and decreases in A-B swaps

Further economic analyses were done to try and profit from the system, and situations where small profits can be gained risk-free have been reported.

For studying MEV, the same approach was used, but with two actors. The objective was to increase the value of the attacker while decreasing the value of the user or the pool. Multiple combinations of transactions were studied with the excel table method, and any results where the attacker gained value were reported.

This analysis was also done on admin-only functions, and some situations where even the admin could extract value from the pool were reported.

4. Centralization Risks

The system is permissionless. There are no black/whitelists involved, and admins cannot forbid any user from participating in the system.

However, the system is centralized. Tokens have individual permissions for removing and adding liquidity, as well as buying or selling. Centralization can be reduced if the various origins, like authority origin and technical origin are voting contracts with timelocks.

5. Further Security Advice

Stableswap

This pallet can benefit more from a thorough fuzz test. The pallet is pretty similar to the curve protocol, however there are some extra rounding operations here and there. An example can be seen here. There is an assumption here that the rounding is sufficient provided there's more than 2 wei of liquidity. The issue is that assumptions and small roundings like these are extremely difficult to validate manually. This is especially true in the curve protocol, there the solution does not come from a simple equation, but is actually converged upon by newton-raphson iterations. Thus the best way to validate these are either via formal verification (too expensive maybe) or via a robust and heavy fuzzing/invariant test suite. While there are proptests in the test suite with 1000 iterations, they are only good for checking the protocol under normal operations. These fuzz tests are insufficient to hit the edge cases which really trigger the rounding errors.

I am also unfamiliar with the robustness of rust fuzzers, but industry leading smart-fuzzers like medusa, echidna etc which try to optimise code coverage and edge cases are the way to go. The pallet can heavily benefit from a good fuzzing suite with such fuzzers which also do random operations on the state, and not just random inputs.

For this module, I would advise the devs to build some test cases specifically catered to hitting edge cases, like low liquidity conditions, or single/few wei operations. These are the conditions where the rounding errors are most likely to occur. Another option would be to maybe contact a team with robust fuzzing experince to build a fuzzing suite.

Omnipool

The omnipool math is easier to deal with, since its the same old x*y=k invariant. However, this module can also benefit from fuzz tests with random operations on the state. Since the math of this module is very close to uniswap, a battle-tested mechanism, this is less prone to breaking under such edge cases. The test suite in omnipool is too optimistic, which is good for normal operations, but does not test heavily skewed pool conditions with very extreme prices.

The test suite and the protocol was also built with the intention of stopping MEV bots from making a profit as the first priority. This is a good design, however, this is unfortunately not enough. If a user is able to cause harm to another user at a loss to themselves, that is still a valid issue since it impacts the second user. The protocol should be designed to prevent such situations as well. This is evident from missing slippage protections, and loose price checks, which will prevent attackers from profiting after considering the fees, but can still impact other users negatively.

For a robust system, the protocol should instead ensure that users or the pool cannot be harmed, or doesn't leak value. This will always ensure that attackers cannot profit, since the value has to come from somewhere.

Hours spent: 120

Time spent:

120 hours