CVE-2021-41138 (Rust, published 2021-10-13T16:15Z)
Description: value: Frontier is Substrate's Ethereum compatibility layer. In the newly introduced signed Frontier-specific extrinsic for `pallet-ethereum`, a large part of transaction validation logic was only called in transaction pool validation, but not in block execution. Malicious validators can take advantage of this to put invalid transactions into a block. The attack is limited in that the signature is always validated, and the majority of the validation is done again in the subsequent `pallet-evm` execution logic. However, do note that a chain ID replay attack was possible. In addition, spamming attacks are of main concerns, while they are limited by Substrate block size limits and other factors. The issue is patched in commit `146bb48849e5393004be5c88beefe76fdf009aba`.
Severity: MEDIUM (CVSS v3 base score: 5.3)
Weakness: CWE-20 \u2014 Improper Input Validation
The product receives input or data, but it does
        not validate or incorrectly validates that the input has the
        properties that are required to process the data safely and
        correctly.
Repository: https://github.com/paritytech/frontier
Files changed:
frame/ethereum/src/lib.rs
frame/ethereum/src/mock.rs
frame/ethereum/src/tests.rs
primitives/self-contained/src/checked_extrinsic.rs
primitives/self-contained/src/lib.rs
template/runtime/src/lib.rs
Commit message: Fix security issue: transaction validity controls must be executed in STF (#495)

* add security tests

* add pre_dispatch phase to fiy security issues

* Revert frame-evm changes

* keeping *_self_contained aligned

* remove unused default impl

Vulnerable code:
	pub fn validate_self_contained(&self, origin: &H160) -> Option<TransactionValidity> {
			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(validate())
					Self::do_transact(source, transaction).expect(
			Self::do_transact(source, transaction)
	fn do_transact(source: H160, transaction: Transaction) -> DispatchResultWithPostInfo {
	traits::{BlakeTwo256, IdentityLookup},
type UncheckedExtrinsic = frame_system::mocking::MockUncheckedExtrinsic<Test>;
			s.1.serialize() as u64 % 2 + ChainId::get() * 2 + 35,
	mock::*, CallOrCreateInfo, Error, RawOrigin, Transaction, TransactionAction,
	ValidTransactionBuilder, H160, H256, U256,
use frame_support::{assert_err, assert_noop, assert_ok, unsigned::ValidateUnsigned};
use sp_runtime::transaction_validity::{InvalidTransaction, TransactionSource};
fn transaction_with_invalid_nonce_should_not_work() {

Fixed code:
	pub fn pre_dispatch_self_contained(
		&self,
		origin: &H160,
	) -> Option<Result<(), TransactionValidityError>> {
			Some(Pallet::<T>::validate_transaction_in_block(
				*origin,
				&transaction,
			))
		} else {
			None
		}
	}
	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,
			))
					Self::validate_transaction_in_block(source, &transaction).expect(
					Self::apply_validated_transaction(source, transaction).expect(
						"pre-block transaction execution failed; the block cannot be built",
					);
			Self::apply_validated_transaction(source, transaction)
	// 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 {

	/// 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(())
		}
	}
use codec::{WrapperTypeDecode, WrapperTypeEncode};
	traits::{BlakeTwo256, IdentityLookup, SignedExtension},
pub type SignedExtra = (frame_system::CheckSpecVersion<Test>,);

type UncheckedExtrinsic = frame_system::mocking::MockUncheckedExtrinsic<Test, (), SignedExtra>;
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,
		}
	}
}

		self.sign_with_chain_id(key, ChainId::get())
	}

	pub fn sign_with_chain_id(&self, key: &H256, chain_id: u64) -> Transaction {
			s.1.serialize() as u64 % 2 + chain_id * 2 + 35,
	mock::*, CallOrCreateInfo, Error, RawOrigin, Transaction, TransactionAction, H160, H256, U256,
use frame_support::{
	assert_err, assert_noop, assert_ok,
	unsigned::{TransactionValidityError, ValidateUnsigned},
};
use sp_runtime::traits::Applyable;
use sp_runtime::transaction_validity::{
	InvalidTransaction, TransactionSource, ValidTransactionBuilder,
};
fn transaction_with_to_low_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::<Test>::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::<Test>(&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::<Test>::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::<Test>(&dispatch_info, 0),
			TransactionValidityError::Invalid(InvalidTransaction::Custom(
				crate::TransactionValidationError::InvalidChainId as u8,
			))
		);
	});
}

				// If pre-dispatch fail, the block must be considered invalid
				self.function
					.pre_dispatch_self_contained(&signed_info)
					.ok_or(TransactionValidityError::Invalid(
						InvalidTransaction::BadProof,
					))??;
	/// 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<Result<(), TransactionValidityError>>;
	fn pre_dispatch_self_contained(
		&self,
		info: &Self::SignedInfo,
	) -> Option<Result<(), TransactionValidityError>> {
		match self {
			Call::Ethereum(call) => call.pre_dispatch_self_contained(info),
			_ => None,
		}
	}