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

Set an upper bound to gasWanted to prevent DoS attack #991

Merged
merged 2 commits into from
Mar 16, 2022
Merged

Set an upper bound to gasWanted to prevent DoS attack #991

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3d5c65c
Set an upper bound to gasWanted to prevent DoS attack.
yihuang Mar 15, 2022
c2a05b3
Merge branch 'main' into fix-mempool
fedekunze Mar 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
### Bug Fixes

* (rpc) [tharsis#990](https://github.com/tharsis/ethermint/pull/990) Calculate reward values from all `MsgEthereumTx` from a block in `eth_feeHistory`.
* (ante) [tharsis#991](https://github.com/tharsis/ethermint/pull/991) Set an upper bound to gasWanted to prevent DoS attack.

## [v0.11.0] - 2022-03-06

Expand Down
15 changes: 13 additions & 2 deletions app/ante/eth.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ import (
ethtypes "github.com/ethereum/go-ethereum/core/types"
)

const MaxTxGasWanted uint64 = 500000

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@calvinaco calvinaco Mar 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@thomas-nguy thomas-nguy Mar 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@yihuang yihuang Mar 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@yihuang yihuang Mar 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

ahh yes true, the evm uses infinite, thanks yihuang

// EthSigVerificationDecorator validates an ethereum signatures
type EthSigVerificationDecorator struct {
evmKeeper EVMKeeper
Expand Down Expand Up @@ -171,7 +173,6 @@ func (egcd EthGasConsumeDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simula
london := ethCfg.IsLondon(blockHeight)
evmDenom := params.EvmDenom
gasWanted := uint64(0)

var events sdk.Events

for _, msg := range tx.GetMsgs() {
Expand All @@ -184,7 +185,17 @@ func (egcd EthGasConsumeDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simula
if err != nil {
return ctx, sdkerrors.Wrap(err, "failed to unpack tx data")
}
gasWanted += txData.GetGas()

if ctx.IsCheckTx() {
// We can't trust the tx gas limit, because we'll refund the unused gas.
if txData.GetGas() > MaxTxGasWanted {
gasWanted += MaxTxGasWanted
} else {
gasWanted += txData.GetGas()
}
} else {
gasWanted += txData.GetGas()
}

fees, err := egcd.evmKeeper.DeductTxCostsFromUserBalance(
ctx,
Expand Down
2 changes: 1 addition & 1 deletion app/ante/eth_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,7 +268,7 @@ func (suite AnteTestSuite) TestEthGasConsumeDecorator() {
{
"success",
tx2,
tx2GasLimit,
ante.MaxTxGasWanted, // it's capped
func() {
vmdb.AddBalance(addr, big.NewInt(1000000))

Expand Down