diff --git a/frame/balances/Cargo.toml b/frame/balances/Cargo.toml index 15ef136aa3800..9646a934df00e 100644 --- a/frame/balances/Cargo.toml +++ b/frame/balances/Cargo.toml @@ -39,5 +39,7 @@ std = [ "sp-runtime/std", "sp-std/std", ] +# Enable support for setting the existential deposit to zero. +insecure_zero_ed = [] runtime-benchmarks = ["frame-benchmarking/runtime-benchmarks"] try-runtime = ["frame-support/try-runtime"] diff --git a/frame/balances/src/lib.rs b/frame/balances/src/lib.rs index ca8e86ef2f64a..460f2be37ff2d 100644 --- a/frame/balances/src/lib.rs +++ b/frame/balances/src/lib.rs @@ -151,6 +151,10 @@ //! ## Assumptions //! //! * Total issued balanced of all accounts should be less than `Config::Balance::max_value()`. +//! * Existential Deposit is set to a value greater than zero. +//! +//! Note, you may find the Balances pallet still functions with an ED of zero in some circumstances, +//! however this is not a configuration which is generally supported, nor will it be. #![cfg_attr(not(feature = "std"), no_std)] mod benchmarking; @@ -231,7 +235,14 @@ pub mod pallet { /// Handler for the unbalanced reduction when removing a dust account. type DustRemoval: OnUnbalanced>; - /// The minimum amount required to keep an account open. + /// The minimum amount required to keep an account open. MUST BE GREATER THAN ZERO! + /// + /// If you *really* need it to be zero, you can enable the feature `insecure_zero_ed` for + /// this pallet. However, you do so at your own risk: this will open up a major DoS vector. + /// In case you have multiple sources of provider references, you may also get unexpected + /// behaviour if you set this to zero. + /// + /// Bottom line: Do yourself a favour and make it at least one! #[pallet::constant] type ExistentialDeposit: Get; @@ -445,6 +456,7 @@ pub mod pallet { impl, I: 'static> GenesisBuild for GenesisConfig { fn build(&self) { let total = self.balances.iter().fold(Zero::zero(), |acc: T::Balance, &(_, n)| acc + n); + >::put(total); for (_, balance) in &self.balances { @@ -492,6 +504,17 @@ pub mod pallet { } } + #[pallet::hooks] + impl, I: 'static> Hooks for Pallet { + #[cfg(not(feature = "insecure_zero_ed"))] + fn integrity_test() { + assert!( + !>::ExistentialDeposit::get().is_zero(), + "The existential deposit must be greater than zero!" + ); + } + } + #[pallet::call] impl, I: 'static> Pallet { /// Transfer some liquid free balance to another account. @@ -533,7 +556,7 @@ pub mod pallet { ) -> DispatchResultWithPostInfo { ensure_root(origin)?; let who = T::Lookup::lookup(who)?; - let existential_deposit = T::ExistentialDeposit::get(); + let existential_deposit = Self::ed(); let wipeout = new_free < existential_deposit; let new_free = if wipeout { Zero::zero() } else { new_free }; @@ -716,7 +739,7 @@ pub mod pallet { ) -> DispatchResultWithPostInfo { ensure_root(origin)?; let who = T::Lookup::lookup(who)?; - let existential_deposit = T::ExistentialDeposit::get(); + let existential_deposit = Self::ed(); let wipeout = new_free < existential_deposit; let new_free = if wipeout { Zero::zero() } else { new_free }; @@ -742,6 +765,9 @@ pub mod pallet { } impl, I: 'static> Pallet { + fn ed() -> T::Balance { + T::ExistentialDeposit::get() + } /// Ensure the account `who` is using the new logic. /// /// Returns `true` if the account did get upgraded, `false` if it didn't need upgrading. @@ -756,7 +782,7 @@ pub mod pallet { // Gah!! We have a non-zero reserve balance but no provider refs :( // This shouldn't practically happen, but we need a failsafe anyway: let's give // them enough for an ED. - a.free = a.free.min(T::ExistentialDeposit::get()); + a.free = a.free.min(Self::ed()); system::Pallet::::inc_providers(who); } let _ = system::Pallet::::inc_consumers(who).defensive(); @@ -864,6 +890,20 @@ pub mod pallet { Self::try_mutate_account(who, |a, _| -> Result { Ok(f(a)) }) } + /// Returns `true` when `who` has some providers or `insecure_zero_ed` feature is disnabled. + /// Returns `false` otherwise. + #[cfg(not(feature = "insecure_zero_ed"))] + fn have_providers_or_no_zero_ed(_: &T::AccountId) -> bool { + true + } + + /// Returns `true` when `who` has some providers or `insecure_zero_ed` feature is disnabled. + /// Returns `false` otherwise. + #[cfg(feature = "insecure_zero_ed")] + fn have_providers_or_no_zero_ed(who: &T::AccountId) -> bool { + frame_system::Pallet::::providers(who) > 0 + } + /// Mutate an account to some new value, or delete it entirely with `None`. Will enforce /// `ExistentialDeposit` law, annulling the account as needed. This will do nothing if the /// result of `f` is an `Err`. @@ -885,13 +925,14 @@ pub mod pallet { let result = T::AccountStore::try_mutate_exists(who, |maybe_account| { let is_new = maybe_account.is_none(); let mut account = maybe_account.take().unwrap_or_default(); - let did_provide = account.free >= T::ExistentialDeposit::get(); + let did_provide = + account.free >= Self::ed() && Self::have_providers_or_no_zero_ed(who); let did_consume = !is_new && (!account.reserved.is_zero() || !account.frozen.is_zero()); let result = f(&mut account, is_new)?; - let does_provide = account.free >= T::ExistentialDeposit::get(); + let does_provide = account.free >= Self::ed(); let does_consume = !account.reserved.is_zero() || !account.frozen.is_zero(); if !did_provide && does_provide { @@ -930,7 +971,7 @@ pub mod pallet { // // 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. - let ed = T::ExistentialDeposit::get(); + let ed = Self::ed(); let maybe_dust = if account.free < ed && account.reserved.is_zero() { if account.free.is_zero() { None diff --git a/frame/balances/src/tests/currency_tests.rs b/frame/balances/src/tests/currency_tests.rs index b0a38f4ac4ed7..f74e6bb8c4993 100644 --- a/frame/balances/src/tests/currency_tests.rs +++ b/frame/balances/src/tests/currency_tests.rs @@ -23,7 +23,8 @@ use frame_support::traits::{ BalanceStatus::{Free, Reserved}, Currency, ExistenceRequirement::{self, AllowDeath}, - LockIdentifier, LockableCurrency, NamedReservableCurrency, ReservableCurrency, WithdrawReasons, + Hooks, LockIdentifier, LockableCurrency, NamedReservableCurrency, ReservableCurrency, + WithdrawReasons, }; const ID_1: LockIdentifier = *b"1 "; @@ -974,7 +975,7 @@ fn slash_reserved_on_non_existant_works() { fn operations_on_dead_account_should_not_change_state() { // These functions all use `mutate_account` which may introduce a storage change when // the account never existed to begin with, and shouldn't exist in the end. - ExtBuilder::default().existential_deposit(0).build_and_execute_with(|| { + ExtBuilder::default().existential_deposit(1).build_and_execute_with(|| { assert!(!frame_system::Account::::contains_key(&1337)); // Unreserve @@ -993,6 +994,16 @@ fn operations_on_dead_account_should_not_change_state() { }); } +#[test] +#[should_panic = "The existential deposit must be greater than zero!"] +fn zero_ed_is_prohibited() { + // These functions all use `mutate_account` which may introduce a storage change when + // the account never existed to begin with, and shouldn't exist in the end. + ExtBuilder::default().existential_deposit(0).build_and_execute_with(|| { + Balances::integrity_test(); + }); +} + #[test] fn named_reserve_should_work() { ExtBuilder::default().build_and_execute_with(|| { diff --git a/frame/balances/src/tests/mod.rs b/frame/balances/src/tests/mod.rs index c4a8a631cad20..68e7e82035b0e 100644 --- a/frame/balances/src/tests/mod.rs +++ b/frame/balances/src/tests/mod.rs @@ -87,7 +87,7 @@ parameter_types! { frame_system::limits::BlockWeights::simple_max( frame_support::weights::Weight::from_parts(1024, u64::MAX), ); - pub static ExistentialDeposit: u64 = 0; + pub static ExistentialDeposit: u64 = 1; } impl frame_system::Config for Test { type BaseCallFilter = frame_support::traits::Everything; diff --git a/frame/vesting/src/mock.rs b/frame/vesting/src/mock.rs index be6afd4a60ec8..1adb36b730b1a 100644 --- a/frame/vesting/src/mock.rs +++ b/frame/vesting/src/mock.rs @@ -89,7 +89,7 @@ parameter_types! { pub const MinVestedTransfer: u64 = 256 * 2; pub UnvestedFundsAllowedWithdrawReasons: WithdrawReasons = WithdrawReasons::except(WithdrawReasons::TRANSFER | WithdrawReasons::RESERVE); - pub static ExistentialDeposit: u64 = 0; + pub static ExistentialDeposit: u64 = 1; } impl Config for Test { type BlockNumberToBalance = Identity;