Fix check of EIP-3607 transaction for internal calls by koushiro · Pull Request #1018 · polkadot-evm/frontier

koushiro · 2023-03-14T07:32:09Z

Only check the restrictions of EIP-3607 if the source of the EVM operation is from an external transaction.

If the source of this EVM operation is from an internal call, like from eth_call or eth_estimateGas RPC, we will skip the checks for the EIP-3607.

related PR and discuss: #905

cc @tgmichel @nanocryk

Only check the restrictions of EIP-3607 if the source of the EVM operation is from an external transaction. If the source of this EVM operation is from an internal call, like from `eth_call` or `eth_estimateGas` RPC, we will skip the checks for the EIP-3607.

koushiro · 2023-03-14T07:35:56Z

@sorpaas PTAL

koushiro · 2023-03-14T08:00:59Z

frame/evm/src/runner/stack.rs

+		// Only check the restrictions of EIP-3607 if the source of the EVM operation is from an external transaction.
+		// If the source of this EVM operation is from an internal call, like from `eth_call` or `eth_estimateGas` RPC,
+		// we will skip the checks for the EIP-3607.
+		if is_transactional {
+			// EIP-3607: https://eips.ethereum.org/EIPS/eip-3607
+			// Do not allow transactions for which `tx.sender` has any code deployed.
+			//
+			// We extend the principle of this EIP to also prevent `tx.sender` to be the address
+			// of a precompile. While mainnet Ethereum currently only has stateless precompiles,
+			// projects using Frontier can have stateful precompiles that can manage funds or
+			// which calls other contracts that expects this precompile address to be trustworthy.
+			if !<AccountCodes<T>>::get(source).is_empty() || precompiles.is_precompile(source) {
+				return Err(RunnerError {
+					error: Error::<T>::TransactionMustComeFromEOA,
+					weight,
+				});
+			}
 		}


Suggested change

// Only check the restrictions of EIP-3607 if the source of the EVM operation is from an external transaction.

// If the source of this EVM operation is from an internal call, like from `eth_call` or `eth_estimateGas` RPC,

// we will skip the checks for the EIP-3607.

if is_transactional {

// EIP-3607: https://eips.ethereum.org/EIPS/eip-3607

// Do not allow transactions for which `tx.sender` has any code deployed.

//

// We extend the principle of this EIP to also prevent `tx.sender` to be the address

// of a precompile. While mainnet Ethereum currently only has stateless precompiles,

// projects using Frontier can have stateful precompiles that can manage funds or

// which calls other contracts that expects this precompile address to be trustworthy.

if !<AccountCodes<T>>::get(source).is_empty() || precompiles.is_precompile(source) {

return Err(RunnerError {

error: Error::<T>::TransactionMustComeFromEOA,

weight,

});

}

}

// Only check the restrictions of EIP-3607 if the source of the EVM operation is from an external transaction.

// If the source of this EVM operation is from an internal call, like from `eth_call` or `eth_estimateGas` RPC,

// we will skip the checks for the EIP-3607.

//

// EIP-3607: https://eips.ethereum.org/EIPS/eip-3607

// Do not allow transactions for which `tx.sender` has any code deployed.

//

// We extend the principle of this EIP to also prevent `tx.sender` to be the address

// of a precompile. While mainnet Ethereum currently only has stateless precompiles,

// projects using Frontier can have stateful precompiles that can manage funds or

// which calls other contracts that expects this precompile address to be trustworthy.

if is_transactional && !<AccountCodes<T>>::get(source).is_empty() || precompiles.is_precompile(source) {

return Err(RunnerError {

error: Error::<T>::TransactionMustComeFromEOA,

weight,

});

}

It seems that the moonbeam team have extended the principle of EIP-3607, should I change it to something like this? @tgmichel @nanocryk

If the precompile is stateful, maybe calling eth_call with this stateful precompile will cause some problems? I'm not sure 😕

Good question, I'd say in non-transactional context we don't care, @nanocryk to confirm

Sounds good to me

Sounds good to me

@nanocryk so calling non-transactional call with stateful precompiles as the source will be fine, right?

Please apply the other suggestions and change this to a single if statement.

tgmichel

Indeed this was missing, thank you @koushiro! A test for it would be great.

koushiro · 2023-03-14T11:03:17Z

Once this PR is merged, @sorpaas can you cherry-pick it to the polkadot-v0.9.36, polkadot-v0.9.37 and polkadot-v0.9.38 branches？
At the same time, downstream developers who have used these branches can upgrade the runtime to apply the changes .

sorpaas

LGTM otherwise.

sorpaas · 2023-03-14T14:44:33Z

frame/evm/src/runner/stack.rs

+		// Only check the restrictions of EIP-3607 if the source of the EVM operation is from an external transaction.
+		// If the source of this EVM operation is from an internal call, like from `eth_call` or `eth_estimateGas` RPC,
+		// we will skip the checks for the EIP-3607.
+		if is_transactional {
+			// EIP-3607: https://eips.ethereum.org/EIPS/eip-3607
+			// Do not allow transactions for which `tx.sender` has any code deployed.
+			//
+			// We extend the principle of this EIP to also prevent `tx.sender` to be the address
+			// of a precompile. While mainnet Ethereum currently only has stateless precompiles,
+			// projects using Frontier can have stateful precompiles that can manage funds or
+			// which calls other contracts that expects this precompile address to be trustworthy.
+			if !<AccountCodes<T>>::get(source).is_empty() || precompiles.is_precompile(source) {
+				return Err(RunnerError {
+					error: Error::<T>::TransactionMustComeFromEOA,
+					weight,
+				});
+			}
 		}


Please apply the other suggestions and change this to a single if statement.

* Improve check of EIP-3607 Only check the restrictions of EIP-3607 if the source of the EVM operation is from an external transaction. If the source of this EVM operation is from an internal call, like from `eth_call` or `eth_estimateGas` RPC, we will skip the checks for the EIP-3607. * fix eip-3607 tests * apply review suggtions

koushiro requested a review from sorpaas as a code owner March 14, 2023 07:32

Improve check of EIP-3607

e9c8ed3

Only check the restrictions of EIP-3607 if the source of the EVM operation is from an external transaction. If the source of this EVM operation is from an internal call, like from `eth_call` or `eth_estimateGas` RPC, we will skip the checks for the EIP-3607.

koushiro commented Mar 14, 2023

View reviewed changes

tgmichel approved these changes Mar 14, 2023

View reviewed changes

fix eip-3607 tests

a372656

nbaztec mentioned this pull request Mar 14, 2023

[MOON-2268] skip eip-3607 transaction verification for internal calls moonbeam-foundation/frontier#165

Merged

koushiro changed the title ~~Improve check of EIP-3607~~ Fix check of EIP-3607 transaction for internal calls Mar 14, 2023

sorpaas reviewed Mar 14, 2023

View reviewed changes

apply review suggtions

5e73056

koushiro requested a review from sorpaas March 14, 2023 15:45

sorpaas approved these changes Mar 15, 2023

View reviewed changes

sorpaas merged commit 8a5492f into polkadot-evm:master Mar 15, 2023

koushiro deleted the improve-eip3607-check branch March 15, 2023 13:20

tgmichel mentioned this pull request Mar 30, 2023

eth_call not working when sender is set to contract address moonbeam-foundation/moonbeam#2193

Closed

tgmichel mentioned this pull request Apr 12, 2023

Pin frontier df4e329 moonbeam-foundation/moonbeam#2221

Merged

dmitrylavrenov mentioned this pull request Oct 16, 2023

Bump to polkadot-sdk v1.1.0 as a base for substrate related deps humanode-network/humanode#803

Closed

37 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix check of EIP-3607 transaction for internal calls#1018

Fix check of EIP-3607 transaction for internal calls#1018
sorpaas merged 3 commits intopolkadot-evm:masterfrom
koushiro:improve-eip3607-check

koushiro commented Mar 14, 2023

Uh oh!

koushiro commented Mar 14, 2023

Uh oh!

koushiro Mar 14, 2023 •

edited

Loading

Uh oh!

koushiro Mar 14, 2023 •

edited

Loading

Uh oh!

tgmichel Mar 14, 2023

Uh oh!

nanocryk Mar 14, 2023

Uh oh!

koushiro Mar 14, 2023

Uh oh!

sorpaas Mar 14, 2023

Uh oh!

tgmichel left a comment

Uh oh!

koushiro commented Mar 14, 2023

Uh oh!

sorpaas left a comment

Uh oh!

sorpaas Mar 14, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

koushiro commented Mar 14, 2023

Uh oh!

koushiro commented Mar 14, 2023

Uh oh!

koushiro Mar 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

koushiro Mar 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tgmichel Mar 14, 2023

Choose a reason for hiding this comment

Uh oh!

nanocryk Mar 14, 2023

Choose a reason for hiding this comment

Uh oh!

koushiro Mar 14, 2023

Choose a reason for hiding this comment

Uh oh!

sorpaas Mar 14, 2023

Choose a reason for hiding this comment

Uh oh!

tgmichel left a comment

Choose a reason for hiding this comment

Uh oh!

koushiro commented Mar 14, 2023

Uh oh!

sorpaas left a comment

Choose a reason for hiding this comment

Uh oh!

sorpaas Mar 14, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

koushiro Mar 14, 2023 •

edited

Loading

koushiro Mar 14, 2023 •

edited

Loading