Skip to content
This repository has been archived by the owner on Apr 12, 2021. It is now read-only.

L1 <> L2 Create2 Address Mismatch #306

Open
gakonst opened this issue Mar 9, 2021 · 5 comments
Open

L1 <> L2 Create2 Address Mismatch #306

gakonst opened this issue Mar 9, 2021 · 5 comments
Labels
Category: Bug Something isn't working discussion

Comments

@gakonst
Copy link
Contributor

gakonst commented Mar 9, 2021

Is your feature request related to a problem? Please describe.

@dmihal mentioned on twitter that due to how OVM bytecode is different from EVM bytecode, Create2 addresses on Optimism would be different from Create2 addresses in Ethereum. This can be problematic for UX e.g. when somebody sends ETH to a user on L2 and accidentally sends it on L1 to an address that does not exist. Normally, you could Create2 to claim that address, but here that's not possible due to this mismatch.

Describe the solution you'd like

  1. Add a mapping(address => address) ovmToEVMBytecodeHash to the ExecutionManager
  2. Allow passing the bytecode hash directly to the create2 address function
  3. In the Create2 code, replace _bytecode with ovmToEVMBytecodeHash[keccak256(_bytecode)]. If the mapping at that key is empty, default to using keccak256(_bytecode).
  4. Add a function for populating that mapping. It can be gated with a trusted admin key. The mapping could also be populated in the constructor of the execution manager.

The overhead from this is pretty small, just an extra lookup in the mapping, and I believe it fully addresses the issue. It'd also be nice if we had a list of "popular" bytecode hashes, to populate most of the needed keys in the mapping in the constructor.

@Agusx1211
Copy link

Why would the mapping need to be populated in the constructor? you mean the constructor of the ExecutionManager?

If there is an admin key that can update the mapping it could lead to a security vulnerability, this admin could break any counter-factual contract that hasn't been deployed yet. Imagine you have a smart contract wallet, the wallet is not deployed but it assumes the default keccak256(_bytecode), the admin updates the mapping and now your wallet can't be deployed.

Another possible solution would be to create a new opcode, this new opcode would need to exist on both L1 and L2, let's call it CREATE3, it would work similarly to CREATE2 but it would compute the address as keccak256( 0x?? ++ address ++ salt )[12:]. This new opcode could be used to deploy a contract on multiple chains, even if the contract bytecode differs across them, but it would require the creator contract to enforce some sort of coherence between the chains.

@gakonst
Copy link
Contributor Author

gakonst commented Mar 9, 2021

Why would the mapping need to be populated in the constructor? you mean the constructor of the ExecutionManager?

Yes the constructor of the EM.

Admin key

We can make this configurable to initially be a trusted key that transitions to something community owned. I agree with the implications if it were to be misused, but I am OK with underwriting that risk given that it cannot result in theft and is at worst a grief.

The new opcode approach could make sense but seems like a lot of effort, comparatively to just having a OVM->EVM bytecodehash map.

@Agusx1211
Copy link

I think something like this would work, but it would be hard develop and also to maintain, requiring community governance for something this primitive is complicated, because that governance can hold much more powers of what initially seems.

If the primary concern is loss of funds, maybe Optimism can add a new transaction type, this transaction type could take the context (msg.sender) from whoever is sending the transaction on the L1 (without any kind of signature), it wouldn't be very efficient for day-to-day transactions, but it would allow almost any smart contract wallet to retrieve funds sent to the L2.

The only downside I see with this approach is that it would break the "only account abstraction" paradigm that Optimism is having, since these kind of transactions would look more like EOAs than AA

@smartcontracts
Copy link
Collaborator

@ben-chain @karlfloersch

@smartcontracts smartcontracts added Category: Bug Something isn't working discussion labels Mar 10, 2021
@ben-chain
Copy link
Collaborator

ben-chain commented Mar 12, 2021

I think that this is a very interesting mechanism and is worth strongly considering. After mulling it over a bit, I think there could even be a further improvement: it seems very notable that the L1 code we are trying to make work with is unsafe code. It feels like it might not violate much existing semantics to add a way to deploy something to the CREATE2 address which would correspond to that bytecode, but is technically impossible to deploy due to the code being unsafe. In the extreme, you could do ovmCREATE3(_bytecode, _unsafeL1Bytecode) would never cause any potential collisions with ovmCREATE2, so long as we require ovmSafetyChecker.isBytecodeSafe(_unsafeL1Bytecode) == false. At the very least, this would be sufficient to recreate all L1 addresses where the factory itself is deployed with an EOA.

Also, a short note on the possible implementation structure:
Due to the nature of how the fraud proofs and execution manager work together; storage is best done at authenticated predeploy addresses; so this would really entail an OVM_CREATE2Mapping contract which the EM talks to. We do a similar thing with the OVM_DeployerWhitelist and a more rudimentary version without a predeploy in the _putGasMetadata logic in the Execution Manager, but this ticket illustrates the need to generalize that in a more useful way. I'll think about the best way to do it.)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Category: Bug Something isn't working discussion
Projects
None yet
Development

No branches or pull requests

4 participants