code-423n4 / 2024-02-hydradx-findings

1 stars 0 forks source link

stableswap: add_liquidity should support minimal received shares protection #189

Closed c4-bot-8 closed 6 months ago

c4-bot-8 commented 6 months ago

Lines of code

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

Vulnerability details

Impact

LP may receive fewer shares than expected because of changed fees.

Proof of Concept

In the stable swap pallet, we apply fees on the different pools which depend on the pool.fee value and the number of coins.

/// Calculate amount of shares to be given to LP after LP provided liquidity of some assets to the pool.
pub fn calculate_shares<const D: u8>(
    initial_reserves: &[AssetReserve],
    updated_reserves: &[AssetReserve],
    amplification: Balance,
    share_issuance: Balance,
    fee: Permill,
) -> Option<Balance> {
    // ...
    let fixed_fee = FixedU128::from(fee);
    let fee = fixed_fee
        .checked_mul(&FixedU128::from(n_coins as u128))?
        .checked_div(&FixedU128::from(4 * (n_coins - 1) as u128))?;
// ...
        let amplification = Self::get_amplification(&pool);
        let share_issuance = T::Currency::total_issuance(pool_id);
        let share_amount = hydra_dx_math::stableswap::calculate_shares::<D_ITERATIONS>(
            &initial_reserves,
            &updated_reserves,
            amplification,
            share_issuance,
            pool.fee,
        )

The fee is taken into account when the shares are calculated later, so when the fee increases, the LP receives fewer shares. Also, the current amplification has impacts on the D which is the liquidity value (shares). However, there is no (slippage) protection for LP, so if an LP calls add_liquidity and there is a AuthorityOrigin update amplification or fee, the LP may receive fewer shares than expected.

Tools Used

Manual Review.

Recommended Mitigation Steps

Add a minimal shares parameter to add_liquidity function.

Assessed type

Uniswap

c4-pre-sort commented 6 months ago

0xRobocop marked the issue as duplicate of #158

c4-judge commented 6 months ago

OpenCoreCH marked the issue as satisfactory