loss of funds for liquidity providers due to that add_liquidity_shares function takes more assets than what add_liquidity function takes to mint the same amount of shares for the liquidity provider .

[c4-bot-6]

Lines of code

https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/stableswap/src/lib.rs#L512-L529 https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/stableswap/src/lib.rs#L475-L492 https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/math/src/stableswap/math.rs#L386-L392

Vulnerability details

Impact

This vulnerability will lead to loss of funds for liquidity providers , since the function add_liquidity_shares takes more assets than the function add_liquidity takes for the same amount of shares . This will affect the liquidity providers who will try to provide_liquidity by using add_liquidity_shares

Proof of Concept

this vulnerability arises from the fact that add_liquidity_shares takes more assets than what add_liquidity function takes for the same amount of shares , which is considered loss of funds for the liquidity providers .

the function calculate_add_one_asset calculates the amount of assets to be taken from the user in order to receive a specific amount of shares , this function returns more assets than what it supposed to be .

coded POC :

consider adding those two test to add_liquidity.rs test file here , and see the printed statements that resulted from the test

#[test]
fn add_liquidity_should_work_correctly_when_fee_is_applied_test() {
    let asset_a: AssetId = 1;
    let asset_b: AssetId = 2;
    let asset_c: AssetId = 3;

    ExtBuilder::default()
        .with_endowed_accounts(vec![
            (BOB, asset_a, 2_000_000_000_000_000_000),
            (ALICE, asset_a, 52425995641788588073263117),
            (ALICE, asset_b, 52033213790329),
            (ALICE, asset_c, 119135337044269),
        ])
        .with_registered_asset("one".as_bytes().to_vec(), asset_a, 18)
        .with_registered_asset("two".as_bytes().to_vec(), asset_b, 6)
        .with_registered_asset("three".as_bytes().to_vec(), asset_c, 6)
        .with_pool(
            ALICE,
            PoolInfo::<AssetId, u64> {
                assets: vec![asset_a, asset_b, asset_c].try_into().unwrap(),
                initial_amplification: NonZeroU16::new(2000).unwrap(),
                final_amplification: NonZeroU16::new(2000).unwrap(),
                initial_block: 0,
                final_block: 0,
                fee: Permill::from_float(0.0001),
            },
            InitialLiquidity {
                account: ALICE,
                assets: vec![
                    AssetAmount::new(asset_a, 52425995641788588073263117),
                    AssetAmount::new(asset_b, 52033213790329),
                    AssetAmount::new(asset_c, 119135337044269),
                ],
            },
        )
        .build()
        .execute_with(|| {
            let pool_id = get_pool_id_at(0);
            let amount = 2_000_000_000_000_000_000;
            Tokens::withdraw(pool_id, &ALICE, 5906657405945079804575283).unwrap();
            assert_ok!(Stableswap::add_liquidity(
                RuntimeOrigin::signed(BOB),
                pool_id,
                vec![AssetAmount::new(asset_a, amount),]
            ));
            let received = Tokens::free_balance(pool_id, &BOB);
            println!("shares received after providing 2_000_000_000_000_000_000 asset and call add_liquidity function : {:?}", received);
            // add_liquidity()
            // 2_000_000_000_000_000_000 asset , and gives 1_947_487_201_901_031_408 shares
        });
}
#[test]

fn add_liquidity_should_apply_fee_when_providing_exact_amount_of_shares_test() {
    let asset_a: AssetId = 1;
    let asset_b: AssetId = 2;
    let asset_c: AssetId = 3;

    ExtBuilder::default()
        .with_endowed_accounts(vec![
            (BOB, asset_a, 3_000_000_000_000_000_000),
            (ALICE, asset_a, 52425995641788588073263117),
            (ALICE, asset_b, 52033213790329),
            (ALICE, asset_c, 119135337044269),
        ])
        .with_registered_asset("one".as_bytes().to_vec(), asset_a, 18)
        .with_registered_asset("two".as_bytes().to_vec(), asset_b, 6)
        .with_registered_asset("three".as_bytes().to_vec(), asset_c, 6)
        .with_pool(
            ALICE,
            PoolInfo::<AssetId, u64> {
                assets: vec![asset_a, asset_b, asset_c].try_into().unwrap(),
                initial_amplification: NonZeroU16::new(2000).unwrap(),
                final_amplification: NonZeroU16::new(2000).unwrap(),
                initial_block: 0,
                final_block: 0,
                fee: Permill::from_float(0.0001),
            },
            InitialLiquidity {
                account: ALICE,
                assets: vec![
                    AssetAmount::new(asset_a, 52425995641788588073263117),
                    AssetAmount::new(asset_b, 52033213790329),
                    AssetAmount::new(asset_c, 119135337044269),
                ],
            },
        )
        .build()
        .execute_with(|| {
            let pool_id = get_pool_id_at(0);
            Tokens::withdraw(pool_id, &ALICE, 5906657405945079804575283).unwrap();
            assert_ok!(Stableswap::add_liquidity_shares(
                RuntimeOrigin::signed(BOB),
                pool_id,
                1947487201901031408,
                asset_a,
                4_000_000_000_000_000_000,
            ));
            let received = Tokens::free_balance(pool_id, &BOB);
            assert_eq!(received, 1947487201901031408);

            let used = 3_000_000_000_000_000_000 - Tokens::free_balance(asset_a, &BOB);
            // add_liquidity_shares()
            // amount used of token a to get the 1947487201901031408 shares  are 2_000_001_423_231_920_170
            println!("amount of assets taken from the user in order to receive 1947487201901031408 shares :   {:?}", used);
                        println!("extra amount of assets taken from the liquidity provider to provide liquidity by the add_liquidity_shares : {:?}", used - 2_000_000_000_000_000_000);

        });
}

the logs

shares received after providing 2_000_000_000_000_000_000 asset and call add_liquidity function : 1947487201901031408

amount of assets taken from the user in order to receive 1947487201901031408 shares :   2000001423231920170
extra amount of assets taken from the liquidity provider to provide liquidity by the add_liquidity_shares : 1423231920170

this POC demonstrates that the function add_liquidity_shares takes 1423231920170 extra amount of assets from the liquidity provider that add_liquidity function which is considered loss of funds for the users .

Tools Used

vs code and manual review

Recommended Mitigation Steps

consider modify the calculate_add_one_asset function to calculate the right amount of assets to be taken from the user .

the protocol may implement this change here :

-           let dy = y1.checked_sub(asset_reserve)?;
+           let dy = y1.checked_sub(reserves[asset_index])?;

Assessed type

Math

[c4-pre-sort]

0xRobocop marked the issue as sufficient quality report

[c4-pre-sort]

0xRobocop marked the issue as primary issue

[c4-pre-sort]

0xRobocop marked the issue as high quality report

[c4-sponsor]

enthusiastmartin (sponsor) disputed

[enthusiastmartin]

This is by design, math cannot be precised enough. We deliberately implemented so using this function, users gets little bit less of shares to ensure that there is no impact for protocol.

Does not break any invariant.

What we would like to see instead is that using this function, users can get same amount if shares for less liquidity.

Similar to #38

[OpenCoreCH]

Rounding difference in the order of magngitue of 10^-7 -> QA

[c4-judge]

OpenCoreCH changed the severity to QA (Quality Assurance)