From 3cb1812ba4a2587bf0d8139da25e52083115602c Mon Sep 17 00:00:00 2001 From: librelois Date: Tue, 12 Oct 2021 14:24:34 +0200 Subject: [PATCH 1/5] add security tests --- frame/ethereum/src/mock.rs | 51 ++++++++++++++++++++++++++-- frame/ethereum/src/tests.rs | 66 ++++++++++++++++++++++++++++++++++--- 2 files changed, 109 insertions(+), 8 deletions(-) diff --git a/frame/ethereum/src/mock.rs b/frame/ethereum/src/mock.rs index 5956762dff..3b7d1439d1 100644 --- a/frame/ethereum/src/mock.rs +++ b/frame/ethereum/src/mock.rs @@ -19,6 +19,7 @@ use super::*; use crate::IntermediateStateRoot; +use codec::{WrapperTypeDecode, WrapperTypeEncode}; use ethereum::{TransactionAction, TransactionSignature}; use frame_support::{parameter_types, traits::FindAuthor, ConsensusEngineId, PalletId}; use pallet_evm::{AddressMapping, EnsureAddressTruncated, FeeCalculator}; @@ -27,11 +28,13 @@ use sha3::Digest; use sp_core::{H160, H256, U256}; use sp_runtime::{ testing::Header, - traits::{BlakeTwo256, IdentityLookup}, + traits::{BlakeTwo256, IdentityLookup, SignedExtension}, AccountId32, }; -type UncheckedExtrinsic = frame_system::mocking::MockUncheckedExtrinsic; +pub type SignedExtra = (frame_system::CheckSpecVersion,); + +type UncheckedExtrinsic = frame_system::mocking::MockUncheckedExtrinsic; type Block = frame_system::mocking::MockBlock; frame_support::construct_runtime! { @@ -166,6 +169,44 @@ impl crate::Config for Test { type StateRoot = IntermediateStateRoot; } +impl fp_self_contained::SelfContainedCall for Call { + type SignedInfo = H160; + + fn is_self_contained(&self) -> bool { + match self { + Call::Ethereum(call) => call.is_self_contained(), + _ => false, + } + } + + fn check_self_contained(&self) -> Option> { + match self { + Call::Ethereum(call) => call.check_self_contained(), + _ => None, + } + } + + fn validate_self_contained(&self, info: &Self::SignedInfo) -> Option { + match self { + Call::Ethereum(call) => call.validate_self_contained(info), + _ => None, + } + } + + fn apply_self_contained( + self, + info: Self::SignedInfo, + ) -> Option>> { + use sp_runtime::traits::Dispatchable as _; + match self { + call @ Call::Ethereum(crate::Call::transact(_)) => { + Some(call.dispatch(Origin::from(crate::RawOrigin::EthereumTransaction(info)))) + } + _ => None, + } + } +} + pub struct AccountInfo { pub address: H160, pub account_id: AccountId32, @@ -255,6 +296,10 @@ impl UnsignedTransaction { } pub fn sign(&self, key: &H256) -> Transaction { + self.sign_with_chain_id(key, ChainId::get()) + } + + pub fn sign_with_chain_id(&self, key: &H256, chain_id: u64) -> Transaction { let hash = self.signing_hash(); let msg = libsecp256k1::Message::parse(hash.as_fixed_bytes()); let s = libsecp256k1::sign( @@ -264,7 +309,7 @@ impl UnsignedTransaction { let sig = s.0.serialize(); let sig = TransactionSignature::new( - s.1.serialize() as u64 % 2 + ChainId::get() * 2 + 35, + s.1.serialize() as u64 % 2 + chain_id * 2 + 35, H256::from_slice(&sig[0..32]), H256::from_slice(&sig[32..64]), ) diff --git a/frame/ethereum/src/tests.rs b/frame/ethereum/src/tests.rs index b1aafcdcda..b793c7edb3 100644 --- a/frame/ethereum/src/tests.rs +++ b/frame/ethereum/src/tests.rs @@ -18,13 +18,18 @@ //! Consensus extension module tests for BABE consensus. use crate::{ - mock::*, CallOrCreateInfo, Error, RawOrigin, Transaction, TransactionAction, - ValidTransactionBuilder, H160, H256, U256, + mock::*, CallOrCreateInfo, Error, RawOrigin, Transaction, TransactionAction, H160, H256, U256, }; use ethereum::TransactionSignature; -use frame_support::{assert_err, assert_noop, assert_ok, unsigned::ValidateUnsigned}; +use frame_support::{ + assert_err, assert_noop, assert_ok, + unsigned::{TransactionValidityError, ValidateUnsigned}, +}; use rustc_hex::{FromHex, ToHex}; -use sp_runtime::transaction_validity::{InvalidTransaction, TransactionSource}; +use sp_runtime::traits::Applyable; +use sp_runtime::transaction_validity::{ + InvalidTransaction, TransactionSource, ValidTransactionBuilder, +}; use std::str::FromStr; // This ERC-20 contract mints the maximum amount of tokens to the contract creator. @@ -91,7 +96,7 @@ fn transaction_without_enough_gas_should_not_work() { } #[test] -fn transaction_with_invalid_nonce_should_not_work() { +fn transaction_with_to_low_nonce_should_not_work() { let (pairs, mut ext) = new_test_ext(1); let alice = &pairs[0]; @@ -140,6 +145,57 @@ fn transaction_with_invalid_nonce_should_not_work() { }); } +#[test] +fn transaction_with_to_hight_nonce_should_fail_in_block() { + let (pairs, mut ext) = new_test_ext(1); + let alice = &pairs[0]; + + ext.execute_with(|| { + let mut transaction = default_erc20_creation_unsigned_transaction(); + transaction.nonce = U256::one(); + + let signed = transaction.sign(&alice.private_key); + let call = crate::Call::::transact(signed); + let source = call.check_self_contained().unwrap().unwrap(); + let extrinsic = fp_self_contained::CheckedExtrinsic::<_, _, SignedExtra, _> { + signed: fp_self_contained::CheckedSignature::SelfContained(source), + function: Call::Ethereum(call), + }; + use frame_support::weights::GetDispatchInfo as _; + let dispatch_info = extrinsic.get_dispatch_info(); + assert_err!( + extrinsic.apply::(&dispatch_info, 0), + TransactionValidityError::Invalid(InvalidTransaction::Future) + ); + }); +} + +#[test] +fn transaction_with_invalid_chain_id_should_fail_in_block() { + let (pairs, mut ext) = new_test_ext(1); + let alice = &pairs[0]; + + ext.execute_with(|| { + let transaction = + default_erc20_creation_unsigned_transaction().sign_with_chain_id(&alice.private_key, 1); + + let call = crate::Call::::transact(transaction); + let source = call.check_self_contained().unwrap().unwrap(); + let extrinsic = fp_self_contained::CheckedExtrinsic::<_, _, SignedExtra, _> { + signed: fp_self_contained::CheckedSignature::SelfContained(source), + function: Call::Ethereum(call), + }; + use frame_support::weights::GetDispatchInfo as _; + let dispatch_info = extrinsic.get_dispatch_info(); + assert_err!( + extrinsic.apply::(&dispatch_info, 0), + TransactionValidityError::Invalid(InvalidTransaction::Custom( + crate::TransactionValidationError::InvalidChainId as u8, + )) + ); + }); +} + #[test] fn contract_constructor_should_get_executed() { let (pairs, mut ext) = new_test_ext(1); From 908d6ce26e9bb3907dd17a304fc22332fce3399c Mon Sep 17 00:00:00 2001 From: librelois Date: Tue, 12 Oct 2021 14:26:06 +0200 Subject: [PATCH 2/5] add pre_dispatch phase to fiy security issues --- frame/ethereum/src/lib.rs | 201 +++++++++++------- frame/ethereum/src/mock.rs | 12 ++ frame/ethereum/src/tests.rs | 8 - frame/evm/src/lib.rs | 29 +-- frame/evm/src/runner/mod.rs | 3 - frame/evm/src/runner/stack.rs | 102 +++------ .../self-contained/src/checked_extrinsic.rs | 6 + primitives/self-contained/src/lib.rs | 17 ++ template/runtime/src/lib.rs | 18 +- 9 files changed, 213 insertions(+), 183 deletions(-) diff --git a/frame/ethereum/src/lib.rs b/frame/ethereum/src/lib.rs index ee2ffd6fe6..d592c642fa 100644 --- a/frame/ethereum/src/lib.rs +++ b/frame/ethereum/src/lib.rs @@ -120,76 +120,10 @@ where pub fn validate_self_contained(&self, origin: &H160) -> Option { if let Call::transact(transaction) = self { - let validate = || { - // We must ensure a transaction can pay the cost of its data bytes. - // If it can't it should not be included in a block. - let mut gasometer = evm::gasometer::Gasometer::new( - transaction.gas_limit.low_u64(), - ::config(), - ); - let transaction_cost = match transaction.action { - TransactionAction::Call(_) => { - evm::gasometer::call_transaction_cost(&transaction.input) - } - TransactionAction::Create => { - evm::gasometer::create_transaction_cost(&transaction.input) - } - }; - if gasometer.record_transaction(transaction_cost).is_err() { - return InvalidTransaction::Custom( - TransactionValidationError::InvalidGasLimit as u8, - ) - .into(); - } - - if let Some(chain_id) = transaction.signature.chain_id() { - if chain_id != T::ChainId::get() { - return InvalidTransaction::Custom( - TransactionValidationError::InvalidChainId as u8, - ) - .into(); - } - } - - if transaction.gas_limit >= T::BlockGasLimit::get() { - return InvalidTransaction::Custom( - TransactionValidationError::InvalidGasLimit as u8, - ) - .into(); - } - - let account_data = pallet_evm::Pallet::::account_basic(&origin); - - if transaction.nonce < account_data.nonce { - return InvalidTransaction::Stale.into(); - } - - let fee = transaction.gas_price.saturating_mul(transaction.gas_limit); - let total_payment = transaction.value.saturating_add(fee); - if account_data.balance < total_payment { - return InvalidTransaction::Payment.into(); - } - - let min_gas_price = T::FeeCalculator::min_gas_price(); - - if transaction.gas_price < min_gas_price { - return InvalidTransaction::Payment.into(); - } - - let mut builder = ValidTransactionBuilder::default() - .and_provides((origin, transaction.nonce)) - .priority(transaction.gas_price.unique_saturated_into()); - - if transaction.nonce > account_data.nonce { - if let Some(prev_nonce) = transaction.nonce.checked_sub(1.into()) { - builder = builder.and_requires((origin, prev_nonce)) - } - } - - builder.build() - }; - - Some(validate()) + Some(Pallet::::validate_transaction_in_pool( + *origin, + transaction, + )) } else { None } @@ -257,9 +191,12 @@ pub mod pallet { "pre-block transaction signature invalid; the block cannot be built", ); - Self::do_transact(source, transaction).expect( + Self::validate_transaction_in_block(source, &transaction).expect( "pre-block transaction verification failed; the block cannot be built", ); + Self::apply_validated_transaction(source, transaction).expect( + "pre-block transaction execution failed; the block cannot be built", + ); } } @@ -280,7 +217,7 @@ pub mod pallet { ) -> DispatchResultWithPostInfo { let source = ensure_ethereum_transaction(origin)?; - Self::do_transact(source, transaction) + Self::apply_validated_transaction(source, transaction) } } @@ -411,7 +348,98 @@ impl Pallet { } } - fn do_transact(source: H160, transaction: Transaction) -> DispatchResultWithPostInfo { + // Common controls to be performed in the same way by the pool and the + // State Transition Function (STF). + // This is the case for all controls except those concerning the nonce. + fn validate_transaction_common( + origin: H160, + transaction: &Transaction, + ) -> Result { + // We must ensure a transaction can pay the cost of its data bytes. + // If it can't it should not be included in a block. + let mut gasometer = evm::gasometer::Gasometer::new( + transaction.gas_limit.low_u64(), + ::config(), + ); + let transaction_cost = match transaction.action { + TransactionAction::Call(_) => evm::gasometer::call_transaction_cost(&transaction.input), + TransactionAction::Create => { + evm::gasometer::create_transaction_cost(&transaction.input) + } + }; + if gasometer.record_transaction(transaction_cost).is_err() { + return Err(InvalidTransaction::Custom( + TransactionValidationError::InvalidGasLimit as u8, + ) + .into()); + } + + if let Some(chain_id) = transaction.signature.chain_id() { + if chain_id != T::ChainId::get() { + return Err(InvalidTransaction::Custom( + TransactionValidationError::InvalidChainId as u8, + ) + .into()); + } + } + + if transaction.gas_limit >= T::BlockGasLimit::get() { + return Err(InvalidTransaction::Custom( + TransactionValidationError::InvalidGasLimit as u8, + ) + .into()); + } + + let account_data = pallet_evm::Pallet::::account_basic(&origin); + + let fee = transaction.gas_price.saturating_mul(transaction.gas_limit); + let total_payment = transaction.value.saturating_add(fee); + if account_data.balance < total_payment { + return Err(InvalidTransaction::Payment.into()); + } + + let min_gas_price = T::FeeCalculator::min_gas_price(); + + if transaction.gas_price < min_gas_price { + return Err(InvalidTransaction::Payment.into()); + } + + Ok(account_data.nonce) + } + + // Controls that must be performed by the pool. + // The controls common with the State Transition Function (STF) are in + // the function `validate_transaction_common`. + fn validate_transaction_in_pool( + origin: H160, + transaction: &Transaction, + ) -> TransactionValidity { + let account_nonce = Self::validate_transaction_common(origin, transaction)?; + + if transaction.nonce < account_nonce { + return Err(InvalidTransaction::Stale.into()); + } + + // The tag provides and requires must be filled correctly according to the nonce. + let mut builder = ValidTransactionBuilder::default() + .and_provides((origin, transaction.nonce)) + .priority(transaction.gas_price.unique_saturated_into()); + + // In the context of the pool, a transaction with + // too high a nonce is still considered valid + if transaction.nonce > account_nonce { + if let Some(prev_nonce) = transaction.nonce.checked_sub(1.into()) { + builder = builder.and_requires((origin, prev_nonce)) + } + } + + builder.build() + } + + fn apply_validated_transaction( + source: H160, + transaction: Transaction, + ) -> DispatchResultWithPostInfo { ensure!( fp_consensus::find_pre_log(&frame_system::Pallet::::digest()).is_err(), Error::::PreLogExists, @@ -427,7 +455,6 @@ impl Pallet { transaction.value, transaction.gas_limit, Some(transaction.gas_price), - Some(transaction.nonce), transaction.action, None, )?; @@ -527,7 +554,6 @@ impl Pallet { value: U256, gas_limit: U256, gas_price: Option, - nonce: Option, action: TransactionAction, config: Option, ) -> Result<(Option, Option, CallOrCreateInfo), DispatchError> { @@ -540,7 +566,6 @@ impl Pallet { value, gas_limit.low_u64(), gas_price, - nonce, config.as_ref().unwrap_or(T::config()), ) .map_err(Into::into)?; @@ -554,7 +579,6 @@ impl Pallet { value, gas_limit.low_u64(), gas_price, - nonce, config.as_ref().unwrap_or(T::config()), ) .map_err(Into::into)?; @@ -563,6 +587,29 @@ impl Pallet { } } } + + /// Validate an Ethereum transaction already in block + /// + /// This function must be called during the pre-dispatch phase + /// (just before applying the extrinsic). + pub fn validate_transaction_in_block( + origin: H160, + transaction: ðereum::TransactionV0, + ) -> Result<(), TransactionValidityError> { + let account_nonce = Self::validate_transaction_common(origin, transaction)?; + + // In the context of the block, a transaction with a nonce that is + // too high should be considered invalid and make the whole block invalid. + if transaction.nonce > account_nonce { + Err(TransactionValidityError::Invalid( + InvalidTransaction::Future, + )) + } else if transaction.nonce < account_nonce { + Err(TransactionValidityError::Invalid(InvalidTransaction::Stale)) + } else { + Ok(()) + } + } } #[derive(Eq, PartialEq, Clone, sp_runtime::RuntimeDebug)] diff --git a/frame/ethereum/src/mock.rs b/frame/ethereum/src/mock.rs index 3b7d1439d1..df2227b161 100644 --- a/frame/ethereum/src/mock.rs +++ b/frame/ethereum/src/mock.rs @@ -193,6 +193,18 @@ impl fp_self_contained::SelfContainedCall for Call { } } + fn pre_dispatch_self_contained( + &self, + info: &Self::SignedInfo, + ) -> Option> { + match self { + Call::Ethereum(crate::Call::transact(transaction)) => { + Some(Ethereum::validate_transaction_in_block(*info, transaction)) + } + _ => None, + } + } + fn apply_self_contained( self, info: Self::SignedInfo, diff --git a/frame/ethereum/src/tests.rs b/frame/ethereum/src/tests.rs index b793c7edb3..7b0dcf73f7 100644 --- a/frame/ethereum/src/tests.rs +++ b/frame/ethereum/src/tests.rs @@ -68,7 +68,6 @@ fn transaction_should_increment_nonce() { t.value, t.gas_limit, Some(t.gas_price), - Some(t.nonce), t.action, None, )); @@ -127,7 +126,6 @@ fn transaction_with_to_low_nonce_should_not_work() { t.value, t.gas_limit, Some(t.gas_price), - Some(t.nonce), t.action, None, )); @@ -212,7 +210,6 @@ fn contract_constructor_should_get_executed() { t.value, t.gas_limit, Some(t.gas_price), - Some(t.nonce), t.action, None, )); @@ -263,7 +260,6 @@ fn contract_should_be_created_at_given_address() { t.value, t.gas_limit, Some(t.gas_price), - Some(t.nonce), t.action, None, )); @@ -286,7 +282,6 @@ fn transaction_should_generate_correct_gas_used() { t.value, t.gas_limit, Some(t.gas_price), - Some(t.nonce), t.action, None, ) @@ -333,7 +328,6 @@ fn call_should_handle_errors() { t.value, t.gas_limit, Some(t.gas_price), - Some(t.nonce), t.action, None, )); @@ -350,7 +344,6 @@ fn call_should_handle_errors() { U256::zero(), U256::from(1048576), Some(U256::from(1)), - Some(U256::from(1)), TransactionAction::Call(H160::from_slice(&contract_address)), None, ) @@ -373,7 +366,6 @@ fn call_should_handle_errors() { U256::zero(), U256::from(1048576), Some(U256::from(1)), - Some(U256::from(2)), TransactionAction::Call(H160::from_slice(&contract_address)), None, ) diff --git a/frame/evm/src/lib.rs b/frame/evm/src/lib.rs index 6b3f9e4c0b..2ccff24c33 100644 --- a/frame/evm/src/lib.rs +++ b/frame/evm/src/lib.rs @@ -180,7 +180,7 @@ pub mod pallet { value: U256, gas_limit: u64, gas_price: U256, - nonce: Option, + _nonce: Option, ) -> DispatchResultWithPostInfo { T::CallOrigin::ensure_address_origin(&source, origin)?; @@ -191,7 +191,6 @@ pub mod pallet { value, gas_limit, Some(gas_price), - nonce, T::config(), )?; @@ -222,19 +221,12 @@ pub mod pallet { value: U256, gas_limit: u64, gas_price: U256, - nonce: Option, + _nonce: Option, ) -> DispatchResultWithPostInfo { T::CallOrigin::ensure_address_origin(&source, origin)?; - let info = T::Runner::create( - source, - init, - value, - gas_limit, - Some(gas_price), - nonce, - T::config(), - )?; + let info = + T::Runner::create(source, init, value, gas_limit, Some(gas_price), T::config())?; match info { CreateInfo { @@ -271,7 +263,7 @@ pub mod pallet { value: U256, gas_limit: u64, gas_price: U256, - nonce: Option, + _nonce: Option, ) -> DispatchResultWithPostInfo { T::CallOrigin::ensure_address_origin(&source, origin)?; @@ -282,7 +274,6 @@ pub mod pallet { value, gas_limit, Some(gas_price), - nonce, T::config(), )?; @@ -616,6 +607,16 @@ impl Pallet { >::insert(address, code); } + /// Get the account balance in EVM format. + pub fn account_balance(address: &H160) -> U256 { + let account_id = T::AddressMapping::into_account_id(*address); + + // keepalive `true` takes into account ExistentialDeposit as part of what's considered liquid balance. + let balance = T::Currency::reducible_balance(&account_id, true); + + U256::from(UniqueSaturatedInto::::unique_saturated_into(balance)) + } + /// Get the account basic in EVM format. pub fn account_basic(address: &H160) -> Account { let account_id = T::AddressMapping::into_account_id(*address); diff --git a/frame/evm/src/runner/mod.rs b/frame/evm/src/runner/mod.rs index 55329c87a3..cbaba4c943 100644 --- a/frame/evm/src/runner/mod.rs +++ b/frame/evm/src/runner/mod.rs @@ -32,7 +32,6 @@ pub trait Runner { value: U256, gas_limit: u64, gas_price: Option, - nonce: Option, config: &evm::Config, ) -> Result; @@ -42,7 +41,6 @@ pub trait Runner { value: U256, gas_limit: u64, gas_price: Option, - nonce: Option, config: &evm::Config, ) -> Result; @@ -53,7 +51,6 @@ pub trait Runner { value: U256, gas_limit: u64, gas_price: Option, - nonce: Option, config: &evm::Config, ) -> Result; } diff --git a/frame/evm/src/runner/stack.rs b/frame/evm/src/runner/stack.rs index e9dd72afa0..e835160d5f 100644 --- a/frame/evm/src/runner/stack.rs +++ b/frame/evm/src/runner/stack.rs @@ -19,7 +19,7 @@ use crate::{ runner::Runner as RunnerT, AccountCodes, AccountStorages, AddressMapping, BlockHashMapping, - Config, Error, Event, FeeCalculator, OnChargeEVMTransaction, Pallet, PrecompileSet, + Config, Error, Event, OnChargeEVMTransaction, Pallet, PrecompileSet, }; use evm::{ backend::Backend as BackendT, @@ -27,10 +27,7 @@ use evm::{ ExitError, ExitReason, Transfer, }; use fp_evm::{CallInfo, CreateInfo, ExecutionInfo, Log, Vicinity}; -use frame_support::{ - ensure, - traits::{Currency, ExistenceRequirement, Get}, -}; +use frame_support::traits::{Currency, ExistenceRequirement, Get}; use sha3::{Digest, Keccak256}; use sp_core::{H160, H256, U256}; use sp_runtime::traits::UniqueSaturatedInto; @@ -48,7 +45,6 @@ impl Runner { value: U256, gas_limit: u64, gas_price: Option, - nonce: Option, config: &'config evm::Config, f: F, ) -> Result, Error> @@ -57,18 +53,7 @@ impl Runner { &mut StackExecutor<'config, SubstrateStackState<'_, 'config, T>>, ) -> (ExitReason, R), { - // Gas price check is skipped when performing a gas estimation. - let gas_price = match gas_price { - Some(gas_price) => { - ensure!( - gas_price >= T::FeeCalculator::min_gas_price(), - Error::::GasPriceTooLow - ); - gas_price - } - None => Default::default(), - }; - + let gas_price = gas_price.unwrap_or_default(); let vicinity = Vicinity { gas_price, origin: source, @@ -81,19 +66,7 @@ impl Runner { let total_fee = gas_price .checked_mul(U256::from(gas_limit)) - .ok_or(Error::::FeeOverflow)?; - let total_payment = value - .checked_add(total_fee) - .ok_or(Error::::PaymentOverflow)?; - let source_account = Pallet::::account_basic(&source); - ensure!( - source_account.balance >= total_payment, - Error::::BalanceLow - ); - - if let Some(nonce) = nonce { - ensure!(source_account.nonce == nonce, Error::::InvalidNonce); - } + .expect("fee overflow"); // If overflow, we must ensure that the block is rejected // Deduct fee from the `source` account. let fee = T::OnChargeTransaction::withdraw_fee(&source, total_fee)?; @@ -163,18 +136,11 @@ impl RunnerT for Runner { value: U256, gas_limit: u64, gas_price: Option, - nonce: Option, config: &evm::Config, ) -> Result { - Self::execute( - source, - value, - gas_limit, - gas_price, - nonce, - config, - |executor| executor.transact_call(source, target, value, input, gas_limit), - ) + Self::execute(source, value, gas_limit, gas_price, config, |executor| { + executor.transact_call(source, target, value, input, gas_limit) + }) } fn create( @@ -183,24 +149,15 @@ impl RunnerT for Runner { value: U256, gas_limit: u64, gas_price: Option, - nonce: Option, config: &evm::Config, ) -> Result { - Self::execute( - source, - value, - gas_limit, - gas_price, - nonce, - config, - |executor| { - let address = executor.create_address(evm::CreateScheme::Legacy { caller: source }); - ( - executor.transact_create(source, value, init, gas_limit), - address, - ) - }, - ) + Self::execute(source, value, gas_limit, gas_price, config, |executor| { + let address = executor.create_address(evm::CreateScheme::Legacy { caller: source }); + ( + executor.transact_create(source, value, init, gas_limit), + address, + ) + }) } fn create2( @@ -210,29 +167,20 @@ impl RunnerT for Runner { value: U256, gas_limit: u64, gas_price: Option, - nonce: Option, config: &evm::Config, ) -> Result { let code_hash = H256::from_slice(Keccak256::digest(&init).as_slice()); - Self::execute( - source, - value, - gas_limit, - gas_price, - nonce, - config, - |executor| { - let address = executor.create_address(evm::CreateScheme::Create2 { - caller: source, - code_hash, - salt, - }); - ( - executor.transact_create2(source, value, init, salt, gas_limit), - address, - ) - }, - ) + Self::execute(source, value, gas_limit, gas_price, config, |executor| { + let address = executor.create_address(evm::CreateScheme::Create2 { + caller: source, + code_hash, + salt, + }); + ( + executor.transact_create2(source, value, init, salt, gas_limit), + address, + ) + }) } } diff --git a/primitives/self-contained/src/checked_extrinsic.rs b/primitives/self-contained/src/checked_extrinsic.rs index e0ecebff67..41a66c60aa 100644 --- a/primitives/self-contained/src/checked_extrinsic.rs +++ b/primitives/self-contained/src/checked_extrinsic.rs @@ -135,6 +135,12 @@ where Ok(res) } CheckedSignature::SelfContained(signed_info) => { + // If pre-dispatch fail, the block must be considered invalid + self.function + .pre_dispatch_self_contained(&signed_info) + .ok_or(TransactionValidityError::Invalid( + InvalidTransaction::BadProof, + ))??; Ok(self.function.apply_self_contained(signed_info).ok_or( TransactionValidityError::Invalid(InvalidTransaction::BadProof), )?) diff --git a/primitives/self-contained/src/lib.rs b/primitives/self-contained/src/lib.rs index 82aebb7a9c..e784a4c256 100644 --- a/primitives/self-contained/src/lib.rs +++ b/primitives/self-contained/src/lib.rs @@ -44,6 +44,23 @@ pub trait SelfContainedCall: Dispatchable { /// Validate a self-contained function. Returns `None` if the /// function is not a self-contained. fn validate_self_contained(&self, info: &Self::SignedInfo) -> Option; + /// Do any pre-flight stuff for a self-contained call. + /// + /// Note this function by default delegates to `validate_self_contained`, so that + /// all checks performed for the transaction queue are also performed during + /// the dispatch phase (applying the extrinsic). + /// + /// If you ever override this function, you need to make sure to always + /// perform the same validation as in `validate_self_contained`. + /// + /// Returns `None` if the function is not a self-contained. + fn pre_dispatch_self_contained( + &self, + info: &Self::SignedInfo, + ) -> Option> { + self.validate_self_contained(info) + .map(|res| res.map(|_| ())) + } /// Apply a self-contained function. Returns `None` if the /// function is not a self-contained. fn apply_self_contained( diff --git a/template/runtime/src/lib.rs b/template/runtime/src/lib.rs index 0b9c7baaaa..b7924fbd2b 100644 --- a/template/runtime/src/lib.rs +++ b/template/runtime/src/lib.rs @@ -440,6 +440,18 @@ impl fp_self_contained::SelfContainedCall for Call { } } + fn pre_dispatch_self_contained( + &self, + info: &Self::SignedInfo, + ) -> Option> { + match self { + Call::Ethereum(pallet_ethereum::Call::transact(transaction)) => { + Some(Ethereum::validate_transaction_in_block(*info, transaction)) + } + _ => None, + } + } + fn apply_self_contained( self, info: Self::SignedInfo, @@ -561,7 +573,7 @@ impl_runtime_apis! { value: U256, gas_limit: U256, gas_price: Option, - nonce: Option, + _nonce: Option, estimate: bool, ) -> Result { let config = if estimate { @@ -579,7 +591,6 @@ impl_runtime_apis! { value, gas_limit.low_u64(), gas_price, - nonce, config.as_ref().unwrap_or(::config()), ).map_err(|err| err.into()) } @@ -590,7 +601,7 @@ impl_runtime_apis! { value: U256, gas_limit: U256, gas_price: Option, - nonce: Option, + _nonce: Option, estimate: bool, ) -> Result { let config = if estimate { @@ -607,7 +618,6 @@ impl_runtime_apis! { value, gas_limit.low_u64(), gas_price, - nonce, config.as_ref().unwrap_or(::config()), ).map_err(|err| err.into()) } From c9b1ce7165c920f85344be4ef0655989c53a2731 Mon Sep 17 00:00:00 2001 From: librelois Date: Wed, 13 Oct 2021 13:23:42 +0200 Subject: [PATCH 3/5] Revert frame-evm changes --- frame/ethereum/src/lib.rs | 4 ++ frame/ethereum/src/tests.rs | 8 +++ frame/evm/src/lib.rs | 29 +++++----- frame/evm/src/runner/mod.rs | 3 + frame/evm/src/runner/stack.rs | 102 +++++++++++++++++++++++++--------- template/runtime/src/lib.rs | 6 +- 6 files changed, 110 insertions(+), 42 deletions(-) diff --git a/frame/ethereum/src/lib.rs b/frame/ethereum/src/lib.rs index 9e4c784bce..6b861790ff 100644 --- a/frame/ethereum/src/lib.rs +++ b/frame/ethereum/src/lib.rs @@ -457,6 +457,7 @@ impl Pallet { transaction.value, transaction.gas_limit, Some(transaction.gas_price), + Some(transaction.nonce), transaction.action, None, )?; @@ -556,6 +557,7 @@ impl Pallet { value: U256, gas_limit: U256, gas_price: Option, + nonce: Option, action: TransactionAction, config: Option, ) -> Result<(Option, Option, CallOrCreateInfo), DispatchError> { @@ -568,6 +570,7 @@ impl Pallet { value, gas_limit.low_u64(), gas_price, + nonce, config.as_ref().unwrap_or(T::config()), ) .map_err(Into::into)?; @@ -581,6 +584,7 @@ impl Pallet { value, gas_limit.low_u64(), gas_price, + nonce, config.as_ref().unwrap_or(T::config()), ) .map_err(Into::into)?; diff --git a/frame/ethereum/src/tests.rs b/frame/ethereum/src/tests.rs index 7b0dcf73f7..b793c7edb3 100644 --- a/frame/ethereum/src/tests.rs +++ b/frame/ethereum/src/tests.rs @@ -68,6 +68,7 @@ fn transaction_should_increment_nonce() { t.value, t.gas_limit, Some(t.gas_price), + Some(t.nonce), t.action, None, )); @@ -126,6 +127,7 @@ fn transaction_with_to_low_nonce_should_not_work() { t.value, t.gas_limit, Some(t.gas_price), + Some(t.nonce), t.action, None, )); @@ -210,6 +212,7 @@ fn contract_constructor_should_get_executed() { t.value, t.gas_limit, Some(t.gas_price), + Some(t.nonce), t.action, None, )); @@ -260,6 +263,7 @@ fn contract_should_be_created_at_given_address() { t.value, t.gas_limit, Some(t.gas_price), + Some(t.nonce), t.action, None, )); @@ -282,6 +286,7 @@ fn transaction_should_generate_correct_gas_used() { t.value, t.gas_limit, Some(t.gas_price), + Some(t.nonce), t.action, None, ) @@ -328,6 +333,7 @@ fn call_should_handle_errors() { t.value, t.gas_limit, Some(t.gas_price), + Some(t.nonce), t.action, None, )); @@ -344,6 +350,7 @@ fn call_should_handle_errors() { U256::zero(), U256::from(1048576), Some(U256::from(1)), + Some(U256::from(1)), TransactionAction::Call(H160::from_slice(&contract_address)), None, ) @@ -366,6 +373,7 @@ fn call_should_handle_errors() { U256::zero(), U256::from(1048576), Some(U256::from(1)), + Some(U256::from(2)), TransactionAction::Call(H160::from_slice(&contract_address)), None, ) diff --git a/frame/evm/src/lib.rs b/frame/evm/src/lib.rs index 2ccff24c33..6b3f9e4c0b 100644 --- a/frame/evm/src/lib.rs +++ b/frame/evm/src/lib.rs @@ -180,7 +180,7 @@ pub mod pallet { value: U256, gas_limit: u64, gas_price: U256, - _nonce: Option, + nonce: Option, ) -> DispatchResultWithPostInfo { T::CallOrigin::ensure_address_origin(&source, origin)?; @@ -191,6 +191,7 @@ pub mod pallet { value, gas_limit, Some(gas_price), + nonce, T::config(), )?; @@ -221,12 +222,19 @@ pub mod pallet { value: U256, gas_limit: u64, gas_price: U256, - _nonce: Option, + nonce: Option, ) -> DispatchResultWithPostInfo { T::CallOrigin::ensure_address_origin(&source, origin)?; - let info = - T::Runner::create(source, init, value, gas_limit, Some(gas_price), T::config())?; + let info = T::Runner::create( + source, + init, + value, + gas_limit, + Some(gas_price), + nonce, + T::config(), + )?; match info { CreateInfo { @@ -263,7 +271,7 @@ pub mod pallet { value: U256, gas_limit: u64, gas_price: U256, - _nonce: Option, + nonce: Option, ) -> DispatchResultWithPostInfo { T::CallOrigin::ensure_address_origin(&source, origin)?; @@ -274,6 +282,7 @@ pub mod pallet { value, gas_limit, Some(gas_price), + nonce, T::config(), )?; @@ -607,16 +616,6 @@ impl Pallet { >::insert(address, code); } - /// Get the account balance in EVM format. - pub fn account_balance(address: &H160) -> U256 { - let account_id = T::AddressMapping::into_account_id(*address); - - // keepalive `true` takes into account ExistentialDeposit as part of what's considered liquid balance. - let balance = T::Currency::reducible_balance(&account_id, true); - - U256::from(UniqueSaturatedInto::::unique_saturated_into(balance)) - } - /// Get the account basic in EVM format. pub fn account_basic(address: &H160) -> Account { let account_id = T::AddressMapping::into_account_id(*address); diff --git a/frame/evm/src/runner/mod.rs b/frame/evm/src/runner/mod.rs index cbaba4c943..55329c87a3 100644 --- a/frame/evm/src/runner/mod.rs +++ b/frame/evm/src/runner/mod.rs @@ -32,6 +32,7 @@ pub trait Runner { value: U256, gas_limit: u64, gas_price: Option, + nonce: Option, config: &evm::Config, ) -> Result; @@ -41,6 +42,7 @@ pub trait Runner { value: U256, gas_limit: u64, gas_price: Option, + nonce: Option, config: &evm::Config, ) -> Result; @@ -51,6 +53,7 @@ pub trait Runner { value: U256, gas_limit: u64, gas_price: Option, + nonce: Option, config: &evm::Config, ) -> Result; } diff --git a/frame/evm/src/runner/stack.rs b/frame/evm/src/runner/stack.rs index e835160d5f..e9dd72afa0 100644 --- a/frame/evm/src/runner/stack.rs +++ b/frame/evm/src/runner/stack.rs @@ -19,7 +19,7 @@ use crate::{ runner::Runner as RunnerT, AccountCodes, AccountStorages, AddressMapping, BlockHashMapping, - Config, Error, Event, OnChargeEVMTransaction, Pallet, PrecompileSet, + Config, Error, Event, FeeCalculator, OnChargeEVMTransaction, Pallet, PrecompileSet, }; use evm::{ backend::Backend as BackendT, @@ -27,7 +27,10 @@ use evm::{ ExitError, ExitReason, Transfer, }; use fp_evm::{CallInfo, CreateInfo, ExecutionInfo, Log, Vicinity}; -use frame_support::traits::{Currency, ExistenceRequirement, Get}; +use frame_support::{ + ensure, + traits::{Currency, ExistenceRequirement, Get}, +}; use sha3::{Digest, Keccak256}; use sp_core::{H160, H256, U256}; use sp_runtime::traits::UniqueSaturatedInto; @@ -45,6 +48,7 @@ impl Runner { value: U256, gas_limit: u64, gas_price: Option, + nonce: Option, config: &'config evm::Config, f: F, ) -> Result, Error> @@ -53,7 +57,18 @@ impl Runner { &mut StackExecutor<'config, SubstrateStackState<'_, 'config, T>>, ) -> (ExitReason, R), { - let gas_price = gas_price.unwrap_or_default(); + // Gas price check is skipped when performing a gas estimation. + let gas_price = match gas_price { + Some(gas_price) => { + ensure!( + gas_price >= T::FeeCalculator::min_gas_price(), + Error::::GasPriceTooLow + ); + gas_price + } + None => Default::default(), + }; + let vicinity = Vicinity { gas_price, origin: source, @@ -66,7 +81,19 @@ impl Runner { let total_fee = gas_price .checked_mul(U256::from(gas_limit)) - .expect("fee overflow"); // If overflow, we must ensure that the block is rejected + .ok_or(Error::::FeeOverflow)?; + let total_payment = value + .checked_add(total_fee) + .ok_or(Error::::PaymentOverflow)?; + let source_account = Pallet::::account_basic(&source); + ensure!( + source_account.balance >= total_payment, + Error::::BalanceLow + ); + + if let Some(nonce) = nonce { + ensure!(source_account.nonce == nonce, Error::::InvalidNonce); + } // Deduct fee from the `source` account. let fee = T::OnChargeTransaction::withdraw_fee(&source, total_fee)?; @@ -136,11 +163,18 @@ impl RunnerT for Runner { value: U256, gas_limit: u64, gas_price: Option, + nonce: Option, config: &evm::Config, ) -> Result { - Self::execute(source, value, gas_limit, gas_price, config, |executor| { - executor.transact_call(source, target, value, input, gas_limit) - }) + Self::execute( + source, + value, + gas_limit, + gas_price, + nonce, + config, + |executor| executor.transact_call(source, target, value, input, gas_limit), + ) } fn create( @@ -149,15 +183,24 @@ impl RunnerT for Runner { value: U256, gas_limit: u64, gas_price: Option, + nonce: Option, config: &evm::Config, ) -> Result { - Self::execute(source, value, gas_limit, gas_price, config, |executor| { - let address = executor.create_address(evm::CreateScheme::Legacy { caller: source }); - ( - executor.transact_create(source, value, init, gas_limit), - address, - ) - }) + Self::execute( + source, + value, + gas_limit, + gas_price, + nonce, + config, + |executor| { + let address = executor.create_address(evm::CreateScheme::Legacy { caller: source }); + ( + executor.transact_create(source, value, init, gas_limit), + address, + ) + }, + ) } fn create2( @@ -167,20 +210,29 @@ impl RunnerT for Runner { value: U256, gas_limit: u64, gas_price: Option, + nonce: Option, config: &evm::Config, ) -> Result { let code_hash = H256::from_slice(Keccak256::digest(&init).as_slice()); - Self::execute(source, value, gas_limit, gas_price, config, |executor| { - let address = executor.create_address(evm::CreateScheme::Create2 { - caller: source, - code_hash, - salt, - }); - ( - executor.transact_create2(source, value, init, salt, gas_limit), - address, - ) - }) + Self::execute( + source, + value, + gas_limit, + gas_price, + nonce, + config, + |executor| { + let address = executor.create_address(evm::CreateScheme::Create2 { + caller: source, + code_hash, + salt, + }); + ( + executor.transact_create2(source, value, init, salt, gas_limit), + address, + ) + }, + ) } } diff --git a/template/runtime/src/lib.rs b/template/runtime/src/lib.rs index 68c46ac13d..6ff18ad95f 100644 --- a/template/runtime/src/lib.rs +++ b/template/runtime/src/lib.rs @@ -573,7 +573,7 @@ impl_runtime_apis! { value: U256, gas_limit: U256, gas_price: Option, - _nonce: Option, + nonce: Option, estimate: bool, ) -> Result { let config = if estimate { @@ -591,6 +591,7 @@ impl_runtime_apis! { value, gas_limit.low_u64(), gas_price, + nonce, config.as_ref().unwrap_or(::config()), ).map_err(|err| err.into()) } @@ -601,7 +602,7 @@ impl_runtime_apis! { value: U256, gas_limit: U256, gas_price: Option, - _nonce: Option, + nonce: Option, estimate: bool, ) -> Result { let config = if estimate { @@ -618,6 +619,7 @@ impl_runtime_apis! { value, gas_limit.low_u64(), gas_price, + nonce, config.as_ref().unwrap_or(::config()), ).map_err(|err| err.into()) } From 17b7f178f32045f8f4fdbc1060c49562d5f03a22 Mon Sep 17 00:00:00 2001 From: librelois Date: Wed, 13 Oct 2021 13:58:32 +0200 Subject: [PATCH 4/5] keeping *_self_contained aligned --- frame/ethereum/src/lib.rs | 14 ++++++++++++++ frame/ethereum/src/mock.rs | 4 +--- template/runtime/src/lib.rs | 4 +--- 3 files changed, 16 insertions(+), 6 deletions(-) diff --git a/frame/ethereum/src/lib.rs b/frame/ethereum/src/lib.rs index 6b861790ff..82f4727867 100644 --- a/frame/ethereum/src/lib.rs +++ b/frame/ethereum/src/lib.rs @@ -118,6 +118,20 @@ where } } + pub fn pre_dispatch_self_contained( + &self, + origin: &H160, + ) -> Option> { + if let Call::transact(transaction) = self { + Some(Pallet::::validate_transaction_in_block( + *origin, + &transaction, + )) + } else { + None + } + } + pub fn validate_self_contained(&self, origin: &H160) -> Option { if let Call::transact(transaction) = self { Some(Pallet::::validate_transaction_in_pool( diff --git a/frame/ethereum/src/mock.rs b/frame/ethereum/src/mock.rs index df2227b161..5a8a385722 100644 --- a/frame/ethereum/src/mock.rs +++ b/frame/ethereum/src/mock.rs @@ -198,9 +198,7 @@ impl fp_self_contained::SelfContainedCall for Call { info: &Self::SignedInfo, ) -> Option> { match self { - Call::Ethereum(crate::Call::transact(transaction)) => { - Some(Ethereum::validate_transaction_in_block(*info, transaction)) - } + Call::Ethereum(call) => call.pre_dispatch_self_contained(info), _ => None, } } diff --git a/template/runtime/src/lib.rs b/template/runtime/src/lib.rs index 6ff18ad95f..209a35c20a 100644 --- a/template/runtime/src/lib.rs +++ b/template/runtime/src/lib.rs @@ -445,9 +445,7 @@ impl fp_self_contained::SelfContainedCall for Call { info: &Self::SignedInfo, ) -> Option> { match self { - Call::Ethereum(pallet_ethereum::Call::transact(transaction)) => { - Some(Ethereum::validate_transaction_in_block(*info, transaction)) - } + Call::Ethereum(call) => call.pre_dispatch_self_contained(info), _ => None, } } From e7a92ae39c41e7cafbd30e665992dfc3baca8b07 Mon Sep 17 00:00:00 2001 From: librelois Date: Wed, 13 Oct 2021 13:59:50 +0200 Subject: [PATCH 5/5] remove unused default impl --- primitives/self-contained/src/lib.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/primitives/self-contained/src/lib.rs b/primitives/self-contained/src/lib.rs index e784a4c256..62cca45818 100644 --- a/primitives/self-contained/src/lib.rs +++ b/primitives/self-contained/src/lib.rs @@ -57,10 +57,7 @@ pub trait SelfContainedCall: Dispatchable { fn pre_dispatch_self_contained( &self, info: &Self::SignedInfo, - ) -> Option> { - self.validate_self_contained(info) - .map(|res| res.map(|_| ())) - } + ) -> Option>; /// Apply a self-contained function. Returns `None` if the /// function is not a self-contained. fn apply_self_contained(