seal: Prevent contracts from going below subsistence (#6623)

* seal: Do not allow transfers to bring total balance below subsistence deposit

This also reworks the rent system to take the total balance into account
when evaluating whether the account is above the subsistence deposit.

* Fix nits from review

* Fix typo

* Do not enforce subsistence when called from EOA

* Rename CallOrigin to TransactorKind

* Add debug asserts to check the invariants of a plain account transactor

* Fix typo

Co-authored-by: Sergei Shulepov <sergei@parity.io>

Co-authored-by: Sergei Shulepov <sergei@parity.io>

Author: athei

src/exec.rs

@@ -16,17 +16,15 @@
 
 use super::{CodeHash, Config, ContractAddressFor, Event, RawEvent, Trait,
 	TrieId, BalanceOf, ContractInfo, TrieIdGenerator};
-use crate::gas::{Gas, GasMeter, Token};
-use crate::rent;
-use crate::storage;
+use crate::{gas::{Gas, GasMeter, Token}, rent, storage, Error, ContractInfoOf};
 use bitflags::bitflags;
-
 use sp_std::prelude::*;
-use sp_runtime::traits::{Bounded, Zero, Convert};
+use sp_runtime::traits::{Bounded, Zero, Convert, Saturating};
 use frame_support::{
 	dispatch::DispatchError,
 	traits::{ExistenceRequirement, Currency, Time, Randomness},
 	weights::Weight,
+	ensure, StorageMap,
 };
 
 pub type AccountIdOf<T> = <T as frame_system::Trait>::AccountId;
@@ -46,6 +44,15 @@ bitflags! {
 	}
 }
 
+/// Describes whether we deal with a contract or a plain account.
+pub enum TransactorKind {
+	/// Transaction was initiated from a plain account. That can be either be through a
+	/// signed transaction or through RPC.
+	PlainAccount,
+	/// The call was initiated by a contract account.
+	Contract,
+}
+
 /// Output of a contract call or instantiation which ran to completion.
 #[cfg_attr(test, derive(PartialEq, Eq, Debug))]
 pub struct ExecReturnValue {
@@ -320,6 +327,7 @@ where
 			Err("contract has been evicted")?
 		};
 
+		let transactor_kind = self.transactor_kind();
 		let caller = self.self_account.clone();
 		let dest_trie_id = contract_info.and_then(|i| i.as_alive().map(|i| i.trie_id.clone()));
 
@@ -328,6 +336,7 @@ where
 				transfer(
 					gas_meter,
 					TransferCause::Call,
+					transactor_kind,
 					&caller,
 					&dest,
 					value,
@@ -372,6 +381,7 @@ where
 			Err("not enough gas to pay base instantiate fee")?
 		}
 
+		let transactor_kind = self.transactor_kind();
 		let caller = self.self_account.clone();
 		let dest = T::DetermineContractAddress::contract_address_for(
 			code_hash,
@@ -399,6 +409,7 @@ where
 			transfer(
 				gas_meter,
 				TransferCause::Instantiate,
+				transactor_kind,
 				&caller,
 				&dest,
 				endowment,
@@ -466,6 +477,17 @@ where
 		&self.self_account == account ||
 			self.caller.map_or(false, |caller| caller.is_live(account))
 	}
+
+	fn transactor_kind(&self) -> TransactorKind {
+		if self.depth == 0 {
+			debug_assert!(self.self_trie_id.is_none());
+			debug_assert!(self.caller.is_none());
+			debug_assert!(ContractInfoOf::<T>::get(&self.self_account).is_none());
+			TransactorKind::PlainAccount
+		} else {
+			TransactorKind::Contract
+		}
+	}
 }
 
 #[cfg_attr(test, derive(Debug, PartialEq, Eq))]
@@ -519,13 +541,15 @@ enum TransferCause {
 fn transfer<'a, T: Trait, V: Vm<T>, L: Loader<T>>(
 	gas_meter: &mut GasMeter<T>,
 	cause: TransferCause,
+	origin: TransactorKind,
 	transactor: &T::AccountId,
 	dest: &T::AccountId,
 	value: BalanceOf<T>,
 	ctx: &mut ExecutionContext<'a, T, V, L>,
 ) -> Result<(), DispatchError> {
 	use self::TransferCause::*;
 	use self::TransferFeeKind::*;
+	use self::TransactorKind::*;
 
 	let token = {
 		let kind: TransferFeeKind = match cause {
@@ -545,10 +569,19 @@ fn transfer<'a, T: Trait, V: Vm<T>, L: Loader<T>>(
 		Err("not enough gas to pay transfer fee")?
 	}
 
-	// Only ext_terminate is allowed to bring the sender below the existential deposit
-	let existence_requirement = match cause {
-		Terminate => ExistenceRequirement::AllowDeath,
-		_ => ExistenceRequirement::KeepAlive,
+	// Only ext_terminate is allowed to bring the sender below the subsistence
+	// threshold or even existential deposit.
+	let existence_requirement = match (cause, origin) {
+		(Terminate, _) => ExistenceRequirement::AllowDeath,
+		(_, Contract) => {
+			ensure!(
+				T::Currency::total_balance(transactor).saturating_sub(value) >=
+					ctx.config.subsistence_threshold(),
+				Error::<T>::InsufficientBalance,
+			);
+			ExistenceRequirement::KeepAlive
+		},
+		(_, PlainAccount) => ExistenceRequirement::KeepAlive,
 	};
 	T::Currency::transfer(transactor, dest, value, existence_requirement)?;
 
@@ -630,6 +663,7 @@ where
 		transfer(
 			gas_meter,
 			TransferCause::Call,
+			TransactorKind::Contract,
 			&self.ctx.self_account.clone(),
 			&to,
 			value,
@@ -654,6 +688,7 @@ where
 		transfer(
 			gas_meter,
 			TransferCause::Terminate,
+			TransactorKind::Contract,
 			&self_id,
 			beneficiary,
 			value,

src/lib.rs

@@ -102,7 +102,7 @@ use sp_std::{prelude::*, marker::PhantomData, fmt::Debug};
 use codec::{Codec, Encode, Decode};
 use sp_runtime::{
 	traits::{
-		Hash, StaticLookup, Zero, MaybeSerializeDeserialize, Member, Convert,
+		Hash, StaticLookup, Zero, MaybeSerializeDeserialize, Member, Convert, Saturating,
 	},
 	RuntimeDebug,
 };
@@ -415,6 +415,11 @@ decl_error! {
 		OutOfGas,
 		/// The output buffer supplied to a contract API call was too small.
 		OutputBufferTooSmall,
+		/// Performing the requested transfer would have brought the contract below
+		/// the subsistence threshold. No transfer is allowed to do this in order to allow
+		/// for a tombstone to be created. Use `ext_terminate` to remove a contract without
+		/// leaving a tombstone behind.
+		InsufficientBalance,
 	}
 }
 
@@ -726,6 +731,25 @@ impl<T: Trait> Config<T> {
 			max_value_size: T::MaxValueSize::get(),
 		}
 	}
+
+	/// Subsistence threshold is the extension of the minimum balance (aka existential deposit) by the
+	/// tombstone deposit, required for leaving a tombstone.
+	///
+	/// Rent or any contract initiated balance transfer mechanism cannot make the balance lower
+	/// than the subsistence threshold in order to guarantee that a tombstone is created.
+	///
+	/// The only way to completely kill a contract without a tombstone is calling `ext_terminate`.
+	fn subsistence_threshold(&self) -> BalanceOf<T> {
+		self.existential_deposit.saturating_add(self.tombstone_deposit)
+	}
+
+	/// The same as `subsistence_threshold` but without the need for a preloaded instance.
+	///
+	/// This is for cases where this value is needed in rent calculation rather than
+	/// during contract execution.
+	fn subsistence_threshold_uncached() -> BalanceOf<T> {
+		T::Currency::minimum_balance().saturating_add(T::TombstoneDeposit::get())
+	}
 }
 
 /// Definition of the cost schedule and other parameterizations for wasm vm.

src/rent.rs

@@ -18,7 +18,7 @@
 
 use crate::{
 	AliveContractInfo, BalanceOf, ContractInfo, ContractInfoOf, Module, RawEvent,
-	TombstoneContractInfo, Trait, CodeHash,
+	TombstoneContractInfo, Trait, CodeHash, Config
 };
 use sp_std::prelude::*;
 use sp_io::hashing::blake2_256;
@@ -87,10 +87,10 @@ enum Verdict<T: Trait> {
 /// This function accounts for the storage rent deposit. I.e. if the contract possesses enough funds
 /// then the fee can drop to zero.
 fn compute_fee_per_block<T: Trait>(
-	balance: &BalanceOf<T>,
+	free_balance: &BalanceOf<T>,
 	contract: &AliveContractInfo<T>,
 ) -> BalanceOf<T> {
-	let free_storage = balance
+	let free_storage = free_balance
 		.checked_div(&T::RentDepositOffset::get())
 		.unwrap_or_else(Zero::zero);
 
@@ -107,30 +107,27 @@ fn compute_fee_per_block<T: Trait>(
 		.unwrap_or(<BalanceOf<T>>::max_value())
 }
 
-/// Subsistence threshold is the extension of the minimum balance (aka existential deposit) by the
-/// tombstone deposit, required for leaving a tombstone.
-///
-/// Rent mechanism cannot make the balance lower than subsistence threshold.
-fn subsistence_threshold<T: Trait>() -> BalanceOf<T> {
-	T::Currency::minimum_balance() + T::TombstoneDeposit::get()
-}
-
 /// Returns amount of funds available to consume by rent mechanism.
 ///
 /// Rent mechanism cannot consume more than `rent_allowance` set by the contract and it cannot make
 /// the balance lower than [`subsistence_threshold`].
 ///
-/// In case the balance is below the subsistence threshold, this function returns `None`.
+/// In case the toal_balance is below the subsistence threshold, this function returns `None`.
 fn rent_budget<T: Trait>(
-	balance: &BalanceOf<T>,
+	total_balance: &BalanceOf<T>,
+	free_balance: &BalanceOf<T>,
 	contract: &AliveContractInfo<T>,
 ) -> Option<BalanceOf<T>> {
-	let subsistence_threshold = subsistence_threshold::<T>();
-	if *balance < subsistence_threshold {
+	let subsistence_threshold = Config::<T>::subsistence_threshold_uncached();
+	// Reserved balance contributes towards the subsistence threshold to stay consistent
+	// with the existential deposit where the reserved balance is also counted.
+	if *total_balance < subsistence_threshold {
 		return None;
 	}
 
-	let rent_allowed_to_charge = *balance - subsistence_threshold;
+	// However, reserved balance cannot be charged so we need to use the free balance
+	// to calculate the actual budget (which can be 0).
+	let rent_allowed_to_charge = free_balance.saturating_sub(subsistence_threshold);
 	Some(<BalanceOf<T>>::min(
 		contract.rent_allowance,
 		rent_allowed_to_charge,
@@ -158,21 +155,22 @@ fn consider_case<T: Trait>(
 		return Verdict::Exempt;
 	}
 
-	let balance = T::Currency::free_balance(account);
+	let total_balance = T::Currency::total_balance(account);
+	let free_balance = T::Currency::free_balance(account);
 
 	// An amount of funds to charge per block for storage taken up by the contract.
-	let fee_per_block = compute_fee_per_block::<T>(&balance, contract);
+	let fee_per_block = compute_fee_per_block::<T>(&free_balance, contract);
 	if fee_per_block.is_zero() {
 		// The rent deposit offset reduced the fee to 0. This means that the contract
 		// gets the rent for free.
 		return Verdict::Exempt;
 	}
 
-	let rent_budget = match rent_budget::<T>(&balance, contract) {
+	let rent_budget = match rent_budget::<T>(&total_balance, &free_balance, contract) {
 		Some(rent_budget) => rent_budget,
 		None => {
-			// The contract's balance is already below subsistence threshold. That indicates that
-			// the contract cannot afford to leave a tombstone.
+			// The contract's total balance is already below subsistence threshold. That
+			// indicates that the contract cannot afford to leave a tombstone.
 			//
 			// So cleanly wipe the contract.
 			return Verdict::Kill;
@@ -195,7 +193,7 @@ fn consider_case<T: Trait>(
 		account,
 		dues_limited,
 		WithdrawReason::Fee.into(),
-		balance.saturating_sub(dues_limited),
+		free_balance.saturating_sub(dues_limited),
 	)
 	.is_ok();
 
@@ -369,14 +367,15 @@ pub fn compute_rent_projection<T: Trait>(
 	};
 
 	// Compute how much would the fee per block be with the *updated* balance.
-	let balance = T::Currency::free_balance(account);
-	let fee_per_block = compute_fee_per_block::<T>(&balance, &alive_contract_info);
+	let total_balance = T::Currency::total_balance(account);
+	let free_balance = T::Currency::free_balance(account);
+	let fee_per_block = compute_fee_per_block::<T>(&free_balance, &alive_contract_info);
 	if fee_per_block.is_zero() {
 		return Ok(RentProjection::NoEviction);
 	}
 
 	// Then compute how much the contract will sustain under these circumstances.
-	let rent_budget = rent_budget::<T>(&balance, &alive_contract_info).expect(
+	let rent_budget = rent_budget::<T>(&total_balance, &free_balance, &alive_contract_info).expect(
 		"the contract exists and in the alive state;
 		the updated balance must be greater than subsistence deposit;
 		this function doesn't return `None`;

src/tests.rs

@@ -797,6 +797,7 @@ fn claim_surcharge(blocks: u64, trigger_call: impl Fn() -> bool, removes: bool)
 /// * if balance is reached and balance > subsistence threshold
 /// * if allowance is exceeded
 /// * if balance is reached and balance < subsistence threshold
+///	    * this case cannot be triggered by a contract: we check whether a tombstone is left
 fn removals(trigger_call: impl Fn() -> bool) {
 	let (wasm, code_hash) = compile_module::<Test>("set_rent").unwrap();
 
@@ -898,10 +899,12 @@ fn removals(trigger_call: impl Fn() -> bool) {
 		.execute_with(|| {
 			// Create
 			let _ = Balances::deposit_creating(&ALICE, 1_000_000);
+			let subsistence_threshold =
+				Balances::minimum_balance() + <Test as Trait>::TombstoneDeposit::get();
 			assert_ok!(Contracts::put_code(Origin::signed(ALICE), wasm.clone()));
 			assert_ok!(Contracts::instantiate(
 				Origin::signed(ALICE),
-				50 + Balances::minimum_balance(),
+				50 + subsistence_threshold,
 				GAS_LIMIT,
 				code_hash.into(),
 				<Test as pallet_balances::Trait>::Balance::from(1_000u32).encode() // rent allowance
@@ -919,7 +922,7 @@ fn removals(trigger_call: impl Fn() -> bool) {
 			);
 			assert_eq!(
 				Balances::free_balance(BOB),
-				50 + Balances::minimum_balance()
+				50 + subsistence_threshold,
 			);
 
 			// Transfer funds
@@ -938,23 +941,23 @@ fn removals(trigger_call: impl Fn() -> bool) {
 					.rent_allowance,
 				1_000
 			);
-			assert_eq!(Balances::free_balance(BOB), Balances::minimum_balance());
+			assert_eq!(Balances::free_balance(BOB), subsistence_threshold);
 
 			// Advance blocks
 			initialize_block(10);
 
 			// Trigger rent through call
 			assert!(trigger_call());
-			assert!(ContractInfoOf::<Test>::get(BOB).is_none());
-			assert_eq!(Balances::free_balance(BOB), Balances::minimum_balance());
+			assert_matches!(ContractInfoOf::<Test>::get(BOB), Some(ContractInfo::Tombstone(_)));
+			assert_eq!(Balances::free_balance(BOB), subsistence_threshold);
 
 			// Advance blocks
 			initialize_block(20);
 
 			// Trigger rent must have no effect
 			assert!(trigger_call());
-			assert!(ContractInfoOf::<Test>::get(BOB).is_none());
-			assert_eq!(Balances::free_balance(BOB), Balances::minimum_balance());
+			assert_matches!(ContractInfoOf::<Test>::get(BOB), Some(ContractInfo::Tombstone(_)));
+			assert_eq!(Balances::free_balance(BOB), subsistence_threshold);
 		});
 }