diff --git a/client/rpc/src/eth/mod.rs b/client/rpc/src/eth/mod.rs index 3e25c1f37c..bd560f4ee2 100644 --- a/client/rpc/src/eth/mod.rs +++ b/client/rpc/src/eth/mod.rs @@ -85,6 +85,8 @@ pub struct Eth { /// When using eth_call/eth_estimateGas, the maximum allowed gas limit will be /// block.gas_limit * execute_gas_limit_multiplier execute_gas_limit_multiplier: u64, + /// Whether RPC submission accepts legacy transactions without EIP-155 chain id. + rpc_allow_unprotected_txs: bool, forced_parent_hashes: Option>, /// Something that can create the inherent data providers for pending state. pending_create_inherent_data_providers: CIDP, @@ -113,6 +115,7 @@ where fee_history_cache: FeeHistoryCache, fee_history_cache_limit: FeeHistoryCacheLimit, execute_gas_limit_multiplier: u64, + rpc_allow_unprotected_txs: bool, forced_parent_hashes: Option>, pending_create_inherent_data_providers: CIDP, pending_consensus_data_provider: Option>>, @@ -130,6 +133,7 @@ where fee_history_cache, fee_history_cache_limit, execute_gas_limit_multiplier, + rpc_allow_unprotected_txs, forced_parent_hashes, pending_create_inherent_data_providers, pending_consensus_data_provider, @@ -326,6 +330,7 @@ where fee_history_cache, fee_history_cache_limit, execute_gas_limit_multiplier, + rpc_allow_unprotected_txs, forced_parent_hashes, pending_create_inherent_data_providers, pending_consensus_data_provider, @@ -345,6 +350,7 @@ where fee_history_cache, fee_history_cache_limit, execute_gas_limit_multiplier, + rpc_allow_unprotected_txs, forced_parent_hashes, pending_create_inherent_data_providers, pending_consensus_data_provider, diff --git a/client/rpc/src/eth/submit.rs b/client/rpc/src/eth/submit.rs index 8e26a83a1a..0eef4d81cd 100644 --- a/client/rpc/src/eth/submit.rs +++ b/client/rpc/src/eth/submit.rs @@ -37,6 +37,20 @@ use crate::{ internal_err, public_key, }; +fn is_unprotected_legacy_tx(transaction: ðereum::TransactionV3) -> bool { + matches!( + transaction, + ethereum::TransactionV3::Legacy(tx) if tx.signature.chain_id().is_none() + ) +} + +fn should_reject_unprotected_legacy_tx( + transaction: ðereum::TransactionV3, + allow_unprotected_txs: bool, +) -> bool { + is_unprotected_legacy_tx(transaction) && !allow_unprotected_txs +} + impl Eth where B: BlockT, @@ -48,6 +62,21 @@ where CT: ConvertTransaction<::Extrinsic> + 'static, CIDP: CreateInherentDataProviders + Send + 'static, { + fn ensure_unprotected_legacy_tx_allowed( + &self, + transaction: ðereum::TransactionV3, + ) -> RpcResult<()> { + if should_reject_unprotected_legacy_tx(transaction, self.rpc_allow_unprotected_txs) { + // Runtime validation remains authoritative since peers can still gossip transactions + // over p2p and bypass RPC submission. + return Err(internal_err( + "unprotected legacy transactions are not allowed by RPC policy", + )); + } + + Ok(()) + } + pub async fn send_transaction(&self, request: TransactionRequest) -> RpcResult { let from = match request.from { Some(from) => from, @@ -143,6 +172,7 @@ where Some(transaction) => transaction, None => return Err(internal_err("no signer available")), }; + self.ensure_unprotected_legacy_tx_allowed(&transaction)?; let transaction_hash = transaction.hash(); let extrinsic = self.convert_transaction(block_hash, transaction)?; @@ -165,6 +195,7 @@ where Ok(transaction) => transaction, Err(_) => return Err(internal_err("decode transaction failed")), }; + self.ensure_unprotected_legacy_tx_allowed(&transaction)?; let transaction_hash = transaction.hash(); let block_hash = self.client.info().best_hash; @@ -290,3 +321,78 @@ where } } } + +#[cfg(test)] +mod tests { + use super::{is_unprotected_legacy_tx, should_reject_unprotected_legacy_tx}; + use ethereum::{ + legacy::TransactionSignature as LegacyTransactionSignature, LegacyTransaction, + TransactionAction, TransactionV3, + }; + use rlp::RlpStream; + use sp_core::{hashing::keccak_256, H160, H256, U256}; + + fn legacy_tx_with_v(v: u64) -> TransactionV3 { + let nonce = U256::zero(); + let gas_price = U256::from(1u8); + let gas_limit = U256::from(21_000u64); + let action = TransactionAction::Call(H160::default()); + let value = U256::zero(); + let input = Vec::new(); + + let mut stream = RlpStream::new_list(6); + stream.append(&nonce); + stream.append(&gas_price); + stream.append(&gas_limit); + stream.append(&action); + stream.append(&value); + stream.append(&input); + let hash = H256::from(keccak_256(&stream.out())); + let msg = libsecp256k1::Message::parse(hash.as_fixed_bytes()); + let secret = libsecp256k1::SecretKey::parse_slice(&[1u8; 32]).expect("valid secret key"); + let (signature, _) = libsecp256k1::sign(&msg, &secret); + let rs = signature.serialize(); + + TransactionV3::Legacy(LegacyTransaction { + nonce, + gas_price, + gas_limit, + action, + value, + input, + signature: LegacyTransactionSignature::new( + v, + H256::from_slice(&rs[0..32]), + H256::from_slice(&rs[32..64]), + ) + .expect("valid legacy signature"), + }) + } + + #[test] + fn detects_unprotected_legacy_transaction() { + let tx = legacy_tx_with_v(27); + + assert!(is_unprotected_legacy_tx(&tx)); + } + + #[test] + fn ignores_protected_legacy_transaction() { + let tx = legacy_tx_with_v(37); + + assert!(!is_unprotected_legacy_tx(&tx)); + } + + #[test] + fn reject_decision_respects_policy() { + let unprotected_legacy = legacy_tx_with_v(27); + assert!(should_reject_unprotected_legacy_tx( + &unprotected_legacy, + false + )); + assert!(!should_reject_unprotected_legacy_tx( + &unprotected_legacy, + true + )); + } +} diff --git a/frame/ethereum/src/lib.rs b/frame/ethereum/src/lib.rs index 49debc0fba..54cbd4f92b 100644 --- a/frame/ethereum/src/lib.rs +++ b/frame/ethereum/src/lib.rs @@ -209,6 +209,8 @@ pub mod pallet { type PostLogContent: Get; /// The maximum length of the extra data in the Executed event. type ExtraDataLength: Get; + /// Whether transactional ethereum calls accept legacy transactions without EIP-155 chain id. + type AllowUnprotectedTxs: Get; } pub mod config_preludes { @@ -225,6 +227,7 @@ pub mod pallet { parameter_types! { pub const PostBlockAndTxnHashes: PostLogContent = PostLogContent::BlockAndTxnHashes; + pub const AllowUnprotectedTxs: bool = false; } #[register_default_impl(TestDefaultConfig)] @@ -232,6 +235,7 @@ pub mod pallet { type StateRoot = IntermediateStateRoot; type PostLogContent = PostBlockAndTxnHashes; type ExtraDataLength = ConstU32<30>; + type AllowUnprotectedTxs = AllowUnprotectedTxs; } } @@ -561,6 +565,7 @@ impl Pallet { base_fee, chain_id: T::ChainId::get(), is_transactional: true, + allow_unprotected_txs: T::AllowUnprotectedTxs::get(), }, transaction_data.clone().into(), weight_limit, @@ -1010,6 +1015,7 @@ impl Pallet { base_fee, chain_id: T::ChainId::get(), is_transactional: true, + allow_unprotected_txs: T::AllowUnprotectedTxs::get(), }, transaction_data.into(), weight_limit, diff --git a/frame/ethereum/src/mock.rs b/frame/ethereum/src/mock.rs index 0cd093be83..9630c523f8 100644 --- a/frame/ethereum/src/mock.rs +++ b/frame/ethereum/src/mock.rs @@ -90,6 +90,7 @@ impl FindAuthor for FindAuthorTruncated { parameter_types! { pub const TransactionByteFee: u64 = 1; pub const GasLimitStorageGrowthRatio: u64 = 0; + pub static AllowUnprotectedTxs: bool = false; // Alice is allowed to create contracts via CREATE and CALL(CREATE) pub AllowedAddressesCreate: Vec = vec![H160::from_str("0x1a642f0e3c3af545e7acbd38b07251b3990914f1").expect("alice address")]; pub AllowedAddressesCreateInner: Vec = vec![H160::from_str("0x1a642f0e3c3af545e7acbd38b07251b3990914f1").expect("alice address")]; @@ -111,7 +112,9 @@ impl pallet_evm::Config for Test { } #[derive_impl(crate::config_preludes::TestDefaultConfig)] -impl Config for Test {} +impl Config for Test { + type AllowUnprotectedTxs = AllowUnprotectedTxs; +} impl fp_self_contained::SelfContainedCall for RuntimeCall { type SignedInfo = H160; @@ -285,6 +288,17 @@ impl LegacyUnsignedTransaction { H256::from(keccak_256(&stream.out())) } + fn unprotected_signing_hash(&self) -> H256 { + let mut stream = RlpStream::new_list(6); + stream.append(&self.nonce); + stream.append(&self.gas_price); + stream.append(&self.gas_limit); + stream.append(&self.action); + stream.append(&self.value); + stream.append(&self.input); + H256::from(keccak_256(&stream.out())) + } + pub fn sign(&self, key: &H256) -> Transaction { self.sign_with_chain_id(key, ChainId::get()) } @@ -315,6 +329,33 @@ impl LegacyUnsignedTransaction { signature: sig, }) } + + pub fn sign_without_chain_id(&self, key: &H256) -> Transaction { + let hash = self.unprotected_signing_hash(); + let msg = libsecp256k1::Message::parse(hash.as_fixed_bytes()); + let s = libsecp256k1::sign( + &msg, + &libsecp256k1::SecretKey::parse_slice(&key[..]).unwrap(), + ); + let sig = s.0.serialize(); + + let sig = LegacyTransactionSignature::new( + s.1.serialize() as u64 % 2 + 27, + H256::from_slice(&sig[0..32]), + H256::from_slice(&sig[32..64]), + ) + .unwrap(); + + Transaction::Legacy(ethereum::LegacyTransaction { + nonce: self.nonce, + gas_price: self.gas_price, + gas_limit: self.gas_limit, + action: self.action, + value: self.value, + input: self.input.clone(), + signature: sig, + }) + } } pub struct EIP2930UnsignedTransaction { diff --git a/frame/ethereum/src/tests/legacy.rs b/frame/ethereum/src/tests/legacy.rs index f19932bd68..05d64d966f 100644 --- a/frame/ethereum/src/tests/legacy.rs +++ b/frame/ethereum/src/tests/legacy.rs @@ -209,6 +209,54 @@ fn transaction_with_invalid_chain_id_should_fail_in_block() { }); } +#[test] +fn unprotected_transaction_should_fail_when_not_allowed() { + let (pairs, mut ext) = new_test_ext(1); + let alice = &pairs[0]; + + ext.execute_with(|| { + AllowUnprotectedTxs::set(false); + let transaction = + legacy_erc20_creation_unsigned_transaction().sign_without_chain_id(&alice.private_key); + + let call = crate::Call::::transact { transaction }; + let source = call.check_self_contained().unwrap().unwrap(); + let extrinsic = CheckedExtrinsic::<_, _, SignedExtra, _> { + signed: fp_self_contained::CheckedSignature::SelfContained(source), + function: RuntimeCall::Ethereum(call), + }; + let dispatch_info = extrinsic.get_dispatch_info(); + assert_err!( + extrinsic.apply::(&dispatch_info, 0), + TransactionValidityError::Invalid(InvalidTransaction::Custom( + fp_evm::TransactionValidationError::InvalidChainId as u8, + )) + ); + }); +} + +#[test] +fn unprotected_transaction_should_succeed_when_allowed() { + let (pairs, mut ext) = new_test_ext(1); + let alice = &pairs[0]; + + ext.execute_with(|| { + AllowUnprotectedTxs::set(true); + let transaction = + legacy_erc20_creation_unsigned_transaction().sign_without_chain_id(&alice.private_key); + + let call = crate::Call::::transact { transaction }; + let source = call.check_self_contained().unwrap().unwrap(); + let extrinsic = CheckedExtrinsic::<_, _, SignedExtra, _> { + signed: fp_self_contained::CheckedSignature::SelfContained(source), + function: RuntimeCall::Ethereum(call), + }; + let dispatch_info = extrinsic.get_dispatch_info(); + assert_ok!(extrinsic.apply::(&dispatch_info, 0)); + AllowUnprotectedTxs::set(false); + }); +} + #[test] fn contract_constructor_should_get_executed() { let (pairs, mut ext) = new_test_ext(1); diff --git a/frame/evm/src/runner/stack.rs b/frame/evm/src/runner/stack.rs index 90250f663e..1c3d6aa159 100644 --- a/frame/evm/src/runner/stack.rs +++ b/frame/evm/src/runner/stack.rs @@ -490,6 +490,7 @@ where base_fee, chain_id: T::ChainId::get(), is_transactional, + allow_unprotected_txs: true, }, fp_evm::CheckEvmTransactionInput { chain_id: Some(T::ChainId::get()), diff --git a/primitives/evm/src/validation.rs b/primitives/evm/src/validation.rs index 7943b90c5f..2efc8a8481 100644 --- a/primitives/evm/src/validation.rs +++ b/primitives/evm/src/validation.rs @@ -46,6 +46,7 @@ pub struct CheckEvmTransactionConfig<'config> { pub base_fee: U256, pub chain_id: u64, pub is_transactional: bool, + pub allow_unprotected_txs: bool, } #[derive(Debug)] @@ -144,10 +145,16 @@ impl<'config, E: From> CheckEvmTransaction<'config, pub fn with_chain_id(&self) -> Result<&Self, E> { // Chain id matches the one in the signature. - if let Some(chain_id) = self.transaction.chain_id { - if chain_id != self.config.chain_id { - return Err(TransactionValidationError::InvalidChainId.into()); + match self.transaction.chain_id { + Some(chain_id) => { + if chain_id != self.config.chain_id { + return Err(TransactionValidationError::InvalidChainId.into()); + } + } + None if self.config.is_transactional && !self.config.allow_unprotected_txs => { + return Err(TransactionValidationError::InvalidChainId.into()) } + None => {} } Ok(self) } @@ -369,6 +376,7 @@ mod tests { pub blockchain_base_fee: U256, pub blockchain_chain_id: u64, pub is_transactional: bool, + pub allow_unprotected_txs: bool, pub chain_id: Option, pub nonce: U256, pub gas_limit: U256, @@ -387,6 +395,7 @@ mod tests { blockchain_base_fee: U256::from(1_000_000_000u128), blockchain_chain_id: 42u64, is_transactional: true, + allow_unprotected_txs: false, chain_id: Some(42u64), nonce: U256::zero(), gas_limit: U256::from(21_000u64), @@ -406,6 +415,7 @@ mod tests { blockchain_base_fee, blockchain_chain_id, is_transactional, + allow_unprotected_txs, chain_id, nonce, gas_limit, @@ -423,6 +433,7 @@ mod tests { base_fee: blockchain_base_fee, chain_id: blockchain_chain_id, is_transactional, + allow_unprotected_txs, }, CheckEvmTransactionInput { chain_id, @@ -494,6 +505,18 @@ mod tests { }) } + fn transaction_without_chain_id<'config>( + is_transactional: bool, + allow_unprotected_txs: bool, + ) -> CheckEvmTransaction<'config, TestError> { + test_env(TestCase { + is_transactional, + allow_unprotected_txs, + chain_id: None, + ..Default::default() + }) + } + fn transaction_none_fee<'config>( is_transactional: bool, ) -> CheckEvmTransaction<'config, TestError> { @@ -721,6 +744,28 @@ mod tests { assert_eq!(res.unwrap_err(), TestError::InvalidChainId); } + #[test] + fn validate_chain_id_none_transactional_fails_by_default() { + let test = transaction_without_chain_id(true, false); + let res = test.with_chain_id(); + assert!(res.is_err()); + assert_eq!(res.unwrap_err(), TestError::InvalidChainId); + } + + #[test] + fn validate_chain_id_none_transactional_succeeds_when_allowed() { + let test = transaction_without_chain_id(true, true); + let res = test.with_chain_id(); + assert!(res.is_ok()); + } + + #[test] + fn validate_chain_id_none_non_transactional_succeeds() { + let test = transaction_without_chain_id(false, false); + let res = test.with_chain_id(); + assert!(res.is_ok()); + } + // Valid max fee per gas succeeds. #[test] fn validate_base_fee_succeeds() { @@ -941,6 +986,7 @@ mod tests { base_fee: U256::from(1_000_000_000u128), chain_id: 42u64, is_transactional: true, + allow_unprotected_txs: false, }, CheckEvmTransactionInput { chain_id: Some(42u64), @@ -978,6 +1024,7 @@ mod tests { base_fee: U256::from(1_000_000_000u128), chain_id: 42u64, is_transactional: true, + allow_unprotected_txs: false, }, CheckEvmTransactionInput { chain_id: Some(42u64), @@ -1015,6 +1062,7 @@ mod tests { base_fee: U256::from(1_000_000_000u128), chain_id: 42u64, is_transactional: true, + allow_unprotected_txs: false, }, CheckEvmTransactionInput { chain_id: Some(42u64), @@ -1047,6 +1095,7 @@ mod tests { base_fee: U256::from(1_000_000_000u128), chain_id: 42u64, is_transactional: true, + allow_unprotected_txs: false, }, CheckEvmTransactionInput { chain_id: Some(42u64), @@ -1080,6 +1129,7 @@ mod tests { base_fee: U256::from(1_000_000_000u128), chain_id: 42u64, is_transactional: true, + allow_unprotected_txs: false, }, CheckEvmTransactionInput { chain_id: Some(42u64), @@ -1112,6 +1162,7 @@ mod tests { base_fee: U256::from(1_000_000_000u128), chain_id: 42u64, is_transactional: true, + allow_unprotected_txs: false, }, CheckEvmTransactionInput { chain_id: Some(42u64), @@ -1145,6 +1196,7 @@ mod tests { base_fee: U256::from(1_000_000_000u128), chain_id: 42u64, is_transactional: true, + allow_unprotected_txs: false, }, CheckEvmTransactionInput { chain_id: Some(42u64), @@ -1177,6 +1229,7 @@ mod tests { base_fee: U256::from(1_000_000_000u128), chain_id: 42u64, is_transactional: true, + allow_unprotected_txs: false, }, CheckEvmTransactionInput { chain_id: Some(42u64), @@ -1211,6 +1264,7 @@ mod tests { base_fee: U256::from(1_000_000_000u128), chain_id: 42u64, is_transactional: false, // Non-transactional (dry-run) + allow_unprotected_txs: false, }, CheckEvmTransactionInput { chain_id: Some(42u64), diff --git a/template/node/src/eth.rs b/template/node/src/eth.rs index 6425807336..bd3a99838e 100644 --- a/template/node/src/eth.rs +++ b/template/node/src/eth.rs @@ -66,6 +66,10 @@ pub struct EthConfiguration { #[arg(long, default_value = "10")] pub execute_gas_limit_multiplier: u64, + /// Allow RPC submission of unprotected legacy transactions (without EIP-155 chain id). + #[arg(long, default_value_t = false)] + pub rpc_allow_unprotected_txs: bool, + /// Size in bytes of the LRU cache for block data. #[arg(long, default_value = "50")] pub eth_log_block_cache: usize, diff --git a/template/node/src/rpc/eth.rs b/template/node/src/rpc/eth.rs index 2aac340010..a15f56734b 100644 --- a/template/node/src/rpc/eth.rs +++ b/template/node/src/rpc/eth.rs @@ -59,6 +59,8 @@ pub struct EthDeps { /// Maximum allowed gas limit will be ` block.gas_limit * execute_gas_limit_multiplier` when /// using eth_call/eth_estimateGas. pub execute_gas_limit_multiplier: u64, + /// Allow RPC submission of unprotected legacy transactions. + pub rpc_allow_unprotected_txs: bool, /// Mandated parent hashes for a given block hash. pub forced_parent_hashes: Option>, /// Something that can create the inherent data providers for pending state @@ -116,6 +118,7 @@ where fee_history_cache, fee_history_cache_limit, execute_gas_limit_multiplier, + rpc_allow_unprotected_txs, forced_parent_hashes, pending_create_inherent_data_providers, } = deps; @@ -139,6 +142,7 @@ where fee_history_cache, fee_history_cache_limit, execute_gas_limit_multiplier, + rpc_allow_unprotected_txs, forced_parent_hashes, pending_create_inherent_data_providers, Some(Box::new(AuraConsensusDataProvider::new(client.clone()))), diff --git a/template/node/src/service.rs b/template/node/src/service.rs index baee221810..98a963f16f 100644 --- a/template/node/src/service.rs +++ b/template/node/src/service.rs @@ -429,6 +429,7 @@ where let max_past_logs = eth_config.max_past_logs; let max_block_range = eth_config.max_block_range; let execute_gas_limit_multiplier = eth_config.execute_gas_limit_multiplier; + let rpc_allow_unprotected_txs = eth_config.rpc_allow_unprotected_txs; let filter_pool = filter_pool.clone(); let frontier_backend = frontier_backend.clone(); let pubsub_notification_sinks = pubsub_notification_sinks.clone(); @@ -477,6 +478,7 @@ where fee_history_cache: fee_history_cache.clone(), fee_history_cache_limit, execute_gas_limit_multiplier, + rpc_allow_unprotected_txs, forced_parent_hashes: None, pending_create_inherent_data_providers, }; diff --git a/template/runtime/src/lib.rs b/template/runtime/src/lib.rs index f4f5a95079..c687de7ed9 100644 --- a/template/runtime/src/lib.rs +++ b/template/runtime/src/lib.rs @@ -388,12 +388,14 @@ impl pallet_evm::Config for Runtime { parameter_types! { pub const PostBlockAndTxnHashes: PostLogContent = PostLogContent::BlockAndTxnHashes; + pub const AllowUnprotectedTxs: bool = false; } impl pallet_ethereum::Config for Runtime { type StateRoot = pallet_ethereum::IntermediateStateRoot; type PostLogContent = PostBlockAndTxnHashes; type ExtraDataLength = ConstU32<30>; + type AllowUnprotectedTxs = AllowUnprotectedTxs; } parameter_types! { diff --git a/ts-tests/tests/test-transaction-cost.ts b/ts-tests/tests/test-transaction-cost.ts index bcde9388ef..96662d92b6 100644 --- a/ts-tests/tests/test-transaction-cost.ts +++ b/ts-tests/tests/test-transaction-cost.ts @@ -5,11 +5,11 @@ import { describeWithFrontier, customRequest } from "./util"; describeWithFrontier("Frontier RPC (Transaction cost)", (context) => { step("should take transaction cost into account and not submit it to the pool", async function () { - // Simple transfer with gas limit 0 manually signed to prevent web3 from rejecting client-side. + // Protected (EIP-155) legacy tx with gas limit 0, signed offline. const tx = await customRequest(context.web3, "eth_sendRawTransaction", [ - "0xf86180843b9aca00809412cb274aad8251c875c0bf6872b67d9983e53fdd01801ca00e28ba2dd3c5a3fd467\ - d4afd7aefb4a34b373314fff470bb9db743a84d674a0aa06e5994f2d07eafe1c37b4ce5471caecec29011f6f5b\ - f0b1a552c55ea348df35f", + "0xf86180843b9aca00809412cb274aad8251c875c0bf6872b67d9983e53fdd0180" + + "77a05e311790fcf3eef1ed6d9ee6ffeea8c44a677b52168240b5713dacb0b85cf203" + + "a05e55ddeb10d2ad84fdf6901ba0047d5c311de899552592e43f1d11b4947527c6", ]); let msg = "intrinsic gas too low"; expect(tx.error).to.include({