Skip to content
This repository has been archived by the owner on Apr 4, 2024. It is now read-only.

feat!: Reject not replay-protected tx to prevent replay attack #1124

Merged
merged 12 commits into from
Jun 13, 2022

Conversation

yihuang
Copy link
Contributor

@yihuang yihuang commented Jun 10, 2022

Description

Closes: #1122

  • reject such txs in ante handler

For contributor use:

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

For admin use:

  • Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
  • Reviewers assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

Closes: evmos#1122

- reject such txs in ante handler
CHANGELOG.md Outdated Show resolved Hide resolved
app/ante/eth.go Outdated Show resolved Hide resolved
app/ante/eth.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jun 13, 2022

Codecov Report

Merging #1124 (40888a0) into main (8283530) will decrease coverage by 0.12%.
The diff coverage is 69.23%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1124      +/-   ##
==========================================
- Coverage   61.86%   61.74%   -0.13%     
==========================================
  Files          88       89       +1     
  Lines        7285     7303      +18     
==========================================
+ Hits         4507     4509       +2     
- Misses       2557     2570      +13     
- Partials      221      224       +3     
Impacted Files Coverage Δ
x/evm/keeper/migrations.go 66.66% <0.00%> (-33.34%) ⬇️
x/evm/types/tx_data.go 97.91% <ø> (ø)
x/evm/migrations/v2/migrate.go 42.85% <42.85%> (ø)
x/evm/module.go 52.85% <60.00%> (-0.88%) ⬇️
app/ante/eth.go 80.61% <100.00%> (-2.06%) ⬇️
x/evm/types/params.go 100.00% <100.00%> (ø)

@yihuang yihuang changed the title Reject not replay-protected tx to prevent replay attack feat!: Reject not replay-protected tx to prevent replay attack Jun 13, 2022
Copy link
Contributor

@crypto-facs crypto-facs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGMT! 🙏

Copy link
Contributor

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, see comments. Can we rename RejectUnprotected to RejectUnprotectedTx

We should also add a comment on NewTxDataFromTx and the proto type saying that the LegacyTx type only works in unprotected mode if the module parameter is disabled.

and on the proto file too

// on proto/ethermint/evm/tx.proto 

// LegacyTx is the transaction data of regular Ethereum transactions.
// NOTE: All non-protected transactions (i.e non EIP155 signed) will fail if the RejectUnprotectedTx parameter is enabled. 
message LegacyTx { 

x/evm/types/params.go Outdated Show resolved Hide resolved
x/feemarket/module.go Outdated Show resolved Hide resolved
app/ante/eth_test.go Outdated Show resolved Hide resolved
x/evm/keeper/migrations.go Show resolved Hide resolved
x/evm/keeper/migrations.go Outdated Show resolved Hide resolved
x/evm/keeper/migrations.go Outdated Show resolved Hide resolved
Co-authored-by: Federico Kunze Küllmer <31522760+fedekunze@users.noreply.github.com>
@fedekunze
Copy link
Contributor

fedekunze commented Jun 13, 2022

@yihuang we should also add a non-breaking change to the JSON-RPC config to disable this (see geth flag below). I guess that some node providers will want this functionality even when the chain supports non-protected txs:

--rpc.allow-unprotected-txs         Allow for unprotected (non EIP155 signed) transactions to be submitted via RPC

We can add this in a follow-up PR

x/evm/types/params.go Outdated Show resolved Hide resolved
x/evm/types/params.go Outdated Show resolved Hide resolved
x/evm/types/params.go Outdated Show resolved Hide resolved
x/evm/types/params.go Outdated Show resolved Hide resolved
yihuang and others added 3 commits June 13, 2022 17:10
Closes: evmos#1122

- reject such txs in ante handler

add reject unprotected parameter

Update CHANGELOG.md

Apply suggestions from code review

Co-authored-by: Federico Kunze Küllmer <31522760+fedekunze@users.noreply.github.com>

pr suggestions

add unit test case

use var
Co-authored-by: Federico Kunze Küllmer <31522760+fedekunze@users.noreply.github.com>
@yihuang
Copy link
Contributor Author

yihuang commented Jun 13, 2022

@yihuang we should also add a non-breaking change to the JSON-RPC config to disable this (see geth flag below). I guess that some node providers will want this functionality even when the chain supports non-protected txs:

--rpc.allow-unprotected-txs         Allow for unprotected (non EIP155 signed) transactions to be submitted via RPC

We can add this in a follow-up PR

But we have implemented in consensus parameters, do you mean if it's not rejected in consensus level, add a flag to reject in mempool or rpc level only?

@yihuang
Copy link
Contributor Author

yihuang commented Jun 13, 2022

Looks good, see comments. Can we rename RejectUnprotected to RejectUnprotectedTx

We should also add a comment on NewTxDataFromTx and the proto type saying that the LegacyTx type only works in unprotected mode if the module parameter is disabled.

and on the proto file too

// on proto/ethermint/evm/tx.proto 

// LegacyTx is the transaction data of regular Ethereum transactions.
// NOTE: All non-protected transactions (i.e non EIP155 signed) will fail if the RejectUnprotectedTx parameter is enabled. 
message LegacyTx { 

done

@yihuang yihuang requested a review from fedekunze June 13, 2022 09:37
@fedekunze fedekunze enabled auto-merge (squash) June 13, 2022 09:40
@fedekunze fedekunze merged commit 8f932dd into evmos:main Jun 13, 2022
@yihuang yihuang deleted the reject-unprotected-tx branch June 13, 2022 09:43
devon-chain pushed a commit to PundiAI/ethermint that referenced this pull request Jun 20, 2022
…vmos#1124)

* Reject not replay-protected tx to prevent replay attack

Closes: evmos#1122

- reject such txs in ante handler

* add reject unprotected parameter

* Update CHANGELOG.md

* Apply suggestions from code review

Co-authored-by: Federico Kunze Küllmer <31522760+fedekunze@users.noreply.github.com>

* pr suggestions

* add unit test case

* Reject not replay-protected tx to prevent replay attack

Closes: evmos#1122

- reject such txs in ante handler

add reject unprotected parameter

Update CHANGELOG.md

Apply suggestions from code review

Co-authored-by: Federico Kunze Küllmer <31522760+fedekunze@users.noreply.github.com>

pr suggestions

add unit test case

use var

* add migrations

* Apply suggestions from code review

Co-authored-by: Federico Kunze Küllmer <31522760+fedekunze@users.noreply.github.com>

* rename

* update comments

Co-authored-by: Federico Kunze Küllmer <31522760+fedekunze@users.noreply.github.com>
yihuang added a commit to yihuang/ethermint that referenced this pull request Jul 14, 2022
yihuang added a commit to crypto-org-chain/ethermint that referenced this pull request Jul 15, 2022
yihuang added a commit to yihuang/cronos that referenced this pull request Jul 15, 2022
Solution:
- backport a non-breaking version of evmos/ethermint#1124

add integration test
yihuang added a commit to yihuang/cronos that referenced this pull request Jul 15, 2022
Solution:
- backport a non-breaking version of evmos/ethermint#1124

add integration test
yihuang added a commit to crypto-org-chain/cronos that referenced this pull request Jul 15, 2022
* Problem: replay un-protected tx is not rejected

Solution:
- backport a non-breaking version of evmos/ethermint#1124

add integration test

* changelog

* fix lint
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
C:x/evm EVM module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

enforce eip-155 to protect against replay
3 participants