HydraDX - Franfran's results

Lines of code

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

Vulnerability details

Description

Some slippage checks are missing for liquidity operations both in the omnipool and the stableswap contracts. That means that all of these vulnerable liquidity operations are placing users at risk, who are implicitly interacting with the protocol with a 100% slippage. Interestingly enough, they are present for some of these operations in the stableswap, but not all.

In the stableswap, it's missing for the add_liquidity() function, but it's present for the do_add_liquidity_shares() function. In the omnipool, these functions are missing a slippage check:

• add_liquidity()
• remove_liquidity()

Note that Curve for instance, does have a slippage check for the add_liquidity function on their Stableswap implementation which this one from

Attack scenario

1. User A interacts with the Stableswap by calling add_liquidity(), the transaction settles in the mempool
2. There is a sudden price drop of one of the stable coins
3. User A transaction finally gets included right after the price drop and received a lot less shares than expected from the pool because of the price impact

Recommendation

Add a slippage check to make sure to capture the users intents and avoid any frontrunner or bad market condition to make the user receive less shares than expected.

This check would be analogous to the add_liquidity_shares() function slippage check which prevents sending too much assets given an expected amount of shares. But this time, we should expect a minimum amount of shares given a certain amount of assets.

Stableswap

diff --git a/pallets/stableswap/src/lib.rs b/pallets/stableswap/src/lib.rs
index e3bb3449..6a153146 100644
--- a/pallets/stableswap/src/lib.rs
+++ b/pallets/stableswap/src/lib.rs
@@ -486,10 +486,11 @@ pub mod pallet {
                        origin: OriginFor<T>,
                        pool_id: T::AssetId,
                        assets: Vec<AssetAmount<T::AssetId>>,
+                       min_shares_amount: Balance,
                ) -> DispatchResult {
                        let who = ensure_signed(origin)?;

-                       let shares = Self::do_add_liquidity(&who, pool_id, &assets)?;
+                       let shares = Self::do_add_liquidity(&who, pool_id, &assets, min_shares_amount)?;

diff --git a/pallets/stableswap/src/lib.rs b/pallets/stableswap/src/lib.rs
index f0c6625d..cd0d0604 100644
--- a/pallets/stableswap/src/lib.rs
+++ b/pallets/stableswap/src/lib.rs
@@ -1010,6 +1010,7 @@ impl<T: Config> Pallet<T> {
                who: &T::AccountId,
                pool_id: T::AssetId,
                assets: &[AssetAmount<T::AssetId>],
+               min_shares_amount: Balance,
        ) -> Result<Balance, DispatchError> {
                let pool = Pools::<T>::get(pool_id).ok_or(Error::<T>::PoolNotFound)?;
                ensure!(assets.len() <= pool.assets.len(), Error::<T>::MaxAssetsExceeded);
@@ -1080,6 +1081,8 @@ impl<T: Config> Pallet<T> {
                )
                .ok_or(ArithmeticError::Overflow)?;

+               ensure!(share_amount >= min_shares_amount, Error::<T>::SlippageLimit);
+
                ensure!(!share_amount.is_zero(), Error::<T>::InvalidAssetAmount);
                let current_share_balance = T::Currency::free_balance(pool_id, who);

Omnipool

diff --git a/pallets/omnipool/src/lib.rs b/pallets/omnipool/src/lib.rs
index 1b40feff..251b2f31 100644
--- a/pallets/omnipool/src/lib.rs
+++ b/pallets/omnipool/src/lib.rs
@@ -578,7 +578,12 @@ pub mod pallet {
 			.saturating_add(T::ExternalPriceOracle::get_price_weight()))
 		)]
 		#[transactional]
-		pub fn add_liquidity(origin: OriginFor<T>, asset: T::AssetId, amount: Balance) -> DispatchResult {
+		pub fn add_liquidity(
+			origin: OriginFor<T>,
+			asset: T::AssetId,
+			amount: Balance,
+			min_shares_amount: Balance,
+		) -> DispatchResult {
 			let who = ensure_signed(origin.clone())?;
 
 			ensure!(
@@ -630,6 +635,11 @@ pub mod pallet {
 				.delta_update(&state_changes.asset)
 				.ok_or(ArithmeticError::Overflow)?;
 
+			ensure!(
+				state_changes.asset.delta_shares >= min_shares_amount,
+				Error::<T>::SlippageLimit
+			);
+
 			let hub_reserve_ratio = FixedU128::checked_from_rational(
 				new_asset_state.hub_reserve,
 				T::Currency::free_balance(T::HubAssetId::get(), &Self::protocol_account())

diff --git a/pallets/omnipool/src/lib.rs b/pallets/omnipool/src/lib.rs
index 1b40feff..adef88d9 100644
--- a/pallets/omnipool/src/lib.rs
+++ b/pallets/omnipool/src/lib.rs
@@ -722,6 +722,7 @@ pub mod pallet {
 			origin: OriginFor<T>,
 			position_id: T::PositionItemId,
 			amount: Balance,
+			max_shares_amount: Balance,
 		) -> DispatchResult {
 			let who = ensure_signed(origin.clone())?;
 
@@ -788,6 +789,11 @@ pub mod pallet {
 			)
 			.ok_or(ArithmeticError::Overflow)?;
 
+			ensure!(
+				state_changes.asset.delta_shares <= max_shares_amount,
+				Error::<T>::SlippageLimit
+			);
+
 			let new_asset_state = asset_state
 				.clone()
 				.delta_update(&state_changes.asset)

Assessed type

MEV

[Low 1] normalize_value cannot round up when normalizing with more decimals, leading to correctness issues

The function normalize_value is used for the reserves to be compatible with the internal math of the protocol, since it has been intended to be working for unsigned integers of a precision of 18 decimals. In a lot of places, it is used for rounding up, but you should note that it is not rounding as intended if the target decimals are greater than the decimals and that the intention is to round up. The reason is that the implementation of the function which uses the saturating_mul which rounds down by default. This correctness error only happens if $decimals_{target} > decimals$ though and that the intention of the caller is to round UP.

See the implementation of the function here

pub(crate) fn normalize_value(amount: Balance, decimals: u8, target_decimals: u8, rounding: Rounding) -> Balance {
	if target_decimals == decimals {
		return amount;
	}
	let diff = target_decimals.abs_diff(decimals);
	if target_decimals > decimals {
		amount.saturating_mul(10u128.saturating_pow(diff as u32))
	} else {
		match rounding {
			Rounding::Down => amount.div(10u128.saturating_pow(diff as u32)),
			Rounding::Up => amount
				.div(10u128.saturating_pow(diff as u32))
				.saturating_add(Balance::one()),
		}
	}
}

This, though, doesn't appear to be severe enough to be exploited. For this to have serious consequences, we would either need some insanely extreme market conditions, or specific pool configurations such as very few liquidity in the pool. But because fees are charged when interacting with the protocol, they swallow this imprecision. It would be a good choice to make sure that the multiplication result is ceiling if the intent is to round UP, though.

[Low 2] Fee calculations are sometimes rounded towards the wrong way

Fees should profit the protocol and be rounded up to charge the user when normalized. In multiple places, the fee calculated from the amount moved in / out of the pool in rounded down and this will cause events to emit with a fee that is a little bit inferior to the expected value in some cases when the precision adding is applied.

[Low 3] No deadline parameters for swap operations

Uniswap is an exchange which popularized the "deadline" parameter. This parameter has been created to avoid people to do bad trades by having them settling in the mempool and once that the gas price become such that miners would pick up this transaction, then the price of the asset would change price and don't capture the intent of the trader anymore. It may be a good addition to the Omnipool and the Stableswap to avoid these edge cases from happening when the mempool is extremely solicited.

[Low 4] Cargo audit

The usage of cargo audit has found 3 vulnerabilities as well as 6 warnings on currently utilized crates. You can find the full output here Here is a breakdown of the issues found.

You may temporarily replace these crates if it can be done or update if a fix has already been provided. In the worst case, make sure that none of these vulnerabilities can be triggered from the project.

[Info 1] Suggested refactors

diff --git a/math/src/stableswap/math.rs b/math/src/stableswap/math.rs
index 171845b8..e1ca5771 100644
--- a/math/src/stableswap/math.rs
+++ b/math/src/stableswap/math.rs
@@ -733,7 +733,7 @@ pub fn calculate_share_price<const D: u8>(
                let num = num.checked_div(p_diff)?;
                (num, denom)
        };
-       let (num, denom) = round_to_rational((num, denom), crate::support::rational::Rounding::Down);
+       let (num, denom) = round_to_rational((num, denom), Rounding::Down);
        //dbg!(FixedU128::checked_from_rational(num, denom));
        Some((num, denom))
 }

diff --git a/pallets/stableswap/src/lib.rs b/pallets/stableswap/src/lib.rs
index bfb32f65..afae35f7 100644
--- a/pallets/stableswap/src/lib.rs
+++ b/pallets/stableswap/src/lib.rs
@@ -1224,19 +1224,14 @@ impl<T: Config> Pallet<T> {
                                amount: reserve,
                                decimals,
                        });
-                       if let Some(liq_added) = added_assets.remove(pool_asset) {
-                               let inc_reserve = reserve.checked_add(liq_added).ok_or(ArithmeticError::Overflow)?;
-                               updated_reserves.push(AssetReserve {
-                                       amount: inc_reserve,
-                                       decimals,
-                               });
+                       let amount = if let Some(liq_added) = added_assets.remove(pool_asset) {
+                               reserve.checked_add(liq_added).ok_or(ArithmeticError::Overflow)?
                        } else {
                                ensure!(!reserve.is_zero(), Error::<T>::InvalidInitialLiquidity);
-                               updated_reserves.push(AssetReserve {
-                                       amount: reserve,
-                                       decimals,
-                               });
-                       }
+                               reserve
+                       };
+
+                       updated_reserves.push(AssetReserve { amount, decimals });
                }

Note that there is a very similar case in the calculate_shares() function of the same file (pallets/stableswap/src/lib.rs).

[Info 2] Untested yet critical operations

omnipool/src/lib.rs

omnipool/src/traits.rs

math/src/omnipool/math.rs

stableswap/src/lib.rs

math/src/stableswap/math.rs

math/src/ema/math.rs

[Info 3] Dead code

math/src/omnipool/math.rs

[Info 4] Misleading variable name change

The function calculate_delta_imbalance is called in two places in the omnipool pallet in the file pallets/omnipool/src/lib.rs. Here are those occurences

			let delta_imbalance = hydra_dx_math::omnipool::calculate_delta_imbalance(
				hub_reserve,
				I129 {
					value: current_imbalance.value,
					negative: current_imbalance.negative,
				},
				current_hub_asset_liquidity,
			)
			.ok_or(ArithmeticError::Overflow)?;

			let delta_imbalance = hydra_dx_math::omnipool::calculate_delta_imbalance(
				asset_state.hub_reserve,
				I129 {
					value: imbalance.value,
					negative: imbalance.negative,
				},
				hub_asset_liquidity,
			)
			.ok_or(ArithmeticError::Overflow)?;

The hub_reserve is misleading and should probably be renamed to delta_hub_reserve since it looks like the variables hub_reserve and current_hub_asset_liquidity have been swapped in the calculate_delta_imbalance function

pub fn calculate_delta_imbalance(
	delta_hub_reserve: Balance,
	imbalance: I129<Balance>,
	hub_reserve: Balance,
) -> Option<Balance> {

The calculation, though is correct, since it conforms to the specs. See the formula in the "Add Tokens" operation

Summary

HydraDX is a DEFI protocol on the Polkadot blockchain which promises to solve many issues of the current protocols such as Impermanent loss, expensive interactions, high slippage, and security at the protocol level by leveraging Polkadot parachains. Parachains can be thought as specialized blockchains that can be deployed and are validated by the consensus layer of Polkadot, namely the relay chain. They allow an unparalleled freedom to developers because applications are not limited anymore by the rules that have been defined by the application layer and can now go out-of-the sandbox and define their own rules. But with great powers comes great responsibilities: the halting problem is one issue that may compromise the security of the parachain, since they define their own weights (which is analogous to the gas on Ethereum). If these rules are not correctly defined or that the state bloat is not controlled, then the chain may quickly either become unusable or vulnerable to DOS attacks. Smart contract are written by the use of contract pallets, which is an utility for writing smart contract in Rust offered by Substrate.

Approach

• Getting more familiar with the docs of Substrate https://docs.substrate.io/reference/rust-api/ as well as existing tools from the Polkadot ecosystem such as https://assethub-polkadot.subscan.io/ to explore and know more about other parachains, which are crucial for understanding how nodes work
• Learning more about HydraDX though their docs https://docs.hydradx.io/
• Setup of tools that will be useful to dive into the code such as rust-analyzer as well as doing specific operations. Launching of cargo audit, cargo test and use of tarpaulin in order to get the coverage of tests
• Line-by-line manual review of the 4 systems in scope: Omnipool, Stableswap, EMA oracle, and the circuit breaker while looking at invariants and the mathematical models expressed by past audit reports
• Diff of each part of the code from the last commit of each audits fixes to highlight new, unaudited changes. git blame is useful to find culprit commits and their underlying PRs

Cargo audit

The usage of cargo audit has found 3 vulnerabilities as well as 6 warnings on currently utilized crates. You can find the full output here Here is a breakdown of the issues found.

You may temporarily replace these crates if it can be done or update if a fix has already been provided. In the worst case, make sure that none of these vulnerabilities can be triggered from the project.

Test coverage

cargo tarpaulin --tests -t 9999 --out lcov

Note that this is a very long process !

With the output

59.33% coverage, 8851/14917 lines covered

# convert the lcov report to html
genhtml lcov.info -o coverage

Also note that these results may not be accurate, since the coverage for macros cannot be done without instrumentation of the code.

Nonetheless, a report has been uploaded on github

The above command may be specialized to the audit scope in order to cut a bit on the generation time

cargo tarpaulin -t 9999 --out lcov -p pallet-circuit-breaker -p pallet-omnipool -p pallet-stableswap -p pallet-ema-oracle -p hydra-dx-math

By looking at the coverage, we can see that most conversion trait implementations are untested. If these branches are never used, you should consider removing them or adding a simple unit test or completely ignoring coverage for these functions by the use of attributes such as #[cfg(not(tarpaulin_include))] or #[no_coverage]. For more information, see Ignoring code in files.

Risks

Pallets are by nature upgradeable without the need of a hard fork. Upgrades actioned by the council are democratic since people can vote on opengov, but they may vastly change the logic of pallets. Great care has to be taken when reviewing these changes.

Here is a list of potentially risky operations that can be executed by the privileged parties

Adding a new token to the Omnipool / Create a stableswap pool

Role

Authority

Risk

Only the authority is able to add tokens to the pool. This is to make sure that malicious tokens cannot be added by anyone to steal shares from the protocol because the price of the asset is arbitrarily set that the time of the operation. Having a democratic governance role behind this operation is a nice mitigation of this, so that people have the time to do their due diligence before that a new token is added.

Note that there is a caveat here, which is that if the initial price may have changed since the time of the proposal and its execution, which is very likely, then first buyers of the token in the omnipool could arbitrage it and effectively steal tokens until that the price has been equalized with other markets.

Recommendation

Use custom referendums to add new tokens or create a new pool. This way, you can define a range of the price that people would be agreeing on in order to address the caveat which is a timing issue. That way, the authority who could execute the proposal could set a price strictly defined in that range and minimize cold-liquidity losses. If you can use an oracle, it's even better.

Change the state of the tradability of assets, including freezing the swap, or liquidity managing operations

Role

Technical

Risk

The "tradability" which is defined for both the Omnipool and the Stableswap has 5 different states represented as a bitmap. This flag is used to have a more fine-grained control about what are the allowed operations on the system. When a "state" is created, the flags are the most permissive; Everything is turned on. There is a rather important risk here though. For instance, if the REMOVE_LIQUIDITY flag is turned off, people won't be able to remove their liquidity from pools, and this could create panic within liquidity providers. If trading is still allowed, they would probably swap their tokens in order to save the most value and the price of it would drop significantly. There is a very sensible use of this flag though in the case of the removal of tokens, which requires the asset to be completely frozen and the pool to own 100% of the liquidity of this token.

Recommendation

Ban the REMOVE_LIQUIDITY flag to guarantee that people will always be able to remove their assets from the pool. Alternatively, make sure that a relatively low amount of liquidity is in the pool for this flag to be allowed turned off.

Notes

Most of the other important state changes are already sufficiently mitigated by the fact that the roles with a lot of powers are behind a governance. The time it takes for a proposal to be in an executable state (which is currently 28 days) makes is safe enough to give time to people to move their funds if they wish to. In general, HydraDX is very good at following a trustless protocol model.

Diffs

For the diffs, here are the relevant commits that were used as a starting point to compare the changes added since that date.

• The omnipool had two public audits. The one from Runtime Verification was last, so it supersedes the one from Blockscience.
• The circuit breaker is not in the list simply because it has, to my knowledge, not been audited before.

Additional references

https://wiki.polkadot.network/docs/build-smart-contracts

Time spent:

35 hours