-
Notifications
You must be signed in to change notification settings - Fork 91
feat: support query gas limit flag #368
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
@@ -482,6 +482,7 @@ func NewExampleApp( | |||
&app.ConsensusParamsKeeper, | |||
&app.Erc20Keeper, | |||
tracer, | |||
cast.ToUint64(appOpts.Get(server.FlagQueryGasLimit)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this flag is meant to control the cosmos SDK query gas limit. im not sure if it makes sense to use it in this way.
imo it makes more sense to set the value like this:
baseapp.SetQueryGasLimit(cast.ToUint64(appOpts.Get(sdkserver.FlagQueryGasLimit))),
this will make the SDK set a gas limit in context during query executions. then we can update the keeper calls to respect the value set here: https://github.com/cosmos/cosmos-sdk/blob/553f8955c3214c21c2755811aae5a8f3e021f0f8/baseapp/abci.go#L1282-L1286
keep in mind, this would enforce a query gas limit for ALL gRPC queries. if we want a separate gas limit (i.e. no gas limit for SDK queries, but one for EVM), i suggest we create a separate flag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we want a separate gas limit (i.e. no gas limit for SDK queries, but one for EVM), i suggest we create a separate flag.
Do we want to support such use case? The FlagQueryGasLimit
is meant to set a max gas limit for all Rest/Grpc queries, intended to prevent DOS attack
My original thought is that the x/evm module rpcs is a subset of those queries (as they all endup calling cosmos level rest/grpc) and need to follow the same limitation. Adding an extra flag may bring an additional layer of complexity, and confuse users.
I can do the change if it is really needed though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
main reason i would think to separate them is because im unsure of the relation between eth gas units and SDK gas units. i know eth gas costs are quite calculated and well thought out, whereas the SDK's feels more arbitrarily chosen.
any thoughts here? @aljo242? i know youve mentioned we dont want to have too many ways to configure things these days, so maybe lets just go with a catch-all flag
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes sense to put it under the one flag
0c54a5c
to
17f1ec1
Compare
@technicallyty I have set the query gas limit in the baseapp by default as well for the other msgs but unfortunately that alone won't fix the issue because it does not apply to the debug_* context. We may to dig deeper but the cosmos gasmeter mechanism seems to be ignored during the vm execution or at least is handled differently. |
Description
This PR enforces a max gas cap when calling the endpoint
eth_call
oreth_estimateGas
Currently, an attacker can bypass the gas cap by querying directly at the cosmos level and specifying a very large gas cap
If done in a malicious way, an attacker could use a high GasCap to execute an infinite loop function, potentially impacting node performance & crashing the node
Step to reproduce
Deploy
Call
This PR remediates it by setting a global query gas limit
Closes: #372
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
main
branchReviewers Checklist
All items are required.
Please add a note if the item is not applicable
and please add your handle next to the items reviewed
if you only reviewed selected items.
I have...
Unreleased
section inCHANGELOG.md