remove_liquidity_one_asset returns a smaller amount of assets than withdraw_asset_amount for the same shares

[c4-bot-2]

Lines of code

https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/math/src/stableswap/math.rs#L173-L230 https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/math/src/stableswap/math.rs#L234-L314

Vulnerability details

Impact

When testing stableswap it turned out that if you use remove_liquidity_one_asset you get a little less than with withdraw_asset_amount if you use the same number of shares for both.

Proof of Concept

To prove this there is this POC that can be inserted into integration-tests/src/omnipool_init.rs.

#[test]
fn poc1() {
    TestNet::reset();

    Hydra::execute_with(|| {
        //Metadata is set for two tokens used in the stable pool so that the pool can use the decimals. In this case both assets have the same decimals
        assert_ok!(hydradx_runtime::AssetRegistry::set_metadata(hydradx_runtime::RuntimeOrigin::root(), DAI, vec![1,2], 12));
        assert_ok!(hydradx_runtime::AssetRegistry::set_metadata(hydradx_runtime::RuntimeOrigin::root(), DOT, vec![3,4], 12));

        //The stable pool is created with DAI and DOT as assets and ACA as share asset
        assert_ok!(hydradx_runtime::Stableswap::create_pool(
            hydradx_runtime::RuntimeOrigin::root(),
            ACA,
            vec![DAI, DOT],
            12,
            Permill::from_percent(1)
        ));

        //Initial liquidity is added
        assert_ok!(hydradx_runtime::Stableswap::add_liquidity(
            hydradx_runtime::RuntimeOrigin::signed(ALICE.into()),
            ACA,
            vec![AssetAmount{
                    asset_id: DAI,
                    amount: 1000 * UNITS
                },
            AssetAmount{
                asset_id: DOT,
                amount: 1000 * UNITS
            }
            ]
        ));

        println!("alice dai before: {}", hydradx_runtime::Tokens::free_balance(DAI, &AccountId::from(ALICE)));
        println!("alice aca before: {}", hydradx_runtime::Tokens::free_balance(ACA, &AccountId::from(ALICE)));
        /*assert_ok!(hydradx_runtime::Stableswap::withdraw_asset_amount( //liquidity is removed with withdraw_asset_amount
            hydradx_runtime::RuntimeOrigin::signed(ALICE.into()),
            ACA,
            DAI,
            100 * UNITS,
            10 * UNITS * UNITS
        ));*/

        assert_ok!(hydradx_runtime::Stableswap::remove_liquidity_one_asset( //liquidity is removed with remove_liquidity_one_asset
            hydradx_runtime::RuntimeOrigin::signed(ALICE.into()),
            ACA,
            DAI,
            100702978470355663983, //number of shares needed from withdraw_asset_amount to get 100 DAI
            0
        ));
        println!("alice dai after: {}", hydradx_runtime::Tokens::free_balance(DAI, &AccountId::from(ALICE)));
        println!("alice aca after: {}", hydradx_runtime::Tokens::free_balance(ACA, &AccountId::from(ALICE)));
    });
}

Description

Now you can run the POC with this command SKIP_WASM_BUILD=1 cargo test -p runtime-integration-tests stableswap_test -- --nocapture and see how many DAI Alice gets for how many shares based on the log in the terminal. To compare the two functions you can now comment out remove_liquidity_one_asset and execute withdraw_asset_amount and compare the logs in the terminal. You will notice that with the withdraw_asset_amount function, Alice gets a little more for the same number of shares as with remove_liquidity_one_asset.

This difference can be explained as follows: remove_liquidity_one_asset uses calculate_withdraw_one_asset to calculate how much of a reserve a user gets for a certain number of shares. withdraw_asset_amount uses calculate_shares_for_amount to calculate how many shares a user must pay to get a certain amount of reserve. Both functions have a part that calculates the fees:

calculate_withdraw_one_asset

for (idx, reserve) in xp_hp.iter().enumerate() {
        let dx_expected = if idx == asset_index {
            // dx_expected = xp[j] * d1 / d0 - new_y
            reserve.checked_mul(d1)?.checked_div(d_hp)?.checked_sub(y_hp)?
        } else {
            // dx_expected = xp[j] - xp[j] * d1 / d0
            reserve.checked_sub(reserve.checked_mul(d1)?.checked_div(d_hp)?)?
        };

        let expected = Balance::try_from(dx_expected).ok()?;
        let reduced = Balance::try_from(*reserve)
            .ok()?
            .checked_sub(fee.checked_mul_int(expected)?)?;

calculate_shares_for_amount

let adjusted_reserves: Vec<AssetReserve> = updated_reserves
        .iter()
        .enumerate()
        .map(|(idx, asset_reserve)| -> Option<AssetReserve> {
            let (initial_reserve, updated_reserve) = to_u256!(initial_reserves[idx].amount, asset_reserve.amount);
            let ideal_balance = d1.checked_mul(initial_reserve)?.checked_div(d0)?;
            let diff = Balance::try_from(updated_reserve.abs_diff(ideal_balance)).ok()?;
            let fee_amount = fee.checked_mul_int(diff)?;
            Some(AssetReserve::new(
                asset_reserve.amount.saturating_sub(fee_amount),
                asset_reserve.decimals,
            ))
        })

Here you can see that for both functions a difference is calculated (for calculate_withdraw_one_asset it is called expected and for calculate_shares_for_amount it is called diff) which is then used to calculate the fees. The problem is that d1 is different for the two functions.

let d1 = d_hp.checked_sub(shares_hp.checked_mul(d_hp)?.checked_div(issuance_hp)?)?;

Here we calculate how many assets the pool still has after the amount for all shares that are burned has been removed. So in this case the fees have already been deducted from d1.

With calculate_shares_for_amount for amount, d1 is calculated by deducting the amount that the user gets at the end from the reserves, but the fees are not yet deducted:

let updated_reserves: Vec<AssetReserve> = initial_reserves
        .iter()
        .enumerate()
        .map(|(idx, v)| -> Option<AssetReserve> {
            if idx == asset_idx {
                Some(AssetReserve::new(v.amount.checked_sub(amount)?, v.decimals))
            } else {
                Some(*v)
            }
        })
        .collect::<Option<Vec<AssetReserve>>>()?;

    let initial_d = calculate_d::<D>(initial_reserves, amplification)?;
    let updated_d = calculate_d::<D>(&updated_reserves, amplification)?;
    let (d1, d0) = to_u256!(updated_d, initial_d);

Since the user gets more out of one function than the other, the same d1 should be used for both functions. So for calculate_withdraw_one_asset you should also use the d1 where the fees have not yet been deducted because they are calculated in this function.

Tools Used

VSCode

Assessed type

Math

[0xRobocop]

No mitigation provided. Seems similar to #59

[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 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.

[OpenCoreCH]

See #59, very small rounding difference.

[c4-judge]

OpenCoreCH changed the severity to QA (Quality Assurance)

[c4-judge]

OpenCoreCH marked the issue as grade-a