Skip to content

Commit

Permalink
Fix security issue: transaction validity controls must be executed in…
Browse files Browse the repository at this point in the history 
… 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
  • Loading branch information
@librelois
librelois authored Oct 13, 2021
1 parent 0a8e696 commit 146bb48
Show file tree
Hide file tree
Showing 6 changed files with 287 additions and 81 deletions.
211 changes: 138 additions & 73 deletions frame/ethereum/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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",
);
}
}

Expand All @@ -287,7 +238,7 @@ pub mod pallet {
Error::<T>::PreLogExists,
);

Self::do_transact(source, transaction)
Self::apply_validated_transaction(source, transaction)
}
}

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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)]
Expand Down
61 changes: 58 additions & 3 deletions frame/ethereum/src/mock.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand All @@ -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! {
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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(
Expand All @@ -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]),
)
Expand Down
Loading

0 comments on commit 146bb48

Please sign in to comment.