[FRAME] Small Balance Fixes

substrate/frame/balances/src/impl_currency.rs

@@ -258,7 +258,7 @@ mod imbalances {
 		/// Basic drop handler will just square up the total issuance.
 		fn drop(&mut self) {
 			if !self.0.is_zero() {
-				<super::TotalIssuance<T, I>>::mutate(|v| *v = v.saturating_add(self.0));
+				super::Pallet::<T, I>::mutate_total_issuance(|v| *v = v.saturating_add(self.0));
 				Pallet::<T, I>::deposit_event(Event::<T, I>::Issued { amount: self.0 });
 			}
 		}
@@ -268,7 +268,7 @@ mod imbalances {
 		/// Basic drop handler will just square up the total issuance.
 		fn drop(&mut self) {
 			if !self.0.is_zero() {
-				<super::TotalIssuance<T, I>>::mutate(|v| *v = v.saturating_sub(self.0));
+				super::Pallet::<T, I>::mutate_total_issuance(|v| *v = v.saturating_sub(self.0));
 				Pallet::<T, I>::deposit_event(Event::<T, I>::Rescinded { amount: self.0 });
 			}
 		}
@@ -278,7 +278,7 @@ mod imbalances {
 		for NegativeImbalance<T, I>
 	{
 		fn handle(amount: T::Balance) {
-			<super::TotalIssuance<T, I>>::mutate(|v| *v = v.saturating_sub(amount));
+			super::Pallet::<T, I>::mutate_total_issuance(|v| *v = v.saturating_sub(amount));
 			Pallet::<T, I>::deposit_event(Event::<T, I>::BurnedDebt { amount });
 		}
 	}
@@ -287,7 +287,7 @@ mod imbalances {
 		for PositiveImbalance<T, I>
 	{
 		fn handle(amount: T::Balance) {
-			<super::TotalIssuance<T, I>>::mutate(|v| *v = v.saturating_add(amount));
+			super::Pallet::<T, I>::mutate_total_issuance(|v| *v = v.saturating_add(amount));
 			Pallet::<T, I>::deposit_event(Event::<T, I>::MintedCredit { amount });
 		}
 	}
@@ -339,7 +339,7 @@ where
 		if amount.is_zero() {
 			return PositiveImbalance::zero();
 		}
-		<TotalIssuance<T, I>>::mutate(|issued| {
+		super::Pallet::<T, I>::mutate_total_issuance(|issued| {
 			*issued = issued.checked_sub(&amount).unwrap_or_else(|| {
 				amount = *issued;
 				Zero::zero()
@@ -357,7 +357,7 @@ where
 		if amount.is_zero() {
 			return NegativeImbalance::zero();
 		}
-		<TotalIssuance<T, I>>::mutate(|issued| {
+		super::Pallet::<T, I>::mutate_total_issuance(|issued| {
 			*issued = issued.checked_add(&amount).unwrap_or_else(|| {
 				amount = Self::Balance::max_value() - *issued;
 				Self::Balance::max_value()

substrate/frame/balances/src/impl_fungible.rs

@@ -173,12 +173,12 @@ impl<T: Config<I>, I: 'static> fungible::Unbalanced<T::AccountId> for Pallet<T,
 	}
 
 	fn set_total_issuance(amount: Self::Balance) {
-		TotalIssuance::<T, I>::mutate(|t| *t = amount);
+		Self::mutate_total_issuance(|ti| *ti = amount);
 	}
 
 	fn deactivate(amount: Self::Balance) {
 		InactiveIssuance::<T, I>::mutate(|b| {
-			// InactiveIssuance cannot be greater than TotalIssuance.
+			// The clamp is to account for a shrunken TI.
 			*b = b.saturating_add(amount).min(TotalIssuance::<T, I>::get());
 		});
 	}
@@ -188,6 +188,21 @@ impl<T: Config<I>, I: 'static> fungible::Unbalanced<T::AccountId> for Pallet<T,
 	}
 }
 
+impl<T: Config<I>, I: 'static> Pallet<T, I> {
+	/// Modify the Total Issuance and ensure the Inactive Issuance is bounded by it.
+	pub fn mutate_total_issuance(mutator: impl FnOnce(&mut <T as Config<I>>::Balance)) {
+		let mut ti = TotalIssuance::<T, I>::get();
+		mutator(&mut ti);
+		TotalIssuance::<T, I>::set(ti);
+
+		let ii = InactiveIssuance::<T, I>::get();
+		let clamped = ii.min(ti);
+		if clamped != ii {
+			InactiveIssuance::<T, I>::put(clamped);
+		}
+	}
+}
+
 impl<T: Config<I>, I: 'static> fungible::Mutate<T::AccountId> for Pallet<T, I> {
 	fn done_mint_into(who: &T::AccountId, amount: Self::Balance) {
 		Self::deposit_event(Event::<T, I>::Minted { who: who.clone(), amount });
@@ -324,11 +339,17 @@ impl<T: Config<I>, I: 'static> fungible::UnbalancedHold<T::AccountId> for Pallet
 				*a = new_account;
 				Ok(())
 			})?;
-		debug_assert!(
-			maybe_dust.is_none(),
-			"Does not alter main balance; dust only happens when it is altered; qed"
-		);
-		Holds::<T, I>::insert(who, holds);
+
+		if let Some(dust) = maybe_dust {
+			<Self as fungible::Unbalanced<_>>::handle_raw_dust(dust);
+		}
+
+		if holds.is_empty() {
+			Holds::<T, I>::remove(who)
+		} else {
+			Holds::<T, I>::insert(who, holds)
+		}
+
 		Ok(result)
 	}
 }

substrate/frame/balances/src/lib.rs

@@ -1125,10 +1125,10 @@ pub mod pallet {
 				// Evaluates `maybe_dust`, which is `Some` containing the dust to be dropped, iff
 				// some dust should be dropped.
 				//
-				// We should never be dropping if reserved is non-zero. Reserved being non-zero
-				// should imply that we have a consumer ref, so this is economically safe.
+				// We should never be dropping if there are either reserves or freezes since that
+				// implies a consumer ref.
 				let ed = Self::ed();
-				let maybe_dust = if account.free < ed && account.reserved.is_zero() {
+				let maybe_dust = if account.free < ed && !does_consume {
 					if account.free.is_zero() {
 						None
 					} else {
@@ -1230,10 +1230,11 @@ pub mod pallet {
 				}
 				after_frozen = b.frozen;
 			})?;
-			if maybe_dust.is_some() {
-				Self::deposit_event(Event::Unexpected(UnexpectedKind::BalanceUpdated));
-				defensive!("caused unexpected dusting/balance update.");
+
+			if let Some(dust) = maybe_dust {
+				<Self as fungible::Unbalanced<_>>::handle_raw_dust(dust);
 			}
+
 			if freezes.is_empty() {
 				Freezes::<T, I>::remove(who);
 			} else {

substrate/frame/balances/src/tests/fungible_tests.rs

@@ -118,6 +118,22 @@ fn unbalanced_trait_set_total_issuance_works() {
 	});
 }
 
+#[test]
+fn inactive_issuance_clamped_when_total_issuance_shrinks_below_it() {
+	ExtBuilder::default().build_and_execute_with(|| {
+		assert_ok!(Balances::mint_into(&1, 120));
+		Balances::deactivate(60);
+		assert_eq!(Balances::active_issuance(), 60);
+
+		// Shrinks TI to 50 < 60 deactivated: the stale excess is clamped away.
+		assert_ok!(Balances::burn_from(&1, 70, Expendable, Exact, Force));
+
+		assert_eq!(Balances::total_issuance(), 50);
+		assert_eq!(crate::InactiveIssuance::<Test>::get(), 50);
+		assert_eq!(Balances::active_issuance(), 0);
+	});
+}
+
 #[test]
 fn unbalanced_trait_decrease_balance_simple_works() {
 	ExtBuilder::default().build_and_execute_with(|| {

substrate/frame/balances/src/tests/general_tests.rs

@@ -20,15 +20,16 @@
 use crate::{
 	system::AccountInfo,
 	tests::{
-		ensure_ti_valid, get_test_account, Balances, ExtBuilder, System, Test, TestId, UseSystem,
+		ensure_ti_valid, get_test_account, get_test_account_data, Balances, ExtBuilder, System,
+		Test, TestId, UseSystem,
 	},
 	AccountData, ExtraFlags, TotalIssuance,
 };
 use frame_support::{
 	assert_noop, assert_ok, hypothetically,
 	traits::{
-		fungible::{Mutate, MutateHold},
-		tokens::Precision,
+		fungible::{self, InspectFreeze, Mutate, MutateFreeze, MutateHold},
+		tokens::{Fortitude, Precision, Preservation},
 	},
 };
 use sp_runtime::DispatchError;
@@ -112,6 +113,112 @@ fn regression_historic_acc_does_not_evaporate_reserve() {
 	});
 }
 
+/// Releasing the last hold from an account dusted below the ED must not leak the
+/// dust from `TotalIssuance` (`set_balance_on_hold` discards `maybe_dust`).
+#[test]
+fn regression_release_hold_below_ed_leaks_issuance() {
+	ExtBuilder::default().existential_deposit(10).build_and_execute_with(|| {
+		UseSystem::set(true);
+
+		let (alice, bob) = (0, 1);
+		Balances::set_balance(&alice, 100);
+		TotalIssuance::<Test>::put(100);
+
+		let _ = System::inc_providers(&alice);
+		assert_ok!(Balances::hold(&TestId::Foo, &alice, 50));
+
+		// free -> 5 (< ED), kept alive by the reserve.
+		assert_ok!(Balances::transfer_allow_death(Some(alice).into(), bob, 45));
+
+		// Releasing the last hold dusts the 5 free units; the dust must be burned.
+		assert_ok!(Balances::release(&TestId::Foo, &alice, 50, Precision::Exact));
+
+		ensure_ti_valid();
+	});
+}
+
+/// Dusting a frozen account (via a `Force` slash) must not desync `Freezes` from
+/// the account's `frozen` field.
+#[test]
+fn regression_force_withdraw_dusting_frozen_account_desyncs_freezes() {
+	ExtBuilder::default().existential_deposit(10).build_and_execute_with(|| {
+		UseSystem::set(true);
+		let alice = 0;
+
+		Balances::set_balance(&alice, 100);
+		let _ = System::inc_providers(&alice); // survives being dusted below ED
+		assert_ok!(Balances::set_freeze(&TestId::Foo, &alice, 30));
+
+		// `Force` ignores the freeze: free drops to 5 (< ED), dusting the account.
+		let credit = <Balances as fungible::Balanced<_>>::withdraw(
+			&alice,
+			95,
+			Precision::Exact,
+			Preservation::Expendable,
+			Fortitude::Force,
+		)
+		.unwrap();
+		drop(credit);
+
+		assert!(
+			Balances::balance_frozen(&TestId::Foo, &alice) <= get_test_account_data(alice).frozen
+		);
+	});
+}
+
+/// Same freeze desync as above, but reachable by an unprivileged
+/// `transfer_allow_death`: a freeze smaller than the ED lets a plain transfer
+/// push free below the ED (but above the freeze) and dust the account.
+#[test]
+fn regression_transfer_allow_death_dusting_frozen_account_desyncs_freezes() {
+	ExtBuilder::default().existential_deposit(10).build_and_execute_with(|| {
+		UseSystem::set(true);
+		let (alice, bob) = (0, 1);
+
+		Balances::set_balance(&alice, 100);
+		let _ = System::inc_providers(&alice); // survives being dusted below ED
+		assert_ok!(Balances::set_freeze(&TestId::Foo, &alice, 5));
+
+		// Leaves free = 7: below ED (10) but at/above the freeze (5), so allowed.
+		assert_ok!(Balances::transfer_allow_death(Some(alice).into(), bob, 93));
+
+		assert!(
+			Balances::balance_frozen(&TestId::Foo, &alice) <= get_test_account_data(alice).frozen
+		);
+	});
+}
+
+/// Thawing the last freeze of an account dusted below the ED must not leak the
+/// dust from `TotalIssuance` (`update_freezes` discards `maybe_dust`).
+#[test]
+fn regression_thaw_dusting_sub_ed_account_leaks_issuance() {
+	ExtBuilder::default().existential_deposit(10).build_and_execute_with(|| {
+		UseSystem::set(true);
+		let alice = 0;
+
+		Balances::set_balance(&alice, 100);
+		TotalIssuance::<Test>::put(100);
+		// This provider makes the withdraw below work
+		let _ = System::inc_providers(&alice);
+		assert_ok!(Balances::set_freeze(&TestId::Foo, &alice, 30));
+
+		// `Force` slash ignores the freeze: free -> 5 (< ED), kept alive by the freeze.
+		let credit = <Balances as fungible::Balanced<_>>::withdraw(
+			&alice,
+			95,
+			Precision::Exact,
+			Preservation::Expendable,
+			Fortitude::Force,
+		)
+		.unwrap();
+		drop(credit);
+
+		// Thawing the last freeze dusts the 5 free units; the dust must be burned.
+		assert_ok!(Balances::thaw(&TestId::Foo, &alice));
+		ensure_ti_valid();
+	});
+}
+
 #[cfg(feature = "try-runtime")]
 #[test]
 fn try_state_works() {