-
Notifications
You must be signed in to change notification settings - Fork 689
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
gas: fix the computation of self.promises_gas
#8179
Conversation
With the wrapping computation of `new_used_gas` any contract is in full control of this value, including those that are actually less than either operand (due to overflows.) This is not actually a big deal by itself, if not for the fact that it gives an opportunity for the attacker to nuke the entire network out of service. The in-line comments largely explain the reasoning behind the fix and why it preserves the protocol-facing behaviour, thus not necessitating a protocol version change. To reiterate what the comments say, basically the only case that we want to resolve is for when the attacker picks a value of `new_used_gas` such that it is less than `burnt_gas`. Instead of asserting and aborting we simply set `self.promises_gas = 0`.
// replacing the wrapping subtraction with a saturating one we make sure that the | ||
// resulting value of `self.promises_gas` is well behaved (becomes 0.) All a potential | ||
// attacker has achieved in this case is throwing some of their gas away. | ||
self.promises_gas = used_gas_limit.saturating_sub(self.fast_counter.burnt_gas); |
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.
Given this is for the next release, do we want to take advantage of it to also fix the TODO just two lines above this diff, and just set self.promises_gas to 0 in the new protocol version? I’m thinking it’d allow simplifying quite a lot the comments, as the failure mode would become obvious.
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 don’t feel like this PR is the right place for the more involved changes to this code. Any further fixes for this issue should be part of fixing the referenced issue (#5148) proper, and I don’t believe we have resources available to (re-)assign somebody to this immediately.
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 we should merge this as-is to reach parity with the rc versions. Follow-up fixes can be discussed in separate issues.
With the wrapping computation of
new_used_gas
any contract is in fullcontrol of this value, including those that are actually less than
either operand (due to overflows.)
This is not actually a big deal by itself, if not for the fact that it
gives an opportunity for the attacker to nuke the entire network out of
service.
The in-line comments largely explain the reasoning behind the fix and
why it preserves the protocol-facing behaviour, thus not necessitating a
protocol version change. To reiterate what the comments say, basically
the only case that we want to resolve is for when the attacker picks
a value of
new_used_gas
such that it is less thanburnt_gas
. Insteadof asserting and aborting we simply set
self.promises_gas = 0
.The fix in this PR has been included in 1.29.3 (72d4a4d) and 1.30.0-rc.6 (4290758).