From 0b5142b4df9a3f783b0f77dfb6bb4a2de2019cb5 Mon Sep 17 00:00:00 2001 From: Xiliang Chen Date: Tue, 15 Sep 2020 10:19:02 +1200 Subject: [PATCH] Signed extension (#128) * validate transaction before allow them enter the pool * avoid panic * fix tests --- frame/ethereum/src/lib.rs | 99 ++++++++++++++++++++++++++----------- frame/ethereum/src/tests.rs | 53 ++++++++++++++------ 2 files changed, 110 insertions(+), 42 deletions(-) diff --git a/frame/ethereum/src/lib.rs b/frame/ethereum/src/lib.rs index c8bb8201b..9d08d8f00 100644 --- a/frame/ethereum/src/lib.rs +++ b/frame/ethereum/src/lib.rs @@ -23,19 +23,19 @@ #![cfg_attr(not(feature = "std"), no_std)] use frame_support::{ - decl_module, decl_storage, decl_error, decl_event, ensure, + decl_module, decl_storage, decl_error, decl_event, traits::Get, traits::FindAuthor }; use sp_std::prelude::*; -use sp_runtime::generic::DigestItem; use frame_system::ensure_none; use ethereum_types::{H160, H64, H256, U256, Bloom}; use sp_runtime::{ traits::UniqueSaturatedInto, - transaction_validity::{TransactionValidity, TransactionSource, ValidTransaction} + transaction_validity::{TransactionValidity, TransactionSource, ValidTransaction, InvalidTransaction}, + generic::DigestItem, + DispatchResult, }; use evm::{ExitError, ExitRevert, ExitFatal, ExitReason}; -use rlp; use sha3::{Digest, Keccak256}; use codec::Encode; use frontier_consensus_primitives::{FRONTIER_ENGINE_ID, ConsensusLog}; @@ -88,6 +88,8 @@ decl_event!( decl_error! { /// Ethereum pallet errors. pub enum Error for Module { + /// Signature is invalid. + InvalidSignature, /// Transaction signed with wrong chain id InvalidChainId, /// Trying to pop from an empty stack. @@ -145,22 +147,9 @@ decl_module! { fn transact(origin, transaction: ethereum::Transaction) { ensure_none(origin)?; - ensure!( - transaction.signature.chain_id().unwrap_or_default() == T::ChainId::get(), - Error::::InvalidChainId - ); - let mut sig = [0u8; 65]; - let mut msg = [0u8; 32]; - sig[0..32].copy_from_slice(&transaction.signature.r()[..]); - sig[32..64].copy_from_slice(&transaction.signature.s()[..]); - sig[64] = transaction.signature.standard_v(); - msg.copy_from_slice(&transaction.message_hash(Some(T::ChainId::get()))[..]); - - let pubkey = sp_io::crypto::secp256k1_ecdsa_recover(&sig, &msg) - .map_err(|_| "Recover public key failed")?; - let source = H160::from(H256::from_slice(Keccak256::digest(&pubkey).as_slice())); - - Self::execute(source, transaction); + let source = Self::recover_signer(&transaction).ok_or_else(|| Error::::InvalidSignature)?; + + Self::execute(source, transaction)?; } fn on_finalize(n: T::BlockNumber) { @@ -169,17 +158,69 @@ decl_module! { } } +#[repr(u8)] +enum TransactionValidationError { + #[allow (dead_code)] + UnknownError, + InvalidChainId, + InvalidSignature, +} + impl frame_support::unsigned::ValidateUnsigned for Module { type Call = Call; fn validate_unsigned(_source: TransactionSource, call: &Self::Call) -> TransactionValidity { - ValidTransaction::with_tag_prefix("Ethereum") - .and_provides(call) - .build() + if let Call::transact(transaction) = call { + if transaction.signature.chain_id().unwrap_or_default() != T::ChainId::get() { + return InvalidTransaction::Custom(TransactionValidationError::InvalidChainId as u8).into(); + } + + let origin = Self::recover_signer(&transaction) + .ok_or_else(|| InvalidTransaction::Custom(TransactionValidationError::InvalidSignature as u8))?; + + let account_data = pallet_evm::Module::::account_basic(&origin); + + if transaction.nonce < account_data.nonce { + return InvalidTransaction::Stale.into(); + } + if transaction.nonce > account_data.nonce { + return InvalidTransaction::Future.into(); + } + + let fee = transaction.gas_price.saturating_mul(transaction.gas_limit); + + if account_data.balance < fee { + return InvalidTransaction::Payment.into(); + } + + let mut builder = ValidTransaction::with_tag_prefix("Ethereum") + .and_provides((&account_data, account_data.nonce)); + + if let Some(prev_nonce) = account_data.nonce.checked_sub(1.into()) { + builder = builder.and_requires((account_data, prev_nonce)) + } + + builder.build() + } else { + Err(InvalidTransaction::Call.into()) + } } } impl Module { + + fn recover_signer(transaction: ðereum::Transaction) -> Option { + let mut sig = [0u8; 65]; + let mut msg = [0u8; 32]; + sig[0..32].copy_from_slice(&transaction.signature.r()[..]); + sig[32..64].copy_from_slice(&transaction.signature.s()[..]); + sig[64] = transaction.signature.standard_v(); + msg.copy_from_slice(&transaction.message_hash(Some(T::ChainId::get()))[..]); + + let pubkey = sp_io::crypto::secp256k1_ecdsa_recover(&sig, &msg).ok()?; + Some(H160::from(H256::from_slice(Keccak256::digest(&pubkey).as_slice()))) + } + fn store_block() { let pending = Pending::take(); @@ -277,7 +318,7 @@ impl Module { } /// Execute an Ethereum transaction, ignoring transaction signatures. - pub fn execute(source: H160, transaction: ethereum::Transaction) { + pub fn execute(source: H160, transaction: ethereum::Transaction) -> DispatchResult { let transaction_hash = H256::from_slice( Keccak256::digest(&rlp::encode(&transaction)).as_slice() ); @@ -295,8 +336,8 @@ impl Module { transaction.gas_price, Some(transaction.nonce), true, - ).unwrap() - ).unwrap(); + )? + )?; TransactionStatus { transaction_hash, @@ -318,8 +359,8 @@ impl Module { transaction.gas_price, Some(transaction.nonce), true, - ).unwrap() - ).unwrap().1; + )? + )?.1; TransactionStatus { transaction_hash, @@ -341,6 +382,8 @@ impl Module { }; Pending::append((transaction, status, receipt)); + + Ok(()) } fn handle_exec(res: (ExitReason, R, U256)) -> Result<(ExitReason, R, U256), Error> { diff --git a/frame/ethereum/src/tests.rs b/frame/ethereum/src/tests.rs index 2fab25e2b..0670cf30e 100644 --- a/frame/ethereum/src/tests.rs +++ b/frame/ethereum/src/tests.rs @@ -22,7 +22,12 @@ use mock::*; use rustc_hex::FromHex; use std::str::FromStr; use ethereum::TransactionSignature; -use frame_support::assert_noop; +use frame_support::{ + assert_noop, assert_err, assert_ok, + unsigned::ValidateUnsigned, +}; +use sp_runtime::transaction_validity::{TransactionSource, InvalidTransaction}; + // This ERC-20 contract mints the maximum amount of tokens to the contract creator. // pragma solidity ^0.5.0; @@ -49,10 +54,10 @@ fn transaction_should_increment_nonce() { let alice = &pairs[0]; ext.execute_with(|| { - Ethereum::execute( + assert_ok!(Ethereum::execute( alice.address, default_erc20_creation_transaction(alice), - ); + )); assert_eq!(Evm::account_basic(&alice.address).nonce, U256::from(1)); }); } @@ -65,10 +70,10 @@ fn transaction_should_be_added_to_pending() { ext.execute_with(|| { let transaction = default_erc20_creation_transaction(alice); - Ethereum::execute( + assert_ok!(Ethereum::execute( alice.address, transaction.clone(), - ); + )); assert_eq!(Pending::get().len(), 1); assert_eq!(Pending::get()[0].0.input, transaction.input); }); @@ -76,7 +81,6 @@ fn transaction_should_be_added_to_pending() { #[test] -#[should_panic(expected = "called `Result::unwrap()` on an `Err` value: BalanceLow")] fn transaction_without_enough_gas_should_not_work() { let (pairs, mut ext) = new_test_ext(1); let alice = &pairs[0]; @@ -85,10 +89,31 @@ fn transaction_without_enough_gas_should_not_work() { let mut transaction = default_erc20_creation_transaction(alice); transaction.gas_price = U256::from(11_000_000); - Ethereum::execute( + assert_err!(Ethereum::validate_unsigned(TransactionSource::External, &Call::transact(transaction)), InvalidTransaction::Payment); + }); +} + +#[test] +fn transaction_with_invalid_nonce_should_not_work() { + let (pairs, mut ext) = new_test_ext(1); + let alice = &pairs[0]; + + ext.execute_with(|| { + // nonce is 0 + let mut transaction = default_erc20_creation_transaction(alice); + transaction.nonce = U256::from(1); + + assert_err!(Ethereum::validate_unsigned(TransactionSource::External, &Call::transact(transaction.clone())), InvalidTransaction::Future); + + // nonce is 1 + assert_ok!(Ethereum::execute( alice.address, - transaction, - ); + default_erc20_creation_transaction(alice), + )); + + transaction.nonce = U256::from(0); + + assert_err!(Ethereum::validate_unsigned(TransactionSource::External, &Call::transact(transaction)), InvalidTransaction::Stale); }); } @@ -100,10 +125,10 @@ fn contract_constructor_should_get_executed() { let alice_storage_address = storage_address(alice.address, H256::zero()); ext.execute_with(|| { - Ethereum::execute( + assert_ok!(Ethereum::execute( alice.address, default_erc20_creation_transaction(alice), - ); + )); assert_eq!(Evm::account_storages( erc20_address, alice_storage_address ), H256::from_str("ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff").unwrap()) @@ -144,7 +169,7 @@ fn invalid_signature_should_be_ignored() { assert_noop!(Ethereum::transact( Origin::none(), transaction, - ), "Recover public key failed"); + ), Error::::InvalidSignature); }); } @@ -157,10 +182,10 @@ fn contract_should_be_created_at_given_address() { let erc20_address = contract_address(alice.address, 0); ext.execute_with(|| { - Ethereum::execute( + assert_ok!(Ethereum::execute( alice.address, default_erc20_creation_transaction(alice), - ); + )); assert_ne!(Evm::account_codes(erc20_address).len(), 0); }); }