Some improvement for security

modules/cdp-treasury/src/lib.rs

@@ -293,7 +293,7 @@ impl<T: Config> Pallet<T> {
 
 		// Burn the amount that is equal to offset amount of stable currency.
 		if !offset_amount.is_zero() {
-			let res = T::Currency::withdraw(T::GetStableCurrencyId::get(), &Self::account_id(), offset_amount);
+			let res = Self::burn_debit(&Self::account_id(), offset_amount);
 			match res {
 				Ok(_) => {
 					DebitPool::<T>::mutate(|debit| {
@@ -346,6 +346,7 @@ impl<T: Config> CDPTreasury<T::AccountId> for Pallet<T> {
 		Self::issue_debit(&Self::account_id(), amount, true)
 	}
 
+	/// This should be the only function in the system that issues stable coin
 	fn issue_debit(who: &T::AccountId, debit: Self::Balance, backed: bool) -> DispatchResult {
 		// increase system debit if the debit is unbacked
 		if !backed {
@@ -356,6 +357,7 @@ impl<T: Config> CDPTreasury<T::AccountId> for Pallet<T> {
 		Ok(())
 	}
 
+	/// This should be the only function in the system that burns stable coin
 	fn burn_debit(who: &T::AccountId, debit: Self::Balance) -> DispatchResult {
 		T::Currency::withdraw(T::GetStableCurrencyId::get(), who, debit)
 	}

modules/homa/src/lib.rs

@@ -170,6 +170,8 @@ pub mod module {
 		FastMatchIsNotAllowed,
 		/// The fast match cannot be matched completely.
 		CannotCompletelyFastMatch,
+		/// Invalid last era bumped block config
+		InvalidLastEraBumpedBlock,
 	}
 
 	#[pallet::event]
@@ -527,17 +529,34 @@ pub mod module {
 		) -> DispatchResult {
 			T::GovernanceOrigin::ensure_origin(origin)?;
 
-			if let Some(change) = last_era_bumped_block {
-				LastEraBumpedBlock::<T>::put(change);
-				Self::deposit_event(Event::<T>::LastEraBumpedBlockUpdated {
-					last_era_bumped_block: change,
-				});
-			}
 			if let Some(change) = frequency {
 				BumpEraFrequency::<T>::put(change);
 				Self::deposit_event(Event::<T>::BumpEraFrequencyUpdated { frequency: change });
 			}
 
+			if let Some(change) = last_era_bumped_block {
+				// config last_era_bumped_block should not cause bump era to occur immediately, because
+				// the last_era_bumped_block after the bump era will not be same with the actual relaychain
+				// era bumped block  again, especially if it leads to multiple bump era.
+				// and it should be config after config no-zero bump_era_frequency.
+				let bump_era_frequency = Self::bump_era_frequency();
+				let current_relay_chain_block = T::RelayChainBlockNumber::current_block_number();
+				if !bump_era_frequency.is_zero() {
+					// ensure change in this range (current_relay_chain_block-bump_era_frequency,
+					// current_relay_chain_block]
+					ensure!(
+						change > current_relay_chain_block.saturating_sub(bump_era_frequency)
+							&& change <= current_relay_chain_block,
+						Error::<T>::InvalidLastEraBumpedBlock
+					);
+
+					LastEraBumpedBlock::<T>::put(change);
+					Self::deposit_event(Event::<T>::LastEraBumpedBlockUpdated {
+						last_era_bumped_block: change,
+					});
+				}
+			}
+
 			Ok(())
 		}
 
@@ -683,7 +702,8 @@ pub mod module {
 				.saturating_mul_int(liquid_amount);
 			let liquid_add_to_void = liquid_amount.saturating_sub(liquid_issue_to_minter);
 
-			T::Currency::deposit(T::LiquidCurrencyId::get(), &minter, liquid_issue_to_minter)?;
+			Self::issue_liquid_currency(&minter, liquid_issue_to_minter)?;
+
 			ToBondPool::<T>::mutate(|pool| *pool = pool.saturating_add(amount));
 			TotalVoidLiquid::<T>::mutate(|total| *total = total.saturating_add(liquid_add_to_void));
 
@@ -833,7 +853,7 @@ pub mod module {
 
 						// burn liquid_to_burn for redeemed_staking and burn fee_in_liquid to reward all holders of
 						// liquid currency.
-						T::Currency::withdraw(T::LiquidCurrencyId::get(), &module_account, actual_liquid_to_redeem)?;
+						Self::burn_liquid_currency(&module_account, actual_liquid_to_redeem)?;
 
 						// transfer redeemed_staking to redeemer.
 						T::Currency::transfer(
@@ -897,7 +917,6 @@ pub mod module {
 
 				let commission_rate = Self::commission_rate();
 				if !total_reward_staking.is_zero() && !commission_rate.is_zero() {
-					let liquid_currency_id = T::LiquidCurrencyId::get();
 					let commission_staking_amount = commission_rate.saturating_mul_int(total_reward_staking);
 					let commission_ratio =
 						Ratio::checked_from_rational(commission_staking_amount, TotalStakingBonded::<T>::get())
@@ -907,7 +926,7 @@ pub mod module {
 						.unwrap_or_else(Ratio::max_value);
 					let inflate_liquid_amount = inflate_rate.saturating_mul_int(Self::get_total_liquid_currency());
 
-					T::Currency::deposit(liquid_currency_id, &T::TreasuryAccount::get(), inflate_liquid_amount)?;
+					Self::issue_liquid_currency(&T::TreasuryAccount::get(), inflate_liquid_amount)?;
 				}
 			}
 
@@ -937,11 +956,7 @@ pub mod module {
 			}
 
 			// issue withdrawn unbonded to module account for redeemer to claim
-			T::Currency::deposit(
-				T::StakingCurrencyId::get(),
-				&Self::account_id(),
-				total_withdrawn_staking,
-			)?;
+			Self::issue_staking_currency(&Self::account_id(), total_withdrawn_staking)?;
 			UnclaimedRedemption::<T>::mutate(|total| *total = total.saturating_add(total_withdrawn_staking));
 
 			Ok(())
@@ -1050,7 +1065,7 @@ pub mod module {
 			}
 
 			// burn total_redeem_amount.
-			T::Currency::withdraw(T::LiquidCurrencyId::get(), &Self::account_id(), total_redeem_amount)
+			Self::burn_liquid_currency(&Self::account_id(), total_redeem_amount)
 		}
 
 		pub fn era_amount_should_to_bump(relaychain_block_number: T::BlockNumber) -> EraIndex {
@@ -1090,6 +1105,22 @@ pub mod module {
 
 			res
 		}
+
+		/// This should be the only function in the system that issues liquid currency
+		fn issue_liquid_currency(who: &T::AccountId, amount: Balance) -> DispatchResult {
+			T::Currency::deposit(T::LiquidCurrencyId::get(), who, amount)
+		}
+
+		/// This should be the only function in the system that burn liquid currency
+		fn burn_liquid_currency(who: &T::AccountId, amount: Balance) -> DispatchResult {
+			T::Currency::withdraw(T::LiquidCurrencyId::get(), who, amount)
+		}
+
+		/// Issue staking currency based on the subaccounts transfer the unbonded staking currency
+		/// to the parachain account
+		fn issue_staking_currency(who: &T::AccountId, amount: Balance) -> DispatchResult {
+			T::Currency::deposit(T::StakingCurrencyId::get(), who, amount)
+		}
 	}
 
 	impl<T: Config> ExchangeRateProvider for Pallet<T> {

modules/homa/src/tests.rs

@@ -300,6 +300,8 @@ fn update_bump_era_params_works() {
 		assert_eq!(Homa::last_era_bumped_block(), 0);
 		assert_eq!(Homa::bump_era_frequency(), 0);
 
+		MockRelayBlockNumberProvider::set(10);
+
 		assert_ok!(Homa::update_bump_era_params(
 			Origin::signed(HomaAdmin::get()),
 			Some(10),
@@ -1147,6 +1149,7 @@ fn era_amount_should_to_bump_works() {
 		assert_eq!(Homa::era_amount_should_to_bump(11), 1);
 		assert_eq!(Homa::era_amount_should_to_bump(30), 3);
 
+		MockRelayBlockNumberProvider::set(10);
 		assert_ok!(Homa::update_bump_era_params(
 			Origin::signed(HomaAdmin::get()),
 			Some(1),
@@ -1359,3 +1362,53 @@ fn bump_current_era_works() {
 			);
 		});
 }
+
+#[test]
+fn last_era_bumped_block_config_check_works() {
+	ExtBuilder::default().build().execute_with(|| {
+		assert_eq!(Homa::last_era_bumped_block(), 0);
+		assert_eq!(Homa::bump_era_frequency(), 0);
+		assert_eq!(MockRelayBlockNumberProvider::current_block_number(), 0);
+
+		MockRelayBlockNumberProvider::set(100);
+
+		// it's ok, nothing happen because bump_era_frequency is zero
+		assert_ok!(Homa::update_bump_era_params(
+			Origin::signed(HomaAdmin::get()),
+			Some(100),
+			None,
+		));
+		assert_eq!(Homa::last_era_bumped_block(), 0);
+		assert_eq!(Homa::bump_era_frequency(), 0);
+
+		// 50 will trigger bump era
+		assert_noop!(
+			Homa::update_bump_era_params(Origin::signed(HomaAdmin::get()), Some(50), Some(50),),
+			Error::<Runtime>::InvalidLastEraBumpedBlock
+		);
+
+		assert_ok!(Homa::update_bump_era_params(
+			Origin::signed(HomaAdmin::get()),
+			Some(51),
+			Some(50),
+		));
+		assert_eq!(Homa::last_era_bumped_block(), 51);
+		assert_eq!(Homa::bump_era_frequency(), 50);
+		assert_eq!(MockRelayBlockNumberProvider::current_block_number(), 100);
+
+		// 101 is great than current relaychain block
+		assert_noop!(
+			Homa::update_bump_era_params(Origin::signed(HomaAdmin::get()), Some(101), None,),
+			Error::<Runtime>::InvalidLastEraBumpedBlock
+		);
+
+		assert_ok!(Homa::update_bump_era_params(
+			Origin::signed(HomaAdmin::get()),
+			Some(100),
+			None,
+		));
+		assert_eq!(Homa::last_era_bumped_block(), 100);
+		assert_eq!(Homa::bump_era_frequency(), 50);
+		assert_eq!(MockRelayBlockNumberProvider::current_block_number(), 100);
+	});
+}

runtime/mandala/src/benchmarking/homa.rs

@@ -133,7 +133,9 @@ runtime_benchmarks! {
 		Some(Rate::saturating_from_rational(1, 100)),
 		Some(Rate::saturating_from_rational(1, 100)))
 
-	update_bump_era_params {}: _(RawOrigin::Root, Some(3000), Some(7200))
+	update_bump_era_params {
+		RelaychainBlockNumberProvider::<Runtime>::set_block_number(10000);
+	}: _(RawOrigin::Root, Some(3000), Some(7200))
 
 	reset_ledgers {
 		let n in 0 .. 10;