diff --git a/substrate/frame/balances/src/impl_currency.rs b/substrate/frame/balances/src/impl_currency.rs index 8554fc6a19e5..2f8e443a2ede 100644 --- a/substrate/frame/balances/src/impl_currency.rs +++ b/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() { - >::mutate(|v| *v = v.saturating_add(self.0)); + super::Pallet::::mutate_total_issuance(|v| *v = v.saturating_add(self.0)); Pallet::::deposit_event(Event::::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() { - >::mutate(|v| *v = v.saturating_sub(self.0)); + super::Pallet::::mutate_total_issuance(|v| *v = v.saturating_sub(self.0)); Pallet::::deposit_event(Event::::Rescinded { amount: self.0 }); } } @@ -278,7 +278,7 @@ mod imbalances { for NegativeImbalance { fn handle(amount: T::Balance) { - >::mutate(|v| *v = v.saturating_sub(amount)); + super::Pallet::::mutate_total_issuance(|v| *v = v.saturating_sub(amount)); Pallet::::deposit_event(Event::::BurnedDebt { amount }); } } @@ -287,7 +287,7 @@ mod imbalances { for PositiveImbalance { fn handle(amount: T::Balance) { - >::mutate(|v| *v = v.saturating_add(amount)); + super::Pallet::::mutate_total_issuance(|v| *v = v.saturating_add(amount)); Pallet::::deposit_event(Event::::MintedCredit { amount }); } } @@ -339,7 +339,7 @@ where if amount.is_zero() { return PositiveImbalance::zero(); } - >::mutate(|issued| { + super::Pallet::::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(); } - >::mutate(|issued| { + super::Pallet::::mutate_total_issuance(|issued| { *issued = issued.checked_add(&amount).unwrap_or_else(|| { amount = Self::Balance::max_value() - *issued; Self::Balance::max_value() diff --git a/substrate/frame/balances/src/impl_fungible.rs b/substrate/frame/balances/src/impl_fungible.rs index 237c010ef2c4..3e23c12477ad 100644 --- a/substrate/frame/balances/src/impl_fungible.rs +++ b/substrate/frame/balances/src/impl_fungible.rs @@ -173,12 +173,12 @@ impl, I: 'static> fungible::Unbalanced for Pallet::mutate(|t| *t = amount); + Self::mutate_total_issuance(|ti| *ti = amount); } fn deactivate(amount: Self::Balance) { InactiveIssuance::::mutate(|b| { - // InactiveIssuance cannot be greater than TotalIssuance. + // The clamp is to account for a shrunken TI. *b = b.saturating_add(amount).min(TotalIssuance::::get()); }); } @@ -188,6 +188,21 @@ impl, I: 'static> fungible::Unbalanced for Pallet, I: 'static> Pallet { + /// Modify the Total Issuance and ensure the Inactive Issuance is bounded by it. + pub fn mutate_total_issuance(mutator: impl FnOnce(&mut >::Balance)) { + let mut ti = TotalIssuance::::get(); + mutator(&mut ti); + TotalIssuance::::set(ti); + + let ii = InactiveIssuance::::get(); + let clamped = ii.min(ti); + if clamped != ii { + InactiveIssuance::::put(clamped); + } + } +} + impl, I: 'static> fungible::Mutate for Pallet { fn done_mint_into(who: &T::AccountId, amount: Self::Balance) { Self::deposit_event(Event::::Minted { who: who.clone(), amount }); @@ -324,11 +339,17 @@ impl, I: 'static> fungible::UnbalancedHold 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::::insert(who, holds); + + if let Some(dust) = maybe_dust { + >::handle_raw_dust(dust); + } + + if holds.is_empty() { + Holds::::remove(who) + } else { + Holds::::insert(who, holds) + } + Ok(result) } } diff --git a/substrate/frame/balances/src/lib.rs b/substrate/frame/balances/src/lib.rs index 5f7fdb615e1e..77d2c70f778d 100644 --- a/substrate/frame/balances/src/lib.rs +++ b/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 { + >::handle_raw_dust(dust); } + if freezes.is_empty() { Freezes::::remove(who); } else { diff --git a/substrate/frame/balances/src/tests/fungible_tests.rs b/substrate/frame/balances/src/tests/fungible_tests.rs index 5027aad0930a..ebf474a9d14e 100644 --- a/substrate/frame/balances/src/tests/fungible_tests.rs +++ b/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::::get(), 50); + assert_eq!(Balances::active_issuance(), 0); + }); +} + #[test] fn unbalanced_trait_decrease_balance_simple_works() { ExtBuilder::default().build_and_execute_with(|| { diff --git a/substrate/frame/balances/src/tests/general_tests.rs b/substrate/frame/balances/src/tests/general_tests.rs index 606feedb9533..72d679ba7ab2 100644 --- a/substrate/frame/balances/src/tests/general_tests.rs +++ b/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::::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 = >::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::::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 = >::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() {