HydraDX - tsvetanovv's results

Lines of code

https://github.com/code-423n4/2024-02-hydradx/blob/main/HydraDX-node/pallets/omnipool/src/traits.rs#L49 https://github.com/code-423n4/2024-02-hydradx/blob/main/HydraDX-node/pallets/omnipool/src/lib.rs#L869-L910 https://github.com/code-423n4/2024-02-hydradx/blob/main/HydraDX-node/pallets/omnipool/src/lib.rs#L1527-L1575

Vulnerability details

Impact

As we can read from the protocol documentation, the calling of on_liquidity_changed hook after a liquidity change is very important. This is because the hook is used to update the oracle and in the circuit breaker. Systems that rely on on_liquidity_changed hooks to calculate or report on liquidity metrics may show inaccurate data since not all liquidity changes are being captured. Also hacker can exploit the absence of liquidity change tracking to manipulate market conditions.

Proof of Concept

on_liquidity_changed hook is used in Omnipool and is very important function that is called when liquidity is added or removed from the pool. It is very important to call it in certain operations because update on-chain oracle and the circuit breaker.

This hook is missing in the sacrifice_position() and remove_token() functions. remove_token() Removes token from Omnipool:

		#[pallet::call_index(12)]
		#[pallet::weight(<T as Config>::WeightInfo::remove_token())]
		#[transactional]
		pub fn remove_token(origin: OriginFor<T>, asset_id: T::AssetId, beneficiary: T::AccountId) -> DispatchResult {
			T::AuthorityOrigin::ensure_origin(origin)?;
			let asset_state = Self::load_asset_state(asset_id)?;

			// Allow only if no shares owned by LPs and asset is frozen.
			ensure!(asset_state.tradable == Tradability::FROZEN, Error::<T>::AssetNotFrozen);
			ensure!(
				asset_state.shares == asset_state.protocol_shares,
				Error::<T>::SharesRemaining
			);
			// Imbalance update
			let imbalance = <HubAssetImbalance<T>>::get();
			let hub_asset_liquidity = Self::get_hub_asset_balance_of_protocol_account();
			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)?;
			Self::update_imbalance(BalanceUpdate::Increase(delta_imbalance))?;

			T::Currency::withdraw(T::HubAssetId::get(), &Self::protocol_account(), asset_state.hub_reserve)?;
			T::Currency::transfer(asset_id, &Self::protocol_account(), &beneficiary, asset_state.reserve)?;
			<Assets<T>>::remove(asset_id);
			Self::deposit_event(Event::TokenRemoved {
				asset_id,
				amount: asset_state.reserve,
				hub_withdrawn: asset_state.hub_reserve,
			});
			Ok(())
		}
	}

sacrifice_position() destroys a position and position's shares become protocol's shares:

		#[pallet::call_index(4)]
		#[pallet::weight(<T as Config>::WeightInfo::sacrifice_position())]
		#[transactional]
		pub fn sacrifice_position(origin: OriginFor<T>, position_id: T::PositionItemId) -> DispatchResult {
			let who = ensure_signed(origin)?;

			let position = Positions::<T>::get(position_id).ok_or(Error::<T>::PositionNotFound)?;

			ensure!(
				T::NFTHandler::owner(&T::NFTCollectionId::get(), &position_id) == Some(who.clone()),
				Error::<T>::Forbidden
			);

			Assets::<T>::try_mutate(position.asset_id, |maybe_asset| -> DispatchResult {
				let asset_state = maybe_asset.as_mut().ok_or(Error::<T>::AssetNotFound)?;

				asset_state.protocol_shares = asset_state
					.protocol_shares
					.checked_add(position.shares)
					.ok_or(ArithmeticError::Overflow)?;

				Ok(())
			})?;

			// Destroy position and burn NFT
			<Positions<T>>::remove(position_id);
			T::NFTHandler::burn(&T::NFTCollectionId::get(), &position_id, Some(&who))?;

			Self::deposit_event(Event::PositionDestroyed {
				position_id,
				owner: who,
			});

			Ok(())
		}

It is very important to call on_liquidity_changed on these operations because it is used to update the oracle and also in the circuit breaker.

Tools Used

Visual Studio Code

Recommended Mitigation Steps

Call the on_liquidity_changed hook at the end of these functions.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

Missing deadline check

Proof of Concept

Few functions don't have deadline parameter. This parameter can provide the user an option to limit the execution of their pending transaction. Without a deadline parameter, users can execute their transactions at unexpected times when market conditions are unfavorable.

Function like do_add_liquidity(), do_add_liquidity_shares(), remove_liquidity_one_asset, withdraw_asset_amount(), sell() or buy() need to have deadline check. However, this is not a big problem in this case because the functions have slippage protection. Even though the users will get at least as much as they set, they may still be missing out on positive slippage if the exchange rate becomes favorable when the transaction is included in a block.

Similar report in code4rena and the explanation why this is medium even though it has slippage protection: https://github.com/code-423n4/2023-08-pooltogether-findings/issues/126#issuecomment-1678355315

Tools Used

Visual Studio Code

Recommended Mitigation Steps

Introduce a deadline parameter in these functions.

Assessed type

MEV