Skip to content
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

Merged
merged 10 commits into from
Jun 11, 2021

Conversation

dankrad
Copy link
Contributor

@dankrad dankrad commented Jun 10, 2021

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)

@alita-moore
Copy link
Contributor

alita-moore commented Jun 10, 2021

Hi! I'm a bot, and I wanted to automerge your PR, but couldn't because of the following issue(s):

 - File with name EIPS/eip-3607.md is new and new files must be reviewed
 - This PR requires review from one of [@MicahZoltu, @lightclient]

@MicahZoltu @lightclient

@dankrad
Copy link
Contributor Author

dankrad commented Jun 10, 2021

Discussion to: #3608

Copy link
Member

@lightclient lightclient left a 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.

dankrad and others added 4 commits June 10, 2021 23:23
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>
Copy link
Member

@lightclient lightclient left a 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

EIPS/eip-3607.md Outdated Show resolved Hide resolved
EIPS/eip-3607.md Outdated Show resolved Hide resolved
dankrad and others added 2 commits June 11, 2021 09:28
Co-authored-by: lightclient <14004106+lightclient@users.noreply.github.com>
Co-authored-by: lightclient <14004106+lightclient@users.noreply.github.com>
Comment on lines +77 to +82
```go
// Make sure the sender is an EOA
if codesize := st.state.GetCodeSize(st.msg.From()); codesize != 0 {
return ErrSenderNoEOA
}
```
Copy link
Contributor

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.

Copy link
Contributor Author

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)
Copy link
Contributor

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).

Copy link
Contributor Author

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?

Copy link
Member

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

Copy link
Contributor

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.

@MicahZoltu MicahZoltu merged commit 1e6d180 into ethereum:master Jun 11, 2021
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.
Copy link

@3sGgpQ8H 3sGgpQ8H Jun 15, 2021

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.

Copy link
Contributor

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.

@eulerbeat5
Copy link

Is this (EIP-3607) deployed?

@MicahZoltu
Copy link
Contributor

Please use the discussions-to link for discussions/questions about an EIP. You can find the link at the top of the EIP: https://eips.ethereum.org/EIPS/eip-3607

PhABC pushed a commit to PhABC/EIPs that referenced this pull request Jan 25, 2022
Do not allow transactions for which `tx.sender` has any code deployed, i.e. `tx.sender` has `CODESIZE != 0`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants