Make ED of zero (kind of) work (#13655)

* Minimum of 1 for ED

* Avoid need for ED to be minimum one

* Docs

* Ban ED of zero unless feature enabled

* use integrity_test

* Docs

* Cleanup

* Update frame/balances/Cargo.toml

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Update frame/balances/src/lib.rs

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Update frame/balances/src/lib.rs

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Ensure dodgy code is disabled by default

* zero_ed -> insecure_zero_ed

---------

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: parity-processbot <>

substrate/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"]

substrate/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<CreditOf<Self, I>>;
 
-		/// 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<Self::Balance>;
 
@@ -445,6 +456,7 @@ pub mod pallet {
 	impl<T: Config<I>, I: 'static> GenesisBuild<T, I> for GenesisConfig<T, I> {
 		fn build(&self) {
 			let total = self.balances.iter().fold(Zero::zero(), |acc: T::Balance, &(_, n)| acc + n);
+
 			<TotalIssuance<T, I>>::put(total);
 
 			for (_, balance) in &self.balances {
@@ -492,6 +504,17 @@ pub mod pallet {
 		}
 	}
 
+	#[pallet::hooks]
+	impl<T: Config<I>, I: 'static> Hooks<T::BlockNumber> for Pallet<T, I> {
+		#[cfg(not(feature = "insecure_zero_ed"))]
+		fn integrity_test() {
+			assert!(
+				!<T as Config<I>>::ExistentialDeposit::get().is_zero(),
+				"The existential deposit must be greater than zero!"
+			);
+		}
+	}
+
 	#[pallet::call]
 	impl<T: Config<I>, I: 'static> Pallet<T, I> {
 		/// 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<T: Config<I>, I: 'static> Pallet<T, I> {
+		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::<T>::inc_providers(who);
 				}
 				let _ = system::Pallet::<T>::inc_consumers(who).defensive();
@@ -864,6 +890,20 @@ pub mod pallet {
 			Self::try_mutate_account(who, |a, _| -> Result<R, DispatchError> { 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::<T>::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

substrate/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::<Test>::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(|| {

substrate/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;

substrate/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;