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-1712: Disallow Deployment of Unused Opcodes #1712

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@sorpaas
Copy link
Contributor

commented Jan 19, 2019

Simple Summary

Hard fork can change existing contract behavior. One of the point can
be raised about adding new opcodes is that it modifies existing unused
opcodes from throwing out of gas to another behavior. While we can
mostly argue that deploying unused opcode is not of much use so no
sane developers would do that, it would be better to just disallow
deployment of unused opcodes.

sorpaas added some commits Jan 19, 2019

@sorpaas sorpaas changed the title EIP-xxx: Disallow Deployment of Unused Opcodes EIP-1712: Disallow Deployment of Unused Opcodes Jan 19, 2019

@jochem-brouwer

This comment has been minimized.

Copy link

commented Jan 28, 2019

I get the view where this EIP is made from but I am strongly against this. The reason is that, for example, solidity appends a swarm hash to the end of the bytecode - which now hence has to be removed. Moreover, you can also use contracts for some kind of storage trick currently: if you have a variable which is read-only you can store this in a contract and then EXTCODECOPY this variable. On top of that, if you define a variable constant in solidity then this data is actually encoded in the bytecode - so if you have a constant owner (of type address) there is a very high chance that this has an "opcode" which is invalid - even though it's in a PUSHx opcode. These considerations make it really hard to figure out gas costs for this EIP, plus it reduces the usage of contracts simply for read-only storage + storing metadata like swarm hashes.

@sorpaas

This comment has been minimized.

Copy link
Contributor Author

commented Jan 28, 2019

@jochem-brouwer Doing things like you said not only open up huge security issues and broke the original intention of JUMPDEST and data/code separation invariant. But also, all use cases you said can be easily fulfilled by simply prefix a PUSH32 in the front.

@jochem-brouwer

This comment has been minimized.

Copy link

commented Jan 28, 2019

@sorpaas Can you provide more in-depth info on your statements? What security issue? What was the original intention of JUMPDEST? (The yellow paper writes that if you have a JUMPDEST opcode which is actually in a PUSHx opcode it is not a valid JUMPDEST).

For contracts storing read only data: you can simply put a STOP opcode right at the start of the code - no code will hence ever run.

@sorpaas

This comment has been minimized.

Copy link
Contributor Author

commented Jan 28, 2019

@jochem-brouwer If we push the swarm hash without a PUSH32, then it is possible that if there's a bug in the code (either in the compiler, or if someone wrote assembly), then a JUMP opcode can accidentally jump into the swarm hash and do something unexpected. The original intention of JUMPDEST is to make this case impossible.

For contracts storing read only data: you can simply put a STOP opcode right at the start of the code - no code will hence ever run.

With all those little "tricks" deployed to the mainnet, we make it harder to upgrade. What if we want to enable eWASM and that data you want to store (also starts with \0) happens to be interpreted as WebAssembly format?

I'm not saying this EIP will solve all of the cases, but I want to point out that your trick breaks the data/code invariant, and we have ways to accomplish the exact same thing, without breaking any invariant.

@jochem-brouwer

This comment has been minimized.

Copy link

commented Jan 28, 2019

What if I want to pre-deploy a contract for Constantinople which uses CREATE2?

@markusj1201

This comment has been minimized.

Copy link

commented Jan 28, 2019

@sorpaas sorpaas force-pushed the sorpaas:sp-disallow branch from c07826f to 8dd93f9 Mar 27, 2019

@Arachnid

This comment has been minimized.

Copy link
Collaborator

commented Mar 27, 2019

There's no assumption at present that all the contents of a contract's data is bytecode, or is reachable. Because JUMPs are dynamic, there's no way to do static reachability analysis in the general case, either.

This EIP would break existing functionality, require a hard fork, and I don't see what measurable benefit it provides.

@sorpaas

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2019

@Arachnid Do you have sources for the claim that "there's no assumption at present that all the contents of a contract's data is bytecode, or is reachable"? I specifically think we might have it when EVM was designed, because we do JUMPDEST static analysis of the whole code every time when we execute EVM.

But it's true that Solidity doesn't follow this, and we'd probably need "exception" rules to make it work.

@Arachnid

This comment has been minimized.

Copy link
Collaborator

commented Mar 27, 2019

@sorpaas Do you have sources for the claim that "there's no assumption at present that all the contents of a contract's data is bytecode, or is reachable"?

I'm not sure how I could prove a negative here.

I specifically think we might have it when EVM was designed, because we do JUMPDEST static analysis of the whole code every time when we execute EVM.

I'm not aware of anything doing reachability analysis in a client. Clients index the locations of JUMPDEST opcodes so they can determine if a JUMP's address is valid, but that's different from determining which sections of code are reachable.

@sorpaas

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2019

@Arachnid JUMPDEST treats all of a contract's bytecode as code, regardless of whether it is reachable. This indicates that we did want to enforce some code/data separation in EVM design.

IMO the use of contract's bytecode as data is still troublesome. You can have a data that accidentally equals to JUMPDEST opcode. And a compiler flaw, or unsafe assembly code, could make the program JUMP to data, and do unexpected and damaging things. Worse, it can be that a type of bug like this is hidden because a current opcode's meaning is invalid/out-of-gas, and the contract only exhibit the bug in a future hard fork. If you followed the immutability/invariant discussions, this is where this EIP came from.

It's could be the case that except Solidity's metadata postfix, no one actually stores data in contract's bytecode because as far as I know, the compiler does not really have features (other than assembly) supporting this (happy if someone can prove me wrong!). Because of this, the above bug scenario is an edge case whose chance of happening is significantly low. But that, to me, is a reason we may want to just fix it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.