Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix security issue: transaction validity controls must be executed in STF #495

Merged
merged 6 commits into from
Oct 13, 2021
Merged

Fix security issue: transaction validity controls must be executed in STF #495

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3cb1812
add security tests
librelois Oct 12, 2021
908d6ce
add pre_dispatch phase to fiy security issues
librelois Oct 12, 2021
9ee0bd0
Start close to used gas and stop search when estimation varies slightly
librelois Oct 13, 2021
c9b1ce7
Revert frame-evm changes
librelois Oct 13, 2021
17b7f17
keeping *_self_contained aligned
librelois Oct 13, 2021
e7a92ae
remove unused default impl
librelois Oct 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
211 changes: 138 additions & 73 deletions frame/ethereum/src/lib.rs
View file
Open in desktop
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
Open in desktop
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