Set an upper bound to gasWanted to prevent DoS attack

[yihuang]

Description

Closes: #989

the gasWanted returned in check tx is set to min(500000, msg.GetGas()).

---

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)

[loredanacirstea]

Now, we are back to the issue where the proposed block includes many transactions that will never get executed. E.g. I ran the test with 50 tx of ~700k gas, all of them were included in the first block, but only 12 were executed.

I do not have a solution yet for solving both these problems, but I am thinking about:

• calculating gasUsed for this gasWanted limit in the ante handler
• looking in more detail at how blocks are processed and how gasWanted works in general for the cosmos sdk. I am wondering if this issue (ddos vs. including too many tx because of gasWanted) is a general cosmos sdk problem and if not, why not.

[loredanacirstea]

I just saw the list of proposals here #989 (comment) and the long term solution sounds right

We can run the tx in the PrepareProposal phase, and filter the preliminary transaction list according to the gas used.

This temporary fix is ok from my side.

[yihuang]

Now, we are back to the issue where the proposed block includes many transactions that will never get executed. E.g. I ran the test with 50 tx of ~700k gas, all of them were included in the first block, but only 12 were executed.

I do not have a solution yet for solving both these problems, but I am thinking about:

• calculating gasUsed for this gasWanted limit in the ante handler

• looking in more detail at how blocks are processed and how gasWanted works in general for the cosmos sdk. I am wondering if this issue (ddos vs. including too many tx because of gasWanted) is a general cosmos sdk problem and if not, why not.

Cosmos-sdk don't refund unused gases, so it's not an issue there.

[calvinaco]

Although this is treated as a temporary fix, I want to propose this idea for further discussion that if we can make this slightly smarter. For example

1. Can we peek into the data field of the EVM Tx, if it is a simple transfer with no data, gasWanted is 21000
2. Otherwise, return a larger gasWanted
   1. Return a larger constant
   2. Calculate the gasWanted as MIN(21000, gasLimit / n) where n can be further decided.

For 2-ii, an attacker could still specify a very large gas limit with this, so this approach would potentially require to charge the sender the MAX(gasWanted, gasLimit/n) to create a base line of defence ... something borrowed from #989.

However, this requires a change of the gas refund approach, which is a bigger decision to make than the tx inclusion logic. Charging the user the gasPrice*gasLimit is common on Cosmos world but probably not on ETH world.

[thomas-nguy]

how about still use the gasLimit but put a maximum limitation so that there is still a "minimum" of transaction that can be accepted?

Someone wouldnt be able to DOS without spending fair amount of gas in that case?

I guess if someone really send a transaction that consume gas over this limit, then we will still have the previous issues of transactions being included but not be executed.

Depending on the value of the max limitation, only one case could be mitigated

[yihuang]

put a maximum limitation to gasLimit sounds good.
pick an arbitrary value: 500000?

[itsdevbear]

Isn't this potentially breaking? In a theoretical world there could be a codepath that requires more than 500k gas to execute. If a chain upgrades to this version of ethermint it could fully brick someones smart contract.

Temporarily I guess it's fine though

[yihuang]

Isn't this potentially breaking? In a theoretical world there could be a codepath that requires more than 500k gas to execute. If a chain upgrades to this version of ethermint it could fully brick someones smart contract.

Temporarily I guess it's fine though

it doesn't affect the tx execution, because the gas meter is infinite (with limit), so only affects the gasWanted returned to tendermint which uses it to decide the tx inclusion.

[itsdevbear]

ahh yes true, the evm uses infinite, thanks yihuang

[codecov]

Codecov Report

Merging #991 (c2a05b3) into main (889ff2b) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #991      +/-   ##
==========================================
+ Coverage   59.03%   59.08%   +0.04%     
==========================================
  Files          79       79              
  Lines        6518     6525       +7     
==========================================
+ Hits         3848     3855       +7     
  Misses       2452     2452              
  Partials      218      218              

Impacted Files	Coverage Δ
app/ante/eth.go	82.37% <100.00%> (+0.38%)	⬆️

[fedekunze]

Temp solution LGTM, let's continue the discussion on an issue.

[loredanacirstea]

Temp solution LGTM.
Note: Contract deployments are generally > 800k - 1mil gas. But we can increase the MaxTxGasWanted later if we see issues.

[fedekunze]

Note: Contract deployments are generally > 800k - 1mil gas. But we can increase the MaxTxGasWanted later if we see issues.

@yihuang maybe we can use a parameter on the EVM module for this?

[yihuang]

Note: Contract deployments are generally > 800k - 1mil gas. But we can increase the MaxTxGasWanted later if we see issues.

@yihuang maybe we can use a parameter on the EVM module for this?

add an config item in app.toml?