Low reserves can lead to a user receiving more shares than intended

[c4-bot-10]

Lines of code

https://github.com/code-423n4/2024-02-hydradx/blob/main/HydraDX-node/math/src/stableswap/math.rs#L498-L503

Vulnerability details

Bug Description

The StableSwap invariant D is used to calculate the amount of shares as well as the tokens returned on swaps. The HydraDx Stableswap deviates from the original whitepaper implementation by adding 2 to the invariant to account for rounding errors. The changed functionality is also described in a comment.

// adding two here is sufficient to account for rounding
// errors, AS LONG AS the minimum reserves are 2 for each
// asset. I.e., as long as xp_hp[0] >= 2 and xp_hp[1] >= 2
// adding two guarantees that this function will return
// a value larger than or equal to the correct D invariant

Unfortunately, as the MinPoolLiquidity is only enforced on the shares of a pool, but not the single reserves, trades can lead to a pool where there is only 1 token of an asset left. This will lead to an incorrect calculation of D, which can result in losses to the users. An example where this, lower than intended D, could lead to issues is on deposits. On a deposit of liquidity, the process is as follows:

1. Calculate D for current reserves
2. Add deposited liquidity
3. Calculate D for new reserves
4. Give the user shares which are $newD - oldD$

One can see that this can lead to issues, if in the current reserve, one of the reserves is 1. In that case the oldD would be smaller than intended, while the newD, where the reserves of all are above 1, would be calculated correctly. This way the user depositing liquidity would receive more shares than intended. This would allow for the following attack path:

1. Withdraw/Swap enough of one asset so that the reserve falls to 1 (0 is impossible)
2. Deposit Liquidity for that asset
3. Receive more shares than intended

   Impact

The issue allows a malicious user to push the pool into a state where he will receive more shares on his deposit of liquidity than intended.It needs to be mentioned that this only works on a pool with a current fee of 0%. As the fee can be set by the Authority for each pool and can be in the range between 0-100% this can be considered a valid state of a pool.

Proof of Concept

The following POC showcases an example of a user pushing the pool into the unintended state:

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

    ExtBuilder::default()
        .with_endowed_accounts(vec![
            (BOB, 1, 200 * ONE),
            (BOB, 2, 200 * ONE),
            (ALICE, 1, 200 * ONE),
            (ALICE, 2, 200 * ONE),
        ])
        .with_registered_asset("one".as_bytes().to_vec(), asset_a, 12)
        .with_registered_asset("two".as_bytes().to_vec(), asset_b, 12)
        .with_pool(
            ALICE,
            PoolInfo::<AssetId, u64> {
                assets: vec![asset_a, asset_b].try_into().unwrap(),
                initial_amplification: NonZeroU16::new(1000).unwrap(),
                final_amplification: NonZeroU16::new(1000).unwrap(),
                initial_block: 0,
                final_block: 0,
                fee: Permill::from_percent(0),
            },
            InitialLiquidity {
                account: ALICE,
                assets: vec![
                    AssetAmount::new(asset_a, 100 * ONE),
                    AssetAmount::new(asset_b, 100 * ONE),
                ],
            },
        )
        .build()
        .execute_with(|| {
            let pool_id = get_pool_id_at(0);

            //Alice wants to withdraw her initial liquidity again
            assert_ok!(Stableswap::withdraw_asset_amount(
                RuntimeOrigin::signed(ALICE),
                pool_id,
                asset_a,
                100 * ONE-1,
                340282366920938463463374607431768211455,
            ));

            // now liquidity of asset_a is 1
        });
}

The file can be added to pallets/stableswap/src/test/remove_liquidity.rs

Tools Used

Manual Review

Recommended Mitigation Steps

The issue can be mitigated by enforcing that reserves of all assets should never fall below 2. This must be enforced on every swap and on each withdrawal of liquidity.

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-sponsor]

enthusiastmartin (sponsor) disputed

[enthusiastmartin]

Although you can make the reserve of asset 1, it does not show that there is an issue with that.

[OpenCoreCH]

The PoC shows that it is possible to end up in a state where the reserve of an asset is 1. However, it is not shown that this will lead to any problems in the system. The comment in calculate_d_internal only states that adding two is sufficient for rounding errors when the reserves are > 1, it does not say anything about the potential system impacts otherwise.

[c4-judge]

OpenCoreCH marked the issue as unsatisfactory: Insufficient proof