Acala - 0xTheC0der's results

Lines of code

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

Vulnerability details

Impact

Incentives are accumulated periodically in intervals of T::AccumulatePeriod blocks. Thereby, a fixed incentive reward amount, which is set via update_incentive_rewards(...), is accumulated among all deposited shares of a respective pool. This means if only 1 share is deposited, it is entitled for the all the rewards of this period, if N shares are deposited, the rewards are split among them, etc. (see PoC).
Furthermore, to be eligible for rewards, it is sufficient to deposit (DEX) shares before accumulate_incentives(...) is called via the on_initialize hook. This is also demonstrated in the transfer_reward_and_update_rewards_storage_atomically_when_accumulate_incentives_work() test case.
Afterwards, rewards can be immediately claimed and (DEX) shares can be withdrawn without any unbonding period or other block/time related restrictions.

This leads to the following consequences:

• Users are not incentivized to keep (DEX) shares deposited, but rather to sandwich the incentive accumulation via deposit, claim & withdraw.
• Since (DEX) shares are only required for a short period of time (> 1 block) to perform the above action, an adversary can borrow vast amounts of (DEX) shares (or the underlying assets to get them) to crowd out honest users during the incentive accumulation and therefore obtain an unfairly high share of the fixed rewards.

For reference, see also the earnings module, where an unbonding period is explicitly enforced.

Proof of Concept

The diff below modifies the existing test case transfer_reward_and_update_rewards_storage_atomically_when_accumulate_incentives_work() to demonstrate that the same total reward amount is accumulated even if twice as many total shares are deposited. Therefore, it's possible to crowd out honest users with great short-term share deposits.

diff --git a/src/modules/incentives/src/tests.rs b/src/modules/incentives/src/tests.rs
index 1370d5b..fa16a08 100644
--- a/src/modules/incentives/src/tests.rs
+++ b/src/modules/incentives/src/tests.rs
@@ -1171,10 +1171,11 @@ fn transfer_reward_and_update_rewards_storage_atomically_when_accumulate_incenti
 		assert_eq!(TokensModule::free_balance(AUSD, &VAULT::get()), 0);
 
 		RewardsModule::add_share(&ALICE::get(), &PoolId::Loans(LDOT), 1);
+		RewardsModule::add_share(&BOB::get(), &PoolId::Loans(LDOT), 1);
 		assert_eq!(
 			RewardsModule::pool_infos(PoolId::Loans(LDOT)),
 			PoolInfo {
-				total_shares: 1,
+				total_shares: 2,
 				..Default::default()
 			}
 		);
@@ -1188,7 +1189,7 @@ fn transfer_reward_and_update_rewards_storage_atomically_when_accumulate_incenti
 		assert_eq!(
 			RewardsModule::pool_infos(PoolId::Loans(LDOT)),
 			PoolInfo {
-				total_shares: 1,
+				total_shares: 2,
 				rewards: vec![(ACA, (30, 0)), (AUSD, (90, 0))].into_iter().collect()
 			}
 		);
@@ -1202,7 +1203,7 @@ fn transfer_reward_and_update_rewards_storage_atomically_when_accumulate_incenti
 		assert_eq!(
 			RewardsModule::pool_infos(PoolId::Loans(LDOT)),
 			PoolInfo {
-				total_shares: 1,
+				total_shares: 2,
 				rewards: vec![(ACA, (60, 0)), (AUSD, (90, 0))].into_iter().collect()
 			}
 		);

Tools Used

Manual review

Recommended Mitigation Steps

The present issue scales with the size of T::AccumulatePeriod in terms of blocks. Therefore, it's recommended (for fairness) to also track deposited shares in-between the accumulation intervals and scale the incentive rewards according to the actual deposit duration.

Assessed type

MEV

L-1: Reliance on block number instead of timestamp

Incentives are accumulated periodically in intervals of T::AccumulatePeriod blocks.
This is not recommended since the block production time is not assured to be constant over time.
Consequently, inconsistent incentive reward amounts (per time interval) can be accumulated, which is perceived is unpredictable and unfair by users.

Recommendation:
Use timestamp based incentive accumulation for fixed time periods. See also: pallet-timestamp
Or, if the block number based approach is still desired, add a method which allows to change the accumulate period if necessary.

NC-1: Inconsistent hook naming among earning an incentives modules

According to the README about the earning module:

The module implements a set of hooks that can be used by other modules (i.e. Incentives) to implement staking.

Hook names in earning module: OnBonded, OnUnbonded

Hook names in incentives module: OnEarningBonded, OnEarningUnbonded

Since those two modules are meant to be connected via these hooks, the inconsistent naming might be misleading and cause integration issues.