Acala - n4nika's results

Building the liquidity layer of Web3 finance.

General Information

Platform: Code4rena

Start Date: 22/03/2024

Pot Size: $36,500 USDC

Total HM: 7

Participants: 17

Period: 14 days

Judge: Lambda

Id: 323

League: POLKADOT

Acala Network

Findings Distribution

Researcher Performance

Rank: 10/17

Findings: 2

Award: $746.92

🌟 Selected for report: 1

🚀 Solo Findings: 0

External Links

Code4rena Contest Page Github Contest Repository Github Findings Repository Acala Network

Findings Information

🌟 Selected for report: n4nika

Also found by: ABAIKUNANBAEV, djxploit

Labels

bug

2 (Med Risk)

primary issue

selected for report

sponsor disputed

sufficient quality report

:robot:_10_group

M-01

Awards

705.0144 USDC - $705.01

External Links

Link to GitHub issue

Lines of code

https://github.com/code-423n4/2024-03-acala/blob/9c71c05cf2d9f0a2603984c50f76fc8a315d4d65/src/modules/incentives/src/lib.rs#L451-L457

Vulnerability details

Impact

If a participant in a pool claims rewards while the deduction rate is != 0, the deducted rewards are redistributed between all the participants.

fn payout_reward_and_reaccumulate_reward(
	pool_id: PoolId,
	who: &T::AccountId,
	reward_currency_id: CurrencyId,
	payout_amount: Balance,
	reaccumulate_amount: Balance,
) -> DispatchResult {
	if !reaccumulate_amount.is_zero() {
		<orml_rewards::Pallet<T>>::accumulate_reward(&pool_id, reward_currency_id, reaccumulate_amount)?;
	}
	T::Currency::transfer(reward_currency_id, &Self::account_id(), who, payout_amount)?;
	Ok(())
}

Since the deduction is redistributed between all participants (since accumulate_reward distributes to all participating users) including the one claiming the reward, they can repeatedly claim rewards and receive more of the rewards pool than they probably should.

Proof of Concept

#[test]
fn repeated_claiming() {
	ExtBuilder::default().build().execute_with(|| {
		assert_ok!(TokensModule::deposit(BTC, &VAULT::get(), 10000));

		assert_ok!(IncentivesModule::update_claim_reward_deduction_rates(
			RuntimeOrigin::signed(ROOT::get()),
			vec![(PoolId::Dex(BTC_AUSD_LP), Rate::saturating_from_rational(0, 100))]
		));

		assert_ok!(IncentivesModule::update_claim_reward_deduction_rates(
			RuntimeOrigin::signed(ROOT::get()),
			vec![(PoolId::Dex(BTC_AUSD_LP), Rate::saturating_from_rational(20, 100))]
		));

		// 40% deduction rate
		assert_ok!(IncentivesModule::update_claim_reward_deduction_rates(
			RuntimeOrigin::signed(ROOT::get()),
			vec![(PoolId::Dex(BTC_AUSD_LP), Rate::saturating_from_rational(40, 100))]
		));

		// give RewardsSource tokens
		TokensModule::deposit(BTC, &RewardsSource::get(), 10000);

		// add 11 participants with equal shares
		TokensModule::deposit(BTC_AUSD_LP, &ALICE::get(), 100);
		TokensModule::deposit(BTC_AUSD_LP, &BOB::get(), 100);


		// 1000 BTC as reward
		IncentivesModule::update_incentive_rewards(RuntimeOrigin::signed(ROOT::get()), vec![(PoolId::Dex(BTC_AUSD_LP), vec![(BTC, 1000)])]);
		
		// all 11 participants deposit their share
		assert_ok!(IncentivesModule::deposit_dex_share(RuntimeOrigin::signed(ALICE::get()), BTC_AUSD_LP, 100));
		assert_ok!(IncentivesModule::deposit_dex_share(RuntimeOrigin::signed(BOB::get()), BTC_AUSD_LP, 100));
		
		// first accumulation of rewards
		IncentivesModule::on_initialize(10);
		
		// ALICE claims their reward and should receive ((1000 // 2) * 0.6) = 300 BTC
		assert_ok!(IncentivesModule::claim_rewards(RuntimeOrigin::signed(ALICE::get()), PoolId::Dex(BTC_AUSD_LP)));
		let btc_before = TokensModule::total_balance(BTC, &ALICE::get());
		
		// here ALICE should have claimed all their rewards and not be able to claim anymore
		assert_ok!(IncentivesModule::claim_rewards(RuntimeOrigin::signed(ALICE::get()), PoolId::Dex(BTC_AUSD_LP)));
		let btc_after = TokensModule::total_balance(BTC, &ALICE::get());

		assert_eq!(btc_before, btc_after);
	});
}

Tools Used

Manual review

Recommended Mitigation Steps

Add the claiming user to accumulate_rewards and implement reaccumulating excluding the calling user.

Assessed type

Other

#0 - c4-pre-sort
2024-04-07T13:01:16Z

DadeKuma marked the issue as primary issue

#1 - DadeKuma
2024-04-07T13:41:44Z

Rewards redistributed between users, including users who are claiming

#2 - c4-pre-sort
2024-04-07T13:42:18Z

DadeKuma marked the issue as sufficient quality report

#3 - c4-sponsor
2024-04-09T03:49:27Z

xlc (sponsor) acknowledged

#4 - c4-judge
2024-04-09T16:15:28Z

OpenCoreCH marked the issue as selected for report

#5 - xlc
2024-04-11T02:13:08Z

This is intended behaviour and non issue.

#6 - c4-sponsor
2024-04-30T03:52:13Z

xlc (sponsor) disputed

Findings Information

🌟 Selected for report: Bauchibred

Also found by: 0xTheC0der, AM, Cryptor, ZanyBonzy, n4nika, zhaojie

Labels

bug

grade-b

QA (Quality Assurance)

sufficient quality report

Q-01

Awards

41.9121 USDC - $41.91

External Links

Link to GitHub issue

Misdistribution of rewards if they cannot be cleanly distributed

Impact

In the incentives module, in certain situations where ((REWARDS // AMNT_PARTICIPANTS) * AMNT_PARTICIPANTS) != REWARDS and the participants claim their rewards, if a new participant deposits shares into the pool after the first accumulation of rewards, upon the next accumulation, the rounding-difference of the first round is distributed between all participants, not only the ones participating during the first accumulation.
This violates "When a user adds liquidity to the pool, they receive shares, and withdrawn rewards are adjusted accordingly so the new user will start with no reward." written in the scope description on the C4 site for the audit.

Proof of Concept

Requires adding the following accounts to modules/incentives/src/mock.rs:

pub const ACC1: AccountId = AccountId::from([6u8; 32]);
pub const ACC2: AccountId = AccountId::from([7u8; 32]);
pub const ACC3: AccountId = AccountId::from([8u8; 32]);
pub const ACC4: AccountId = AccountId::from([9u8; 32]);
pub const ACC5: AccountId = AccountId::from([10u8; 32]);
pub const ACC6: AccountId = AccountId::from([11u8; 32]);
pub const ACC7: AccountId = AccountId::from([12u8; 32]);
pub const ACC8: AccountId = AccountId::from([13u8; 32]);
pub const ACC9: AccountId = AccountId::from([14u8; 32]);

#[test]
fn bad_distribution() {
	ExtBuilder::default().build().execute_with(|| {
		assert_ok!(TokensModule::deposit(BTC, &VAULT::get(), 10000));

		assert_ok!(IncentivesModule::update_claim_reward_deduction_rates(
			RuntimeOrigin::signed(ROOT::get()),
			vec![(PoolId::Dex(BTC_AUSD_LP), Rate::saturating_from_rational(0, 100))]
		));

		// give RewardsSource tokens
		TokensModule::deposit(BTC, &RewardsSource::get(), 10000);

		// add 11 participants with equal shares
		TokensModule::deposit(BTC_AUSD_LP, &ALICE::get(), 100);
		TokensModule::deposit(BTC_AUSD_LP, &BOB::get(), 100);
		TokensModule::deposit(BTC_AUSD_LP, &CHARLIE::get(), 100);
		TokensModule::deposit(BTC_AUSD_LP, &ACC1::get(), 100);
		TokensModule::deposit(BTC_AUSD_LP, &ACC2::get(), 100);
		TokensModule::deposit(BTC_AUSD_LP, &ACC3::get(), 100);
		TokensModule::deposit(BTC_AUSD_LP, &ACC4::get(), 100);
		TokensModule::deposit(BTC_AUSD_LP, &ACC5::get(), 100);
		TokensModule::deposit(BTC_AUSD_LP, &ACC6::get(), 100);
		TokensModule::deposit(BTC_AUSD_LP, &ACC7::get(), 100);
		TokensModule::deposit(BTC_AUSD_LP, &ACC8::get(), 100);
		TokensModule::deposit(BTC_AUSD_LP, &ACC9::get(), 100);

		// 1000 BTC as reward
		IncentivesModule::update_incentive_rewards(RuntimeOrigin::signed(ROOT::get()), vec![(PoolId::Dex(BTC_AUSD_LP), vec![(BTC, 1000)])]);
		
		// all 11 participants deposit their share
		assert_ok!(IncentivesModule::deposit_dex_share(RuntimeOrigin::signed(ALICE::get()), BTC_AUSD_LP, 100));
		assert_ok!(IncentivesModule::deposit_dex_share(RuntimeOrigin::signed(BOB::get()), BTC_AUSD_LP, 100));
		assert_ok!(IncentivesModule::deposit_dex_share(RuntimeOrigin::signed(CHARLIE::get()), BTC_AUSD_LP, 100));
		assert_ok!(IncentivesModule::deposit_dex_share(RuntimeOrigin::signed(ACC1::get()), BTC_AUSD_LP, 100));
		assert_ok!(IncentivesModule::deposit_dex_share(RuntimeOrigin::signed(ACC2::get()), BTC_AUSD_LP, 100));
		assert_ok!(IncentivesModule::deposit_dex_share(RuntimeOrigin::signed(ACC3::get()), BTC_AUSD_LP, 100));
		assert_ok!(IncentivesModule::deposit_dex_share(RuntimeOrigin::signed(ACC4::get()), BTC_AUSD_LP, 100));
		assert_ok!(IncentivesModule::deposit_dex_share(RuntimeOrigin::signed(ACC5::get()), BTC_AUSD_LP, 100));
		assert_ok!(IncentivesModule::deposit_dex_share(RuntimeOrigin::signed(ACC6::get()), BTC_AUSD_LP, 100));
		assert_ok!(IncentivesModule::deposit_dex_share(RuntimeOrigin::signed(ACC7::get()), BTC_AUSD_LP, 100));
		assert_ok!(IncentivesModule::deposit_dex_share(RuntimeOrigin::signed(ACC8::get()), BTC_AUSD_LP, 100));
		
		// first accumulation of rewards
		IncentivesModule::on_initialize(10);
		
		// everyone claims their rewards
		assert_ok!(IncentivesModule::claim_rewards(RuntimeOrigin::signed(ALICE::get()), PoolId::Dex(BTC_AUSD_LP)));
		assert_ok!(IncentivesModule::claim_rewards(RuntimeOrigin::signed(BOB::get()), PoolId::Dex(BTC_AUSD_LP)));
		assert_ok!(IncentivesModule::claim_rewards(RuntimeOrigin::signed(CHARLIE::get()), PoolId::Dex(BTC_AUSD_LP)));
		assert_ok!(IncentivesModule::claim_rewards(RuntimeOrigin::signed(ACC1::get()), PoolId::Dex(BTC_AUSD_LP)));
		assert_ok!(IncentivesModule::claim_rewards(RuntimeOrigin::signed(ACC2::get()), PoolId::Dex(BTC_AUSD_LP)));
		assert_ok!(IncentivesModule::claim_rewards(RuntimeOrigin::signed(ACC3::get()), PoolId::Dex(BTC_AUSD_LP)));
		assert_ok!(IncentivesModule::claim_rewards(RuntimeOrigin::signed(ACC4::get()), PoolId::Dex(BTC_AUSD_LP)));
		assert_ok!(IncentivesModule::claim_rewards(RuntimeOrigin::signed(ACC5::get()), PoolId::Dex(BTC_AUSD_LP)));
		assert_ok!(IncentivesModule::claim_rewards(RuntimeOrigin::signed(ACC6::get()), PoolId::Dex(BTC_AUSD_LP)));
		assert_ok!(IncentivesModule::claim_rewards(RuntimeOrigin::signed(ACC7::get()), PoolId::Dex(BTC_AUSD_LP)));
		assert_ok!(IncentivesModule::claim_rewards(RuntimeOrigin::signed(ACC8::get()), PoolId::Dex(BTC_AUSD_LP)));

		// new user deposits into the pool
		assert_ok!(IncentivesModule::deposit_dex_share(RuntimeOrigin::signed(ACC9::get()), BTC_AUSD_LP, 100));
		
		// new accumulation, this adds another 1000 BTC as rewards
		IncentivesModule::on_initialize(20);

		assert_ok!(IncentivesModule::claim_rewards(RuntimeOrigin::signed(ACC9::get()), PoolId::Dex(BTC_AUSD_LP)));
		
		// the nerw participant should have (1000 // 12) = 83 BTC 
		// but it has 84, which is a portion of the rounded-away tokens from the first accumulation
		assert_eq!(TokensModule::total_balance(BTC, &ACC9::get()), 83);
	});
}

#0 - DadeKuma
2024-04-07T15:00:49Z

L1 -> dup of #98

If downgraded to QA, passes B grade

#1 - c4-pre-sort
2024-04-07T15:01:02Z

DadeKuma marked the issue as sufficient quality report

#2 - c4-pre-sort
2024-04-07T15:01:05Z

DadeKuma marked the issue as grade-c

#3 - c4-judge
2024-04-09T16:10:33Z

OpenCoreCH marked the issue as grade-b

Acala - n4nika's results

Building the liquidity layer of Web3 finance.

General Information

Findings Distribution

Researcher Performance

External Links

🌟 [M-01] Claiming rewards while the deduction rate is `!= 0`, allows for repeated withdrawal of redistributed rewards - $705.01

Findings Information

Labels

Awards

External Links

Lines of code

Vulnerability details

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps

Assessed type

#0 - c4-pre-sort2024-04-07T13:01:16Z

#1 - DadeKuma2024-04-07T13:41:44Z

#2 - c4-pre-sort2024-04-07T13:42:18Z

#3 - c4-sponsor2024-04-09T03:49:27Z

#4 - c4-judge2024-04-09T16:15:28Z

#5 - xlc2024-04-11T02:13:08Z

#6 - c4-sponsor2024-04-30T03:52:13Z

[Q-01] QA Report - $41.91

Findings Information

Labels

Awards

External Links

Misdistribution of rewards if they cannot be cleanly distributed

Impact

Proof of Concept

#0 - DadeKuma2024-04-07T15:00:49Z

#1 - c4-pre-sort2024-04-07T15:01:02Z

#2 - c4-pre-sort2024-04-07T15:01:05Z

#3 - c4-judge2024-04-09T16:10:33Z

#0 - c4-pre-sort
2024-04-07T13:01:16Z

#1 - DadeKuma
2024-04-07T13:41:44Z

#2 - c4-pre-sort
2024-04-07T13:42:18Z

#3 - c4-sponsor
2024-04-09T03:49:27Z

#4 - c4-judge
2024-04-09T16:15:28Z

#5 - xlc
2024-04-11T02:13:08Z

#6 - c4-sponsor
2024-04-30T03:52:13Z

#0 - DadeKuma
2024-04-07T15:00:49Z

#1 - c4-pre-sort
2024-04-07T15:01:02Z

#2 - c4-pre-sort
2024-04-07T15:01:05Z

#3 - c4-judge
2024-04-09T16:10:33Z