HydraDX - TheSchnilch's results

Lines of code

https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/omnipool/src/lib.rs#L1541-L1574 https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/omnipool/src/traits.rs#L164-L190 https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/ema-oracle/src/lib.rs#L558-L566

Vulnerability details

Summary

If an asset is removed from the omnipool, it is ensured that all data records in the omnipool are deleted and also all positions from liquidity providers. However, the data records in the Oracle are not reset. This means that if the asset is to be added again after some time and it then has a different price, the price in the Oracle is falsified.

POC

Here is a poc which can be inserted into the file “integration-tests/src/omnipool_init.rs” and that can be started with the following command: SKIP_WASM_BUILD=1 cargo test -p runtime-integration-tests poc -- --nocapture

#[test]
pub fn poc() {
	TestNet::reset();

	Hydra::execute_with(|| {
		let omnipool_account = hydradx_runtime::Omnipool::protocol_account();

		init_omnipool();

		let position_id_init = hydradx_runtime::Omnipool::next_position_id();
		assert_ok!(hydradx_runtime::Omnipool::add_token( //DOT token is added for the first time
			hydradx_runtime::RuntimeOrigin::root(),
			DOT,
			FixedU128::from_float(1.0),
			Permill::from_percent(100),
			AccountId::from(ALICE)
		));
		hydradx_run_to_next_block();

		assert_ok!(hydradx_runtime::Omnipool::sacrifice_position( //The shares from the initial liquidity are assigned to the protocol so that the token can be removed
			hydradx_runtime::RuntimeOrigin::signed(ALICE.into()),
			position_id_init
		));
		hydradx_run_to_next_block();

		assert_ok!(hydradx_runtime::Omnipool::set_asset_tradable_state( //The asset is frozen so that the token can be removed
			hydradx_runtime::RuntimeOrigin::root(),
			DOT,
			Tradability::FROZEN
		));

		assert_ok!(hydradx_runtime::Omnipool::remove_token( //This removes the token
			hydradx_runtime::RuntimeOrigin::root(),
			DOT,
			AccountId::from(ALICE)
		));

		//This is simply to skip a few blocks to show that some time has passed since the token was removed and the prices are still there
		hydradx_run_to_next_block();
		hydradx_run_to_next_block();
		hydradx_run_to_next_block();
		hydradx_run_to_next_block();
		hydradx_run_to_next_block();
		hydradx_run_to_next_block();
		hydradx_run_to_next_block();

		assert_ok!(hydradx_runtime::Tokens::transfer( //The initial liquidity is sent to the pool to add DOT the second time
			hydradx_runtime::RuntimeOrigin::signed(ALICE.into()),
			omnipool_account,
			DOT,
			500 * UNITS
		));

		assert_ok!(hydradx_runtime::Omnipool::add_token( //DOT is added the second time
			hydradx_runtime::RuntimeOrigin::root(),
			DOT,
			FixedU128::from_float(10.0), //The price has increased from 1.0 to 10.0
			Permill::from_percent(100),
			AccountId::from(ALICE)
		));
		hydradx_run_to_next_block();

		let return_value = hydradx_runtime::Omnipool::add_liquidity( //A liquidity provider tries to add liquidity
			hydradx_runtime::RuntimeOrigin::signed(ALICE.into()),
			DOT,
			1 * UNITS
		);
		println!("return_value: {:?}", return_value); //shows that adding liquidity didn't work because the price difference was too big
		hydradx_run_to_next_block();

		let (price_short, _) = hydradx_runtime::EmaOracle::get_price(LRNA, DOT, OraclePeriod::Short, OMNIPOOL_SOURCE).unwrap();
		println!("price_short: {:?}", price_short); //shows that the price for the period short is not 10, which it should actually be since the liquidity has not changed since DOT was added for the second time
	});
}

Description

If an asset is removed from the Omnipool, all of its data will be deleted from the Omnipool. However, the data from the asset remains in the Oracle and is not deleted there. The Oracle then continues to store the data of the removed asset in this StorageMap:

pub type Oracles<T: Config> = StorageNMap<
                _,
                (
                        NMapKey<Twox64Concat, Source>,
                        NMapKey<Twox64Concat, (AssetId, AssetId)>,
                        NMapKey<Twox64Concat, OraclePeriod>,
                ),
                (OracleEntry<BlockNumberFor<T>>, BlockNumberFor<T>),
                OptionQuery,
        >;

(https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/ema-oracle/src/lib.rs#L153-L162)

The price is stored here once for the last block and once with the exponential moving average logic for a few blocks in the past. (This is the short period)

The problem now is when an asset is added again and its price has changed. As a result, the new price is calculated using the exponential moving average logic with the entries of the blocks before the asset was removed and the new entries that have been added since the asset was re-added, which leads to an incorrect price. The wrong price can also cause ensure_price in add_liquidity to fail and therefore no liquidity can be added:

T::PriceBarrier::ensure_price(
			&who,
      T::HubAssetId::get(),
		  asset,
	    EmaPrice::new(asset_state.hub_reserve, asset_state.reserve),
)
.map_err(|_| Error::<T>::PriceDifferenceTooHigh)?;

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

So any protocol that uses this oracle as a price source would receive an incorrect price for the re-added asset for a short period of time.

Recommendation

Even if the price balances out again after some time (the length of the short period), an incorrect price is initially calculated after re-adding an asset for which the price has changed. I would recommend deleting the Oracle entries when removing an asset. This means that the price of the asset can be calculated correctly from the start when it is added again.

Assessed type

Oracle

Lines of code

https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/math/src/stableswap/math.rs#L173-L230 https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/math/src/stableswap/math.rs#L234-L314

Vulnerability details

Impact

When testing stableswap it turned out that if you use remove_liquidity_one_asset you get a little less than with withdraw_asset_amount if you use the same number of shares for both.

Proof of Concept

To prove this there is this POC that can be inserted into integration-tests/src/omnipool_init.rs.

#[test]
fn poc1() {
    TestNet::reset();

    Hydra::execute_with(|| {
		//Metadata is set for two tokens used in the stable pool so that the pool can use the decimals. In this case both assets have the same decimals
		assert_ok!(hydradx_runtime::AssetRegistry::set_metadata(hydradx_runtime::RuntimeOrigin::root(), DAI, vec![1,2], 12));
		assert_ok!(hydradx_runtime::AssetRegistry::set_metadata(hydradx_runtime::RuntimeOrigin::root(), DOT, vec![3,4], 12));

		//The stable pool is created with DAI and DOT as assets and ACA as share asset
        assert_ok!(hydradx_runtime::Stableswap::create_pool(
            hydradx_runtime::RuntimeOrigin::root(),
            ACA,
            vec![DAI, DOT],
            12,
            Permill::from_percent(1)
        ));

		//Initial liquidity is added
		assert_ok!(hydradx_runtime::Stableswap::add_liquidity(
			hydradx_runtime::RuntimeOrigin::signed(ALICE.into()),
			ACA,
			vec![AssetAmount{
					asset_id: DAI,
					amount: 1000 * UNITS
				},
			AssetAmount{
				asset_id: DOT,
				amount: 1000 * UNITS
			}
			]
		));

		println!("alice dai before: {}", hydradx_runtime::Tokens::free_balance(DAI, &AccountId::from(ALICE)));
		println!("alice aca before: {}", hydradx_runtime::Tokens::free_balance(ACA, &AccountId::from(ALICE)));
		/*assert_ok!(hydradx_runtime::Stableswap::withdraw_asset_amount( //liquidity is removed with withdraw_asset_amount
			hydradx_runtime::RuntimeOrigin::signed(ALICE.into()),
			ACA,
			DAI,
			100 * UNITS,
			10 * UNITS * UNITS
		));*/

		assert_ok!(hydradx_runtime::Stableswap::remove_liquidity_one_asset( //liquidity is removed with remove_liquidity_one_asset
			hydradx_runtime::RuntimeOrigin::signed(ALICE.into()),
			ACA,
			DAI,
			100702978470355663983, //number of shares needed from withdraw_asset_amount to get 100 DAI
			0
		));
		println!("alice dai after: {}", hydradx_runtime::Tokens::free_balance(DAI, &AccountId::from(ALICE)));
		println!("alice aca after: {}", hydradx_runtime::Tokens::free_balance(ACA, &AccountId::from(ALICE)));
    });
}

Description

Now you can run the POC with this command SKIP_WASM_BUILD=1 cargo test -p runtime-integration-tests stableswap_test -- --nocapture and see how many DAI Alice gets for how many shares based on the log in the terminal. To compare the two functions you can now comment out remove_liquidity_one_asset and execute withdraw_asset_amount and compare the logs in the terminal. You will notice that with the withdraw_asset_amount function, Alice gets a little more for the same number of shares as with remove_liquidity_one_asset.

This difference can be explained as follows: remove_liquidity_one_asset uses calculate_withdraw_one_asset to calculate how much of a reserve a user gets for a certain number of shares. withdraw_asset_amount uses calculate_shares_for_amount to calculate how many shares a user must pay to get a certain amount of reserve. Both functions have a part that calculates the fees:

calculate_withdraw_one_asset

for (idx, reserve) in xp_hp.iter().enumerate() {
		let dx_expected = if idx == asset_index {
			// dx_expected = xp[j] * d1 / d0 - new_y
			reserve.checked_mul(d1)?.checked_div(d_hp)?.checked_sub(y_hp)?
		} else {
			// dx_expected = xp[j] - xp[j] * d1 / d0
			reserve.checked_sub(reserve.checked_mul(d1)?.checked_div(d_hp)?)?
		};

		let expected = Balance::try_from(dx_expected).ok()?;
		let reduced = Balance::try_from(*reserve)
			.ok()?
			.checked_sub(fee.checked_mul_int(expected)?)?;

calculate_shares_for_amount

let adjusted_reserves: Vec<AssetReserve> = updated_reserves
		.iter()
		.enumerate()
		.map(|(idx, asset_reserve)| -> Option<AssetReserve> {
			let (initial_reserve, updated_reserve) = to_u256!(initial_reserves[idx].amount, asset_reserve.amount);
			let ideal_balance = d1.checked_mul(initial_reserve)?.checked_div(d0)?;
			let diff = Balance::try_from(updated_reserve.abs_diff(ideal_balance)).ok()?;
			let fee_amount = fee.checked_mul_int(diff)?;
			Some(AssetReserve::new(
				asset_reserve.amount.saturating_sub(fee_amount),
				asset_reserve.decimals,
			))
		})

Here you can see that for both functions a difference is calculated (for calculate_withdraw_one_asset it is called expected and for calculate_shares_for_amount it is called diff) which is then used to calculate the fees. The problem is that d1 is different for the two functions.

let d1 = d_hp.checked_sub(shares_hp.checked_mul(d_hp)?.checked_div(issuance_hp)?)?;

Here we calculate how many assets the pool still has after the amount for all shares that are burned has been removed. So in this case the fees have already been deducted from d1.

With calculate_shares_for_amount for amount, d1 is calculated by deducting the amount that the user gets at the end from the reserves, but the fees are not yet deducted:

let updated_reserves: Vec<AssetReserve> = initial_reserves
		.iter()
		.enumerate()
		.map(|(idx, v)| -> Option<AssetReserve> {
			if idx == asset_idx {
				Some(AssetReserve::new(v.amount.checked_sub(amount)?, v.decimals))
			} else {
				Some(*v)
			}
		})
		.collect::<Option<Vec<AssetReserve>>>()?;

	let initial_d = calculate_d::<D>(initial_reserves, amplification)?;
	let updated_d = calculate_d::<D>(&updated_reserves, amplification)?;
	let (d1, d0) = to_u256!(updated_d, initial_d);

Since the user gets more out of one function than the other, the same d1 should be used for both functions. So for calculate_withdraw_one_asset you should also use the d1 where the fees have not yet been deducted because they are calculated in this function.

Tools Used

VSCode

Assessed type

Math

Summary of the In-Scope part of the codebase

The code for this audit can be divided into two main parts: Omnipool, Stableswap, Oracle and Circuit Breaker

Omnipool

This is a pool into which all possible assets can be added by governance. Since it is a pool with all assets, there is a subpool in the omnipool for each asset, which consists of the asset and LRNA (hub token). Since all assets have LRNA as a second asset in their subpool, you can simply swap all assets by first swapping the asset that the user puts in into LRNA and then swapping this LRNA into the asset that should come out. This is done automatically by the sell and buy functions. After the state of the pool has been modified, a hook is called to update the oracle. In addition, the circuit breakers also check in these hooks that the amount traded does not exceed a limit that is measured per block.

• A brief overview of all external callable functions of the omnipool
  • add_token - This function is there so that governance can add a new token with initial liquidity to the pool
  • remove_token - With this function, governance can remove a token if all shares that still exist belong to the protocol
  • add_liquidity - With this function, every user can add liquidity to a specific token and receive shares in the form of an NFT
  • remove_liquidity - With this function, a liquidity provider can remove liquidity from his position
  • sell - Swap from one asset to another where the user specifies the amount of asset in
  • buy - Swap from one asset to another where the user specifies the amount of asset out
  • sacrifice_position - Liquidity providers can transfer their shares to the protocol by deleting their position and giving the shares to the protocol without receiving anything in return
  • set_asset_tradable_state - With this function, governance can determine the tradability of an asset
  • refund_refused_asset - If the initial liquidity has already been sent to the pool for a new asset and the asset was not added, the initial liquidity can be refunded using this function
  • set_asset_weight_cap - This function is for governance to set the weight cap for an asset
  • withdraw_protocol_liquidity - With this function the liquidity belonging to the protocol with the protocol shares of sacrifice_position can be withdrawn

Stableswap

Stableswap is also a pool that can hold 2-5 assets. However, these assets must be fixed when the pool is created and can no longer be changed. The pool is designed to contain only assets with the same price, e.g. DAI, USDC and USDT. However, several stable pools can be created. Hooks are also used here to update the oracle.

• A brief overview of all external callable functions of stableswap
  • create_pool - This allows governance to create a new pool with a few tokens that have the same price. The amplifiction and fees are also set
  • update_pool_fee - This allows governance to set a new fee value for a specific pool
  • update_amplification - With this function, governance can set a new value for amplification for a specific pool
  • add_liquidity - This allows a liquidity provider to add liquidity for one or more tokens by specifying how many of them they would like to provide. In return they then receive share assets
  • add_liquidity_shares - The same as add_liquidity only that the liquidity provider can only add tokens to a single asset and that he specifies the number of shares he would like to receive
  • remove_liquidity_one_asset - With this function, a liquidity provider can burn a certain number of shares and receive a part of the liquidity in return
  • withdraw_asset_amount - The same as remove_liquidity_one_asset but the liquidity provider does not specify the number of shares to burn but rather the number of assets that he would like to receive.
  • sell - This allows a user to swap a specific token for another by specifying the amount they want to sell
  • buy - This allows a user to swap one token for another by specifying the number of tokens they want to buy
  • set_asset_tradable_state - With this function, governance can determine the tradability of an asset

Circuit Breaker

The circuit breaker limits how much liquidity can be traded, added and removed within a block. This is done by storing all amounts that are added, removed and traded within a block and ensuring that they are under a certain limit. After a block these values are reset.

Oracle

The oracle receives the data through the hooks after liquidity has been added, removed or traded. This data is not saved directly and used by the oracle but only at the end of the block using exponential moving average logic. The oracle also stores the price for different time periods. So you should note that the price of Oracle is at least a block old.

Architecture and code analysis

• The code was built using substrate and the substrate-node-template, which already implements core features of a blockchain and is very configurable. In this case, it was used to build a parachain for Polkadot. You can find out more about it here: https://docs.substrate.io/, https://github.com/substrate-developer-hub/substrate-node-template
• The code is well structured by dividing important components such as omnipool, stableswap, ema-oracle and circuit-breaker into different pallets. There is also an extra crate for math.
• The code also looks well written and wasn't too difficult to read. This is also because larger functions are separated into different parts, e.g. Calculations were not done in the main function and the fact that the oracles were updated via hook is a good idea in my opinion
• The error handling in calculations could be improved. Often the error ArithmeticError::Overflow is simply returned, even if it is a completely different error. This makes testing more difficult because you have to find the right error yourself. I would recommend creating custom error messages for this.
• Events are always emitted at the end of the function and contain the important information for the offchain services.

Protocol Risks & Recommendations

• Even though the code already validates values in many places, there are still some places where this could still be improved:
  • remove_liquidity: I would recommend a check like in the other functions that the amount entered must be more than a minimum number of shares.
  • sell_hub_asset: Here, as in the sell function, there should also be a check that state_changes.asset.delta_reserve is greater than 0 to ensure that the user does not get no assets out even though he sells a few assets
  • calculate_add_liquidity_state_changes: There should be a check whether shares_hp is zero, since if this is the case the liquidity provider would add its tokens to the pool but would not receive any shares in return. This scenario can occur in the unlikely event that the initial liquidity provider, if whitelisted, removes all of its liquidity after the pool is created
• The circuit breakers protect against the liquidity in the omnipool changing too much within a block to protect the pool. It would be advisable to adapt this mechanism and then also protect the stableswap with it.
• In stableswap a function to remove a single token would be good. In a case where a token is depegged or decommissioned, it would be practical if governance could simply remove that token instead of having to set up the entire pool again without the one token.

Centralization & Special Roles

• The protocol is controlled by democratic governance (not in scope). This is mainly built with the substrate democracy module. All users that hold HDX can take part through referendums. These can be all sorts of proposals, including changes to the runtime code. Voting period of a referendum is 3 days and then an enactment period which lasts another 6 days. There is also an emergency referendum that can be used by the technical committee to make important changes to the code, such as fixing a security hole. The voting period is then only 3 hours. The technical commitee then consists of a few of the core developers of the protocol. This emergency mechanism is very good for reacting to security gaps or other events that require quick action. In a normal referedum, however, only one can be done in the voting period. All other referenda are in a queue at this time. Every 3 days, the referendum with the greatest support is moved into the voting period. This mechanism could be improved by making it possible for several referenda to be in the voting period at the same time. The protocol could be improved more quickly through the proposals. I wouldn't recommend putting all proposals into the voting period at the same time, but 3-4 should be good.

  Here is a list of functions from Omnipool that can be used by governance to set new parameters for the protocol and the impact these functions can have:

  • add_token - Impact: This means that governance has control over which new tokens are added. Here governance gets a lot of power because it can set the initial price. If this is wrong there can be quite large arbitrage opportunity. If weight_cap is set to 0 then it is not possible for liquidity to be added for this asset.
  • remove_token - Impact: This can be used by governance to remove a token from the pool when an asset is frozen and whatever liquidity is still there belongs to the protocol. So this shouldn't have a big impact on users.
  • set_asset_tradable_state - Impact: This function can only be used by the technical commitee. This allows an asset to be given different states so that liquidity can be removed, added, sold and bought. In addition, an asset can also be frozen, which means you can't do any of these things with the asset. So the technical commitee has the power to make an asset almost unusable if they freeze it.
  • refund_refused_asset - Impact: Since this can only be used to refund tokens of an asset that has not been added to the pool, this function has no impact on users of the pool or other parts of the protocol.
  • set_asset_weight_cap - Impact: If the weight cap is set incorrectly here, i.e. too low or 0, it can happen that it is only possible to add very little or no liquidity to an asset.
  • withdraw_protocol_liquidity - Impact: This function can have a big impact on the protocol and the liquidity provider, because if the price is set incorrectly, a lot more liquidity can be withdrawn from an asset than should actually be possible. Tokens are stolen from liquidity providers and the price of the asset is manipulated. This could be mitigated by storing the price that the protocol shares had when they were given to the protocol.

  Here is a list of functions from Stableswap that can be used by governance to set new parameters for the protocol and the impact these functions can have:

  • create_pool - Impact: Here the protocol has the option of creating a new pool with any tokens. Governance is responsible for ensuring that only tokens with the same price enter a pool. Amplification can only be set in a certain area, so it shouldn't have a very big impact. However, the fee can be set very high, which means that a pool becomes unusable, because a user would get almost nothing from a swap and would only pay fees.
  • update_pool_fee - Impact: As with create_pool, the fee can be set here without any validation, which can result in it being 100% and the pool becoming unusable and users only paying fees and getting nothing in return.
  • update_amplification - Impact: Here too, the amplification can only be set in a certain area and should not have a very large impact.
  • set_asset_tradable_state - Impact: This function has the same impact as the one in the Omnipool with the exception that not only the technical commitee is allowed to use this function.

  Even with the circuit breaker, governance has the power to restrict the Omnipool very much if the limits are very low. This can make the omnipool almost unusable.

Testing

• There are unit tests for all pallets that are in-scope and ensure that the functions do what they are supposed to.
• There are also integration tests for the omnipool, circuit-breaker and ema-oracle pallets, which test whether the pallets also work with all the others in runtime. I would also recommend adding integration tests for stableswap as this is a very important part of the protocol. Such tests can also help when the code is updated so that you can see directly whether the tests still work or whether an error was made during the update.

Code documentation and comments analysis

• The short explanations above the most important external callable functions in Omnipool and Stableswap were very helpful for getting a brief impression of what the function does. I would recommend doing this for other important functions such as the hooks and the calculation functions, which would make reading the code even easier.
• There is a README for all pallets that briefly explains what a pallet is for. This came in handy when I wanted to understand the meaning of a pallet without reading the entire code. (E.g. for pallets that are out of scope for this contest)
• Apart from the comments about the external callable functions, there weren't many others. For a few important sections within a function, it would be good if there were a few more comments.
• There were some very good explanations for the design of the Omnipool in the docs (https://docs.hydradx.io/omnipool_design). It would be good if there was a page like this in the docs for stableswap too.

Time spent:

25 hours