-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
EIP: Reject transactions from senders with deployed code #3607
Conversation
Hi! I'm a bot, and I wanted to automerge your PR, but couldn't because of the following issue(s):
|
Discussion to: #3608 |
EIPS/eip-reject-transactions-from-senders-with-deployed-code.md
Outdated
Show resolved
Hide resolved
EIPS/eip-reject-transactions-from-senders-with-deployed-code.md
Outdated
Show resolved
Hide resolved
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.
Well written, just a couple nits.
EIPS/eip-reject-transactions-from-senders-with-deployed-code.md
Outdated
Show resolved
Hide resolved
EIPS/eip-reject-transactions-from-senders-with-deployed-code.md
Outdated
Show resolved
Hide resolved
EIPS/eip-reject-transactions-from-senders-with-deployed-code.md
Outdated
Show resolved
Hide resolved
Co-authored-by: lightclient <14004106+lightclient@users.noreply.github.com>
Co-authored-by: lightclient <14004106+lightclient@users.noreply.github.com>
Co-authored-by: lightclient <14004106+lightclient@users.noreply.github.com>
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.
sorry, missed these in the initial review
Co-authored-by: lightclient <14004106+lightclient@users.noreply.github.com>
Co-authored-by: lightclient <14004106+lightclient@users.noreply.github.com>
```go | ||
// Make sure the sender is an EOA | ||
if codesize := st.state.GetCodeSize(st.msg.From()); codesize != 0 { | ||
return ErrSenderNoEOA | ||
} | ||
``` |
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.
Consider writing this in pseudocode or a more well known/easier to read language than Go. Also, consider extracting the assignment out of the conditional for a bit more clarity/readability.
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.
} | ||
``` | ||
|
||
An implementation in go-ethereum can be found [here](https://github.com/ethereum/go-ethereum/pull/23002) |
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.
Though we are no longer blocking PRs that include external links to transient things, I still recommend keeping these out of the EIP (same for similar links above).
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 link in particular would not be transient -- however the links to Hackmd would. What is the preferred format of these references? We could add them as an appendix? Or push them to a blog with a permalink?
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 the most preferable method is to add a PDF or markdown to this repository to the text. Here is the format for doing that: https://github.com/ethereum/EIPs/blob/master/EIPS/eip-1.md#auxiliary-files
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.
Text, JSON, PDF, code, etc. are all fine as assets, we just prefer they live permanently within the EIPs repository rather than living externally since organizations change names, repositories change names, GitHub may change its addressing format, etc. Also, someone should be able to read and understand this EIP in isolation without having context about go-ethereum, Go, etc.
2. A chain reorg can happen after a contract is deployed. If the reorg removes the contract deployment transaction the funds can still be accessed using the private key. | ||
3. A contract can self desctruct, with the stated intention that ERC20s (or other tokens) in the contract would be burned. However, they can now be accessed by a key for that address. | ||
|
||
All these scenarios are much harder to exploit for an attacker, and likely have much lower yield making the attacks unlikely to be economically viable. |
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.
The proposed solution doesn't solve the problem for those token contracts, that do implement EIP-2612, as well as for other contracts that rely on the ecrecover
function. A very dirty way to fix this would be to modify the behavior of the ecrecover
function to never return an address where any code is deployed.
Another attack scenario, the proposed solution doesn't prevent, is when an attacker approves token transfer from an address before deploying a contract at this address. However, preparation to such attack could be detected, as the attack requires a contract to be deployed at an address, that already has some outgoing transactions.
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.
Please leave comments in the discussions-to link at the top of the EIP. Messages left here are unlikely to be seen.
Is this (EIP-3607) deployed? |
Please use the |
Do not allow transactions for which `tx.sender` has any code deployed, i.e. `tx.sender` has `CODESIZE != 0`
This EIP fixes the most sever problem with EOA and contract address collisions, by rejecting all transactions that come from accounts with deployed code (i.e. contracts)