c4-bot-9 commented 6 months ago

Lines of code

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

Vulnerability details

Impact

By front-running the add_token() with a direct transfer to the pool account, an attacker can break the weight cap restriction.

Proof of Concept

To defend against some liquidity attacks, every asset in the omnipool has a weight cap that can't be broken when liquidities are adding to the pool. However, when the authorities add tokens that provide the initiative liquidity, we don't check whether the weight cap is broken. This is correct and fine for the first added token since there are no other tokens in the pool, but for subsequent tokens, this can lead to:

The authority calls add_token with initial liquidity.
Attacker calls transfer with higher tip fees to front-running and transfer tokens directly to the pool.
Since there is no check for the cap weight in add_token, the token pair is created and the weight cap is broken.

Tools Used

Manual Review.

Recommended Mitigation Steps

In add_token, if Assets is not empty (not the first added token), we should make sure the weight cap is not broken just like we do in add_liquidity:

ensure!(
        hub_reserve_ratio <= state.weight_cap(),
        Error::<T>::AssetWeightCapExceeded
);

Assessed type

Context

c4-pre-sort commented 6 months ago

0xRobocop marked the issue as sufficient quality report

c4-pre-sort commented 6 months ago

0xRobocop marked the issue as primary issue

0xRobocop commented 6 months ago

Related to #188

c4-sponsor commented 6 months ago

enthusiastmartin (sponsor) disputed

enthusiastmartin commented 6 months ago

This is incorrect.

add_token is designed to add new token to the pool, with any liquidity volume. It does not need to respect the chosen weight cap for that asset.

OpenCoreCH commented 6 months ago

While add_token does not check the weight caps, this function is permissioned. This means that AuthorityOrigin does not need to respect the defined soft limits, which is a valid design decision. Therefore downgrading to QA.

c4-judge commented 6 months ago

OpenCoreCH changed the severity to QA (Quality Assurance)