From efc69d8219b462f5cb8a726f24ee01d309382b95 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Fri, 10 Jul 2020 14:45:31 +0200 Subject: [PATCH] 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 Co-authored-by: Sergei Shulepov --- substrate/frame/contracts/src/exec.rs | 53 +++++++++++++++---- substrate/frame/contracts/src/lib.rs | 26 ++++++++- substrate/frame/contracts/src/rent.rs | 49 +++++++++-------- substrate/frame/contracts/src/tests.rs | 17 +++--- substrate/frame/contracts/src/wasm/runtime.rs | 18 +++++-- 5 files changed, 116 insertions(+), 47 deletions(-) diff --git a/substrate/frame/contracts/src/exec.rs b/substrate/frame/contracts/src/exec.rs index 27b843c5e1..67e2a4375e 100644 --- a/substrate/frame/contracts/src/exec.rs +++ b/substrate/frame/contracts/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 = ::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::::get(&self.self_account).is_none()); + TransactorKind::PlainAccount + } else { + TransactorKind::Contract + } + } } #[cfg_attr(test, derive(Debug, PartialEq, Eq))] @@ -519,6 +541,7 @@ enum TransferCause { fn transfer<'a, T: Trait, V: Vm, L: Loader>( gas_meter: &mut GasMeter, cause: TransferCause, + origin: TransactorKind, transactor: &T::AccountId, dest: &T::AccountId, value: BalanceOf, @@ -526,6 +549,7 @@ fn transfer<'a, T: Trait, V: Vm, L: Loader>( ) -> 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, L: Loader>( 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::::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, diff --git a/substrate/frame/contracts/src/lib.rs b/substrate/frame/contracts/src/lib.rs index c00e07c062..18c88c02b4 100644 --- a/substrate/frame/contracts/src/lib.rs +++ b/substrate/frame/contracts/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 Config { 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 { + 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::Currency::minimum_balance().saturating_add(T::TombstoneDeposit::get()) + } } /// Definition of the cost schedule and other parameterizations for wasm vm. diff --git a/substrate/frame/contracts/src/rent.rs b/substrate/frame/contracts/src/rent.rs index 6afd85aa8e..a3f582810a 100644 --- a/substrate/frame/contracts/src/rent.rs +++ b/substrate/frame/contracts/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 { /// 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( - balance: &BalanceOf, + free_balance: &BalanceOf, contract: &AliveContractInfo, ) -> BalanceOf { - 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( .unwrap_or(>::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() -> BalanceOf { - 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( - balance: &BalanceOf, + total_balance: &BalanceOf, + free_balance: &BalanceOf, contract: &AliveContractInfo, ) -> Option> { - let subsistence_threshold = subsistence_threshold::(); - if *balance < subsistence_threshold { + let subsistence_threshold = Config::::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(>::min( contract.rent_allowance, rent_allowed_to_charge, @@ -158,21 +155,22 @@ fn consider_case( 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::(&balance, contract); + let fee_per_block = compute_fee_per_block::(&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::(&balance, contract) { + let rent_budget = match rent_budget::(&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( 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( }; // 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::(&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::(&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::(&balance, &alive_contract_info).expect( + let rent_budget = rent_budget::(&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`; diff --git a/substrate/frame/contracts/src/tests.rs b/substrate/frame/contracts/src/tests.rs index 7af514f5dc..aac191e76e 100644 --- a/substrate/frame/contracts/src/tests.rs +++ b/substrate/frame/contracts/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::("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() + ::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(), ::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::::get(BOB).is_none()); - assert_eq!(Balances::free_balance(BOB), Balances::minimum_balance()); + assert_matches!(ContractInfoOf::::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::::get(BOB).is_none()); - assert_eq!(Balances::free_balance(BOB), Balances::minimum_balance()); + assert_matches!(ContractInfoOf::::get(BOB), Some(ContractInfo::Tombstone(_))); + assert_eq!(Balances::free_balance(BOB), subsistence_threshold); }); } diff --git a/substrate/frame/contracts/src/wasm/runtime.rs b/substrate/frame/contracts/src/wasm/runtime.rs index 6d272ce929..a221e3c7cf 100644 --- a/substrate/frame/contracts/src/wasm/runtime.rs +++ b/substrate/frame/contracts/src/wasm/runtime.rs @@ -379,10 +379,7 @@ fn write_sandbox_output( let len: u32 = read_sandbox_memory_as(ctx, out_len_ptr, 4)?; if len < buf_len { - ctx.trap_reason = Some(TrapReason::SupervisorError( - Error::::OutputBufferTooSmall.into() - )); - return Err(sp_sandbox::HostError); + Err(map_err(ctx, Error::::OutputBufferTooSmall))? } charge_gas( @@ -398,6 +395,17 @@ fn write_sandbox_output( Ok(()) } +/// Stores a DispatchError returned from an Ext function into the trap_reason. +/// +/// This allows through supervisor generated errors to the caller. +fn map_err(ctx: &mut Runtime, err: Error) -> sp_sandbox::HostError where + E: Ext, + Error: Into, +{ + ctx.trap_reason = Some(TrapReason::SupervisorError(err.into())); + sp_sandbox::HostError +} + // *********************************************************** // * AFTER MAKING A CHANGE MAKE SURE TO UPDATE COMPLEXITY.MD * // *********************************************************** @@ -517,7 +525,7 @@ define_env!(Env, , let value: BalanceOf<::T> = read_sandbox_memory_as(ctx, value_ptr, value_len)?; - ctx.ext.transfer(&callee, value, ctx.gas_meter).map_err(|_| sp_sandbox::HostError) + ctx.ext.transfer(&callee, value, ctx.gas_meter).map_err(|e| map_err(ctx, e)) }, // Make a call to another contract.