We need to make ethers a peer dependency at version ^5.0.0 instead of having a hard dependency at specifically version 5.0.0. Causes lots of headaches downstream.

https://github.com/ethereum-optimism/contracts-v2/blob/4571e5d01998fd9da3f7c7c4fcde2d9eef9db40f/package.json#L40

Describe the bug

Hey folks!

Perhaps I'm missing something but I was checking out the OVM_L1ETHGateway.sol contract:
https://github.com/ethereum-optimism/contracts/blob/master/contracts/optimistic-ethereum/OVM/bridge/tokens/OVM_L1ETHGateway.sol

The deposit() function comment states "deposit an amount of the ERC20 to the caller's balance on L2". Is that correct?

Expected behavior
My understanding is this bridge contract is for depositing ETH, not ERC20 tokens.

Additional context
Feel free to close this out if I'm incorrect 😅

push2 is a keyword in Solidity (in assembly). We have various functions named push2 that need to be renamed to something like pushTwo. See here for a complete list: https://github.com/ethereum-optimism/contracts/search?q=push2

To be consistent, we should also rename all instances of get2 to getTwo. This is a pretty easy ticket so would be great to leave this for a new contributor :-)

Is your feature request related to a problem? Please describe.
At present, the optimism contracts rely on msg.sender to authenticate the caller. It would be nice if the sequencer can be authenticated via ecrecover as this will allow any signing key to send the sequencer's transaction on their behalf. This is useful if the sequencer relies on a third party relay service (like ITX) or just to provide the option to send the transaction via a different signing key due to some unforeseen reason (e.g. bug that prevents sequencer key sending the transaction / bumping the fee).

Describe the solution you'd like
I have provided an exploratory PR here #301 to work out the best way to implement it. I have modified:

ReplayProtection.sol

• A contract to standardise how the replay protection is handled. At present, it is a simple incrementing nonce. It has the lowest gas costs since it re-uses the same storage slot.

For OVM_CanonicalTransactionChain.sol and OVM_StateCommitmentChain.sol I have tried to add in two extra parameters, uint nonce and bytes signature, that then checks if the signature is pre-filled.

OVM_CanonicalTransactionChain.sol

• enqueue().
• appendQueueBatch().
• appendSequencerBatch() - I couldn't get it to work yet with this function. Some parameters are read-in via assembly.

OVM_StateCommitmentChain.sol

• appendStateBatch().
• deleteStateBatch().

Describe alternatives you've considered
I am not sure if there is any alternative. Wallet contract's won't work due to the .call() gas expansion. I do wonder if the above functions really need to authenticate the caller? Mostly around the transaction chain, since that will prevent honest users from sending withdrawal transactions?

Also, is there any other contracts that requires the sequencer interaction? I couldn't find any.

Additional context
I am mostly trying to work out how we could separate msg.sender and ecrecover in the contracts. To make it easier to authenticate the sequencer.

Would prevent problems in the future because the TS just won't compile

TODO

• Rename / update transaction types
• Rename native transaction
• Fix Ring Buffer
• Integrate optimized merkle tree
• Test multi-batch submission
• Implement efficient multi-batch submission encoding [Karl - in prog]

Descriptions

Rename / update transaction types

Transaction type includes number -- is this block number? If it is tx index then we should call it txIndex but if it's block number we should either call it blockNumber, or (my preference) call timestamp and blockNumber lastL1Timestamp and lastL1BlockNumber to avoid confusion -- https://github.com/ethereum-optimism/contracts-v2/blob/feat/polish/contracts/optimistic-ethereum/libraries/codec/Lib_OVMCodec.sol#L73-L81

Rename native transaction

Replace "native" transaction here https://github.com/ethereum-optimism/contracts-v2/blob/feat/polish/contracts/optimistic-ethereum/libraries/codec/Lib_OVMCodec.sol#L31 with EIP155 transaction

Fix Ring Buffer

Our current ring buffer is not correctly implemented & has weird edge cases where garbage collection is not handled correctly.

Integrate optimized merkle tree

We've got to integrate the optimized merkle tree -- https://github.com/ethereum-optimism/contracts-v2/blob/feat/polish/contracts/optimistic-ethereum/libraries/utils/Lib_MerkleUtils.sol#L9

Is your feature request related to a problem? Please describe.
As it stands all of our transactions are sent without waiting for receipts & without hardcoding the nonce. This means we often fail during "real" deployments (to chains other than ganache).

Describe the solution you'd like
Add a function sendTransaction(tx) which handles incrementing the nonce of each tx. This will allow us to send all of our transactions upfront & just wait until they all get mined vs waiting around for receipts.

Describe alternatives you've considered
Wait for receipts

Optimistic Ethereum's L2 does not have a native token like Ethereum's Ether. Rather we have an WETH-like OMV_ETH contract pre-deployed in the genesis state:

We need to finalize on the implementation we'll build on prior to our public mainnet launch.

Any ETH deposited to L2 will be held in this contract.

Wanted:

• Secure and battle tested
• Supports EIP-2612 (ie. using permit() to set approvals via signature)
• Permissive licensing

Questions:

• Should we use WETH10 (with effectively unlimited flashminting?)

Candidate Implementations:

• UniswapV2ERC20.sol
• Dai (requires modifying permit slightly to match spec)

Describe the bug
The attack is as follows:

1. Attacker commits fraud at time T and T+10.
2. Attacker proves fraud at time T+10, getting 1/2 of their bond back.
3. Verifier now needs to prove fraud at time T to protect their funds, but they do not get a reward for doing so.

Steps to reproduce
http://www.gambit-project.org/
lulz jk

Expected behavior
The bond should be paid out to the people who contributed to the earliest fraud proof.

Solution
Once fraud is proven at T+10, start withdrawal period where earliestFraud=T+10 . Then once fraud is proven a second time, earliestFraud = min(currentEarliestFraud, T) . Of course The new fraud proof at time T is now the earliestFraud.
At the end of the week, if you contributed to the earliestFraud fraud proof, you get the payout!

Shout out @kfichter for figuring this out.

Note: This issue is labeled as a "Good First Issue". If you've previously contributed to this project, please consider leaving this issue open for someone new. Thank you!! :-)

We're using a constant named ZERO_ADDRESS all over the place. This constant is defined as:

Ethers also has this constant at ethers.constants.ZeroAddress. We want to replace all instances of ZERO_ADDRESS with ethers.constants.ZeroAddress. You can see an example of this change in this PR: https://github.com/ethereum-optimism/contracts/pull/327/files. Feel free to leave a comment on this PR if you have any questions!

Note: We have another constant named NON_ZERO_ADDRESS that can be left untouched.

Describe the bug
Right now all deployed contracts in our deployment object are prefixed with Proxy__. This is old behavior when we were auto-generating proxies for all contracts, but now it's really unneeded.

Steps to reproduce
Deploy some contracts and look at the output object.

Expected behavior
Just provide the contract name with no Proxy__, or re-add a proxy system.

It would be nice to have a mapping of the precompile addresses to the contract objects exported so that its easy to know which precompiles are what

I ran deploy, and while I see the contracts going on to the chain, I don't see anything in the console, been hanging for about 10 minutes now.

The deployer now depends on ethers hardware wallet which in turn depends on node-hid which depends on having libusb-1.0 installed in the containers. It would be better to remove the deployer script so that every container that installs the contracts doesn't need to also install the usb library. Note that containers without libusb-1.0 can built, but it throws a nasty warning.

The Address Manager contract contains a registry of the contracts in the system, which are retrieved using the Address Resolver's resolve() method.

We just need to change the string per the title of this issue.

Compiling our contracts currently returns a large number of compiler warnings.

Fixing them should be a fairly simple task, but will require some time to go through everything.

Recommended approach:

IMO the solc binary give much easier to read colored output compared to hardhat, so install solc

1. Install solc verson 0.7.6 following these instructions (any of the methods described on that page other than npm i -g solc because solcjs != solc!).
2. cd into the root of this repo.
3. Run the compiler on the full system using: solc --allow-paths . contracts/optimistic-ethereum/**/*.sol
4. Work through the warnings systematically. You might choose to focus on one file at a time, or one type of warning at a time, whichever works for you.

I would suggest submitting a draft PR for early review after clearing up the first dozen or so warnings, just to be sure we're on track.

Describe the bug
We don't use this and it's being deployed into L2 state now

To Reproduce
Steps to reproduce the behavior:

1. Go to '...'
2. Click on '....'
3. Scroll down to '....'
4. See error

Expected behavior
A clear and concise description of what you expected to happen.

Screenshots
If applicable, add screenshots to help explain your problem.

System Specs:

• OS:
• Package Version (or commit hash):

Additional context
Add any other context about the problem here.

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.

Describe the bug

It results in a Error: Cannot find module 'ganache-core'

Expected behavior
It should just work when installing from npm

Screenshots
If applicable, add screenshots to help explain your problem.

Specifications

• Version:
• OS:
• Node Version:

Additional context
Add any other context about the problem here.

Describe the bug

This repo is on version 0.1.15, but on npm.org the latest version is shown as 0.0.0-alpha.1.

The same info is returned by yarn info @eth-optimism/contracts and npm info @eth-optimism/contracts.

Expected behavior

The package number should be 0.1.15.

Is your feature request related to a problem? Please describe.
It is really annoying saying "Canonical Tx Chain" or "State Commitment Chain" -- shorter is better.

Describe the solution you'd like
Rename Canonical Tx Chain -> Tx Chain & State Commitment Chain -> State Chain

It's currently non-obvious where the chainID gets populated / decoded. We should make sure that that tx flow is as simple as possible / at minimum add better comments for how it works.

Decimals is using an immutable variable, which isn't getting set correctly in the constructor. You can fix this by removing immutable, and I can confirm decimals works, but that moves the storage slots which breaks my existing chain.

This comment indicates that finalizeWithdrawal is enforcing the 7 day window itself, where as the only thing enforcing the window is the onlyFromCrossDomainAccount() modifier.

Updating this comment would clear up some confusion for new readers of the codebase.

Describe the bug
A clear and concise description of the bug.

Steps to reproduce
Steps to reproduce the behavior:
1.
2.
3.

Expected behavior
A clear and concise description of what you expected to happen.

Screenshots
If applicable, add screenshots to help explain your problem.

Specifications

• Version:
• OS:
• Node Version:

Additional context
Add any other context about the problem here.

Problem Description
As a developer of a 3rd party (external to a birdge) fast withdrawal solution I would like to have a way to connect an original withdrawal requester from an l2 to l1 message payload. Additionally, I would like to embed even more details about the withdrawal request (fees etc).

Solution

Add extraData field to iOVM_L1TokenGateway's finalizeWithdrawal. This adds more context to cross-chain messages and allows to build 3rd party fast withdrawal solutions OUTSIDE of the token bridge.

extraData can contain a hash of a message containing the address of an original requester, info about fees etc.

Additional context
FW solution flow possible after this change:

1. Alice calls withdrawTo on L2DepositedToken with an address of a smartcontract coordinating FW let's call it Coordinator.
2. A liqudity provider Bob validates that withdrawal is correct.
3. Bob sends Alice funds on L1 by letting Coordinator to send his funds to Alice and record this fact. Alice can enjoy their token on L1.
4. When Alice's withdrawal settles, Bob claims it as he can proof that he already sent Alice funds.

If (3) never happened, after withdrawal settles, Alice proofs that she initiated the original withdraw and she claims tokens. Without the change described in this PR a cross-chain message only contains address of a Coordinator SC (to field) and hence Alice can not prove that she originated the withdrawal.

PoC of such solution was implemented by @gakonst in: https://github.com/gakonst/optimistic-fast-withdrawals/blob/feat/fast-withdrawals/contracts/MarketMaker.sol

Alice and Bob can negotiate additional fees in a state channel and they don't have to be part of the cross-chain message.

Original discussion: #349

Is your feature request related to a problem? Please describe.
I noticed in https://github.com/ethereum-optimism/batch-submitter they need access to OVM_CanonicalTransactionChain.sol and OVM_StateCommitmentChain.sol (example: https://github.com/ethereum-optimism/batch-submitter/blob/master/src/transaciton-chain-contract.ts#L40). It looks like they have had to do some custom work to make the functions available for the contracts.

Describe the solution you'd like
if the typechain of the contracts is exported from this repo, then you could easily import it to other optimism projects. So it is easy just to use the ContractFactory(). Useful for encoding function data.

Describe alternatives you've considered
Seems like a nobrainer. Maybe it isn't best practice? I dunno. I am a humble typechain user.

Additional context
Add any other context or screenshots about the feature request here.

Is your feature request related to a problem? Please describe.
With the current design of the ERC20 Gateway, the user have to first approve the gateway to allow it to transfer the token on its behalf.

Describe the solution you'd like
If the gateway would support ERC677 function onTokenTransfer(address, uint, bytes calldata) external returns (bool); then users could simply call transferAndCall and be done

ERC677 is a defacto standard being used by WETH10

Describe alternatives you've considered
An alternative that would also be great to have is for the deposit function to accept a signature so it can call EIP-2612 permit if needed

Executive summary

This internal report summarizes my review of the code involved in withdrawals from L2 to L1, performed during the week of January 11. It is being posted here for posterity and transparency.

Scope

Key components

• OVM_L2CrossDomainMessenger
• OVM_L1CrossDomainMessenger
• OVM_L2ToL1MessagePasser
• OVM_BaseCrossDomainMessenger
• OVM_ChainStorageContainer
• and more as you follow the call trace

Also the checks in Synthetix' code.

Description of the withdrawal flow

Starting on L2:

• Any account on L2 may call OVM_L2CrossDomainMessenger.sendMessage() with the information for the L1 message (aka xDomainCalldata)
  • (ie. _target, msg.sender, _message)
  • This data is hashed with the messageNonce storage variable, and the hash is store in the sentMessages mapping (this is not actually used AFAIK)
  • The messageNonce is then incremented.
• The OVM_L2CrossDomainMessenger then passes the xDomainCalldata to OVM_L2ToL1MessagePasser.passMessageToL1()
  • the xDomainCalldata is hashed with msg.sender (ie. ovmCaller), and written to the sentMessages mapping.

Then on L1:

• The Relayer (and currently only the Relayer) may call OVM_L1CrossDomainMessenger.relayMessage() providing the raw message inputs and an L2 inclusion proof.
  • The relayMessage() function checks the following things:
    • in _verifyStateRootProof():
      • checks that the fraud proof window has closed for the batch to which the transaction belongs
      • checks that the batch is stored in the OVM_ChainStorageContainer
        • (all state changing functions are onlyOwner protected in the OVM_ChainStorageContainer)
    • in _verifyStorageProof():
      • checks the proof to confirm that the message data provided is in the OVM_L2ToL1MessagePasser.sentMessages mapping
    • checks that this transaction has not already been written to the successfulMessages mapping.
  • The address of the L2 call is then written to the xDomainMessageSender state var
  • the call is then executed
  • if it succeeds it is added to the successfulMessages and cannot be relayed again
  • regardless of success, an entry is written to the relayedMessages mapping

Then the receiver (in this case SynthetixBridgeToOptimism):

• Checks that the caller is the OVM_L1CrossDomainMessenger and that the xDomainMessageSender is the synthetixBridgeToBase on L2.

Comments and impressions

• The SNX checks look fine.
• The fraud proof window check feels good to me.
• I'd like to look more closely at verifyStateCommitment, there's a lot of complexity buried in there.
  • Especially given the nested structs, I'm worried about the possibility of passing in some piece data that might go unverified.
• It might be a good idea to zero out the OVM_L1CrossDomainMessenger.xDomainMessageSender value at the end of the relayMessage function.
• The relayedMessages mapping is unnecessary, since we're using the onlyRelayer modifier. Also should we even write to it if the call fails? Also, why not just emit an event instead?
• relayMessage is going to be an expensive and commonly called function, there are a lot of opportunities for gas optimization here.
• I did not look closely at the EM, which is of course an important factor in all of this, but would be an explosion in scope.
• I see that the gasLimit argument on _sendXDomainMessage() is unused. We'll probably need that in the future.
• What, if anything, is ensuring that a message is eventually relayed (liveness)? I think nothing.

Discussion of possible attacks and their mitigations

• Passing invalid data:

  • Mitigated by onlyRelayer
  • More work needed to review data validation, but at the upper layers of checks it looks OK.

• Message replay attacks:

  • Mitigated by hashing with a messageNonce

• Reentrancy on relayMessage (calling back in to repeatedly relay the same message, before updating succesfulMessages mapping)

  • Mitigated with the nonReentrant keyword

• Gas "griefing" on relayMessage: (Relayer calls relayMessage with insufficient gas)

  • Not an issue because we only update the succesfulMessages mapping if the call succeeds.

• L1 Receiving contract always fails:

  • What if an attacker has us relay messages that constantly fail
    • Do we keep trying to send them? How do we decide whether or not a message should be re-relayed
    • Future mitigation: L2>L1 messages can have a minimum gas amount. If it fails with that much gas, well to fuckin' bad.

• Gas Siphoning:

  • Currently protected against just by access control.
    • Not-mitigated otherwise. Without being careful about gas limits, users could trick the relayer into sending messages to mint gas token.

Is your feature request related to a problem? Please describe.
We're currently using a custom constant NULL_BYTES32 all over the place:

It'd be much better if we used ethers.constants.HashZero so that we can outsource these constants to one place. Same idea as #329.

Describe the solution you'd like
Replace all instances of NULL_BYTES32 with ethers.constants.HashZero. Please be careful not to replace NON_NULL_BYTES32 by accident!

Describe the bug
The variable passed into the _elements function param of Lib_MerkleTree.getMerkleRoot() is unintentionally overwritten when _elements.length > 1 and when the variable being passed in is stored as memory.

If the _elements array has a length of n where n > 1, all elements up to the final one are overwritten with an unexpected hash when execution continues after the completion of getMerkleRoot().

To Reproduce
Steps to reproduce the behavior:

1. Paste this gist into remix
2. Deploy ExampleContract
3. Pass in 2+ bytes32 values as an array into exampleFunction()
4. Observe different values for idBefore and idAfter

Expected behavior
I would expect idBefore and idAfter to be the same value.

Additional context
I do not believe Optimism's current contracts are affected by this issue. However, future Optimism contracts may be. Additionally, developers and projects who fork this code or use the Lib_MerkleTree library may have issues.

Since the contracts are getting close to being frozen, it would be useful if the artifacts directory is committed so that a consumer of this gitrepo doesn't need yarn and have to install a bunch of dependencies to get the compiled contracts/abi. This is useful for the script I'm writing to autogenerate bindings to the contracts, I'd like to download the artifact JSON files directly from github and then use the abi and bytecode directly with abigen.

What do you think?

Is your feature request related to a problem? Please describe.
xDomainMessageSender will continue to return the sender of the last message until the next message is relayed.

This shouldn't be an issue for the most part since relayMessage has a reentrancy guard and every contract consuming xDomainMessageSender should also be checking that msg.sender is the Optimism messenger. That being said, it could lead to some weird behavior and is easy to fix.

Describe the solution you'd like
Consider either zeroing out xDomainMessageSender after the calls here and here or setting is to an address like 0x000000000000000000000000000000000000dead. Either one should result in some gas savings since the transaction will always reset the storage slot to the same value that it was at the beginning of the transaction. Each option will have slightly different gas implications (I'd recommend setting to the dead address).

Basically, it should improve security and save a bit of gas.

Describe alternatives you've considered
Leave as is

Describe the bug
There is no check inside of initializeFraudVerification that the (preStateRoot index + 1 == transaction index). Anyone could submit a fraud proof with a pre state root index that is not actually the pre state for the given transaction

Steps to reproduce
N/A

Expected behavior

Screenshots
If applicable, add screenshots to help explain your problem.

Specifications

• Version:
• OS:
• Node Version:

Additional context
Add any other context about the problem here.