-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
Add a reentrancy protection to EIP 1283 #1701
Conversation
EIPS/eip-1283.md
Outdated
@@ -63,7 +63,8 @@ the following logic: | |||
gas to refund counter. | |||
* If *original value* does not equal *current value* (this storage | |||
slot is dirty), 200 gas is deducted. Apply both of the following | |||
clauses. | |||
clauses. If *gasleft* is less then or equal 2300, fail the transaction |
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 would recommend to apply the gasleft check in the beginning, before any of the branches here are applied.
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.
Done, please see cf76bba
Co-Authored-By: alex-forshtat-tbk <alex@tabookey.com>
IMO this would have been better off as a stand-alone EIP, so fork definitions in node impl-ns don't have to invent branching Constantinoples. As described here. |
What do you think of replacing 200 gas cost with 2300 gas cost and 2100 gas refund? |
I think that as refund is limited to half of the transaction gas cost, this would in some cases limit the benefits of this EIP. Why do you think this is a better solution? |
Recommend changing 2300 -> 1600. As noted in the ChainSecurity article, the |
2300 is a magic number inherited from The former covers cases where The former makes an existing uncodified invariant explicit, whereas the latter guards against one pattern only. |
1283 is final and describes something implemented on test networks and in clients. If you'd like to propose an alternative version, please open a new EIP for that. |
UPD: As per @Arachnid 's request, moved into a separate EIP: #1706
An attack is described in:
https://medium.com/chainsecurity/constantinople-enables-new-reentrancy-attack-ace4088297d9
This is easily mitigated if SSTORE is not allowed in low gasleft state, without breaking the backward compatibility and the original intention of this EIP.