MinTradingLimit check can be bypassed in do_add_liquidity_shares()

[c4-bot-6]

Lines of code

https://github.com/code-423n4/2024-02-hydradx/blob/main/HydraDX-node/pallets/stableswap/src/lib.rs#L1082-L1135
https://github.com/code-423n4/2024-02-hydradx/blob/main/HydraDX-node/pallets/stableswap/src/lib.rs#L1008-L1011
https://github.com/code-423n4/2024-02-hydradx/blob/main/HydraDX-node/pallets/stableswap/src/lib.rs#L155-L157

Vulnerability details

Impact

An important check(MinTradingLimit) can be bypassed.

Proof of Concept

The function do_add_liquidity_shares() allow a liquidity provider to add liquidity to a selected pool and receive a precise amount of shares in return. The difference between this function and do_add_liquidity() is that do_add_liquidity_shares() user can specify exact amount of shares to receive.

But do_add_liquidity_shares() missing MinTradingLimit check:

        /// Minimum trading amount
        #[pallet::constant]
        type MinTradingLimit: Get<Balance>;

We can see that this check has it in do_add_liquidity():

1008:        ensure!(
1009:           asset.amount >= T::MinTradingLimit::get(), 
1010:           Error::<T>::InsufficientTradingAmount
1011:            );

But this check completely misses in do_add_liquidity_shares(). This allows a user to bypass this important check and add liquidity below the minimum asset amount allowed by the protocol.

Tools Used

Visual Studio Code

Recommended Mitigation Steps

Add a check for MinTradingLimit as you do in do_add_liquidity().

Assessed type

Invalid Validation

[0xRobocop]

There is a check for a minimum amount of shares. Leaving for sponsor review.

[c4-pre-sort]

0xRobocop marked the issue as sufficient quality report

[c4-pre-sort]

0xRobocop marked the issue as primary issue

[c4-sponsor]

enthusiastmartin (sponsor) confirmed

[c4-sponsor]

enthusiastmartin marked the issue as disagree with severity

[enthusiastmartin]

MinTradingLimit is there to protect users due to rounding issues..

It is not any risk to protocol. Definitely not Medium.

[OpenCoreCH]

This would require an active user mistake that would not hurt the protocol in any way. As per the SC verdict "conditional on user mistake", downgrading to QA

[c4-judge]

OpenCoreCH changed the severity to QA (Quality Assurance)

[c4-judge]

OpenCoreCH marked the issue as grade-b