Storage can be bloated with low value liquidity positions

[c4-bot-1]

Lines of code

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

Vulnerability details

Bug Description

When using the substrate framework, it is one of the main goals of developers to prevent storage bloat. If storage can easily be bloated by users, this can lead to high costs for the maintainers of the chain and a potential DOS. A more in detail explanation can be found here.

The Omnipool allows users to deposit liquidity to earn fees on swaps. Whenever a user deposits liquidity through add_liquidity(), he gets an NFT minted and the details of his deposit are stored in the Positions map.

let instance_id = Self::create_and_mint_position_instance(&position_owner)?;

<Positions<T>>::insert(instance_id, lp_position);

To ensure that this storage is only used for serious deposits, it is ensured to be above MinimumPoolLiquidity which is 1,000,000 tokens in the runtime configuration.

ensure!(
	amount >= T::MinimumPoolLiquidity::get() && amount > 0,
	Error::<T>::MissingBalance
);

Additionally, whenever a deposit gets fully withdrawn, the storage entry is removed.

if updated_position.shares == Balance::zero() {
	// All liquidity removed, remove position and burn NFT instance

	<Positions<T>>::remove(position_id);
	T::NFTHandler::burn(&T::NFTCollectionId::get(), &position_id, Some(&who))?;

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

Unfortunately, this implementation does not take into account that a malicious user can add MinimumPoolLiquidity tokens, and then instantly withdraw all but 1. In that case, he has incurred almost no cost for bloating the storage (besides the 1 token and gas fees) and can keep on doing this countless times.

Impact

The issue allows a malicious attacker to bloat the storage in a cheap way. If done often enough this allows him to DOS the functionality of the HydraDX protocol by bloating the storage significantly until it can't be maintained anymore. If the attacker uses a very low-value token, he only incurs the gas fee for each new entry.

If we consider that the intended cost for adding a new position entry (to potentially DOS) as defined by the MinimumPoolLiquidity should be 1_000_000 tokens, this issue allows an attacker to get the same storage bloat for 1/1_000_000 or 0.0001% of the intended cost.

Proof of Concept

The following testcase showcases the issue.

#[test]
fn remove_liquidity_but_one() {
	ExtBuilder::default()
		.with_endowed_accounts(vec![
			(Omnipool::protocol_account(), DAI, 1000 * ONE),
			(Omnipool::protocol_account(), HDX, NATIVE_AMOUNT),
			(LP2, 1_000, 2000 * ONE),
			(LP1, 1_000, 5000 * ONE),
		])
		.with_initial_pool(FixedU128::from_float(0.5), FixedU128::from(1))
		.with_token(1_000, FixedU128::from_float(0.65), LP2, 2000 * ONE)
		.build()
		.execute_with(|| {

			let liq_added = 1_000_000; //Exactly MinimumPoolLiquidity
			let current_position_id = <NextPositionId<Test>>::get();

			assert_ok!(Omnipool::add_liquidity(RuntimeOrigin::signed(LP1), 1_000, liq_added));

			let liq_removed = 200 * ONE;
			assert_ok!(Omnipool::remove_liquidity(
				RuntimeOrigin::signed(LP1),
				current_position_id,
				1_000_000-1 //Remove all but one asset
			));

			let position = Positions::<Test>::get(current_position_id).unwrap();
			let expected = Position::<Balance, AssetId> {
				asset_id: 1_000,
				amount: 1,
				shares: 1,
				price: (1300000000650000, 2000000001000000),
			};

			//There now is a position with 1 single Wei bloating the storage
			assert_eq!(position, expected);
		});
}

The testcase can be added to the pallets/omnipool/src/tests/remove_liquidity.rs file.

Tools Used

Manual Review

Recommended Mitigation Steps

The issue can be mitigated by not letting the amount in an open position fall below MinimumPoolLiquidity. This can be enforced as follows in the remove_liquidity() function.

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

Assessed type

DoS

[c4-pre-sort]

0xRobocop marked the issue as primary issue

[c4-pre-sort]

0xRobocop marked the issue as sufficient quality report

[c4-pre-sort]

0xRobocop marked the issue as high quality report

[c4-sponsor]

enthusiastmartin (sponsor) disputed

[enthusiastmartin]

This is publicly known issue, raised by our team:

galacticcouncil/hydration-node#674

[OpenCoreCH]

While the sponsor was already aware of the issue, it was not ruled out as a known issue in the contest description and can therefore be not deemed out of scope.

[c4-judge]

OpenCoreCH marked the issue as selected for report

[QiuhaoLi]

Hi @OpenCoreCH and @enthusiastmartin, thanks for the review. I have a question (not a dispute): Haven't we already limited the storage usage with gas fees (weight)? In omnipool/src/weights.rs:

	/// Storage: `Omnipool::Positions` (r:0 w:1)  <===
	/// Proof: `Omnipool::Positions` (`max_values`: None, `max_size`: Some(100), added: 2575, mode: `MaxEncodedLen`)
	fn add_liquidity() -> Weight {
		// Proof Size summary in bytes:
		//  Measured:  `3919`
		//  Estimated: `8739`
		// Minimum execution time: 220_969_000 picoseconds.
		Weight::from_parts(222_574_000, 8739)
			.saturating_add(T::DbWeight::get().reads(20))
			.saturating_add(T::DbWeight::get().writes(14)) // <===
	}

As we can see, the user will be charged the fees of storage writes for minting new positions. So if an attack tries to bloat the storage, it will suffer from the corresponding fees.

[J4X-98]

Hi @QiuhaoLi ,

The costs for a protocol on Polkadot consist of 2 kinds of costs. The computation costs are forwarded to the user using the weights and the storage costs, which have to be handled by the protocol themselves.

The attacker is correctly charged for the storage instruction (computation cost) but is able to force the protocol to incur the constant cost of maintaining the positions (storage cost). This storage cost should only be incurred by the protocol for serious positions, which is why they have set a minimum of 1 million tokens. From positions of that size, they can recoup their storage cost through other fees. As one can see in the issue this can be circumvented and the protocol will not be able to recoup the storage costs through fees on dust positions leading to a potential DOS. This can happen if the storage is flooded with dust positions, leading to massive storage costs that the protocol can not recoup through fees due to the insufficient size of each position.

As the sponsor has acknowledged this is a valid issue that they are trying to fix internally, so I don't see why this should be invalidated.

[QiuhaoLi]

@J4X-98 Hi, thanks a lot for the explanations! I once thought about the cost of positions and decided it has been charged as fees just like Ethereum storage, which seems wrong. As I said this is not a dispute, just a question, nice finding!