Fix security issue: transaction validity controls must be executed in STF (polkadot-evm#495)

* add security tests

* add pre_dispatch phase to fiy security issues

* Revert frame-evm changes

* keeping *_self_contained aligned

* remove unused default impl

Author: librelois

frame/ethereum/src/lib.rs

@@ -118,78 +118,26 @@ where
 		}
 	}
 
-	pub fn validate_self_contained(&self, origin: &H160) -> Option<TransactionValidity> {
+	pub fn pre_dispatch_self_contained(
+		&self,
+		origin: &H160,
+	) -> Option<Result<(), TransactionValidityError>> {
 		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(),
-					<T as pallet_evm::Config>::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::<T>::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(Pallet::<T>::validate_transaction_in_block(
+				*origin,
+				&transaction,
+			))
+		} else {
+			None
+		}
+	}
 
-			Some(validate())
+	pub fn validate_self_contained(&self, origin: &H160) -> Option<TransactionValidity> {
+		if let Call::transact(transaction) = self {
+			Some(Pallet::<T>::validate_transaction_in_pool(
+				*origin,
+				transaction,
+			))
 		} else {
 			None
 		}
@@ -259,9 +207,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",
+					);
 				}
 			}
 
@@ -287,7 +238,7 @@ pub mod pallet {
 				Error::<T>::PreLogExists,
 			);
 
-			Self::do_transact(source, transaction)
+			Self::apply_validated_transaction(source, transaction)
 		}
 	}
 
@@ -418,7 +369,98 @@ impl<T: Config> Pallet<T> {
 		}
 	}
 
-	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<U256, TransactionValidityError> {
+		// 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(),
+			<T as pallet_evm::Config>::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::<T>::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 {
 		let transaction_hash =
 			H256::from_slice(Keccak256::digest(&rlp::encode(&transaction)).as_slice());
 		let transaction_index = Pending::<T>::get().len() as u32;
@@ -565,6 +607,29 @@ impl<T: Config> Pallet<T> {
 			}
 		}
 	}
+
+	/// 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: &ethereum::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)]

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<Test>;
+pub type SignedExtra = (frame_system::CheckSpecVersion<Test>,);
+
+type UncheckedExtrinsic = frame_system::mocking::MockUncheckedExtrinsic<Test, (), SignedExtra>;
 type Block = frame_system::mocking::MockBlock<Test>;
 
 frame_support::construct_runtime! {
@@ -166,6 +169,54 @@ 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<Result<Self::SignedInfo, TransactionValidityError>> {
+		match self {
+			Call::Ethereum(call) => call.check_self_contained(),
+			_ => None,
+		}
+	}
+
+	fn validate_self_contained(&self, info: &Self::SignedInfo) -> Option<TransactionValidity> {
+		match self {
+			Call::Ethereum(call) => call.validate_self_contained(info),
+			_ => None,
+		}
+	}
+
+	fn pre_dispatch_self_contained(
+		&self,
+		info: &Self::SignedInfo,
+	) -> Option<Result<(), TransactionValidityError>> {
+		match self {
+			Call::Ethereum(call) => call.pre_dispatch_self_contained(info),
+			_ => None,
+		}
+	}
+
+	fn apply_self_contained(
+		self,
+		info: Self::SignedInfo,
+	) -> Option<sp_runtime::DispatchResultWithInfo<sp_runtime::traits::PostDispatchInfoOf<Self>>> {
+		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 +306,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 +319,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]),
 		)