ethereum-optimism / contracts Goto Github PK
View Code? Open in Web Editor NEW[Optimism] Smart Contracts
License: MIT License
[Optimism] Smart Contracts
License: MIT License
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.
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
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
OVM_StateCommitmentChain.sol
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
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
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
Our current ring buffer is not correctly implemented & has weird edge cases where garbage collection is not handled correctly.
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.
permit()
to set approvals via signature)permit
slightly to match spec)Describe the bug
The attack is as follows:
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:
contracts/test/helpers/constants.ts
Line 24 in 2c30fad
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
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
!).cd
into the root of this repo.solc --allow-paths . contracts/optimistic-ethereum/**/*.sol
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:
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:
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
mapping(address => address) ovmToEVMBytecodeHash
to the ExecutionManager_bytecode
with ovmToEVMBytecodeHash[keccak256(_bytecode)]
. If the mapping at that key is empty, default to using keccak256(_bytecode)
.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
Additional context
Add any other context about the problem here.
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
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).
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:
withdrawTo
on L2DepositedToken
with an address of a smartcontract coordinating FW let's call it Coordinator
.Coordinator
to send his funds to Alice and record this fact. Alice can enjoy their token on L1.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
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.
OVM_L2CrossDomainMessenger
OVM_L1CrossDomainMessenger
OVM_L2ToL1MessagePasser
OVM_BaseCrossDomainMessenger
OVM_ChainStorageContainer
Also the checks in Synthetix' code.
Starting on L2:
OVM_L2CrossDomainMessenger.sendMessage()
with the information for the L1 message (aka xDomainCalldata
)
_target
, msg.sender
, _message
)messageNonce
storage variable, and the hash is store in the sentMessages
mapping (this is not actually used AFAIK)messageNonce
is then incremented.OVM_L2CrossDomainMessenger
then passes the xDomainCalldata
to OVM_L2ToL1MessagePasser.passMessageToL1()
xDomainCalldata
is hashed with msg.sender
(ie. ovmCaller
), and written to the sentMessages
mapping.Then on L1:
Relayer
(and currently only the Relayer
) may call OVM_L1CrossDomainMessenger.relayMessage()
providing the raw message inputs and an L2 inclusion proof.
relayMessage()
function checks the following things:
_verifyStateRootProof()
:
OVM_ChainStorageContainer
onlyOwner
protected in the OVM_ChainStorageContainer
)_verifyStorageProof()
:
OVM_L2ToL1MessagePasser.sentMessages
mappingsuccessfulMessages
mapping.xDomainMessageSender
state varsuccessfulMessages
and cannot be relayed againrelayedMessages
mappingThen the receiver (in this case SynthetixBridgeToOptimism
):
OVM_L1CrossDomainMessenger
and that the xDomainMessageSender
is the synthetixBridgeToBase
on L2.verifyStateCommitment
, there's a lot of complexity buried in there.
OVM_L1CrossDomainMessenger.xDomainMessageSender
value at the end of the relayMessage
function.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.gasLimit
argument on _sendXDomainMessage()
is unused. We'll probably need that in the future.Passing invalid data:
onlyRelayer
Message replay attacks:
messageNonce
Reentrancy on relayMessage
(calling back in to repeatedly relay the same message, before updating succesfulMessages
mapping)
nonReentrant
keywordGas "griefing" on relayMessage
: (Relayer calls relayMessage
with insufficient gas)
succesfulMessages
mapping if the call succeeds.L1 Receiving contract always fails:
Gas Siphoning:
Is your feature request related to a problem? Please describe.
We're currently using a custom constant NULL_BYTES32
all over the place:
contracts/test/helpers/constants.ts
Lines 20 to 21 in 74f8f79
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:
bytes32
values as an array into exampleFunction()
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
Additional context
Add any other context about the problem here.
A declarative, efficient, and flexible JavaScript library for building user interfaces.
๐ Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
An Open Source Machine Learning Framework for Everyone
The Web framework for perfectionists with deadlines.
A PHP framework for web artisans
Bring data to life with SVG, Canvas and HTML. ๐๐๐
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
Some thing interesting about web. New door for the world.
A server is a program made to process requests and deliver data to clients.
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
Some thing interesting about visualization, use data art
Some thing interesting about game, make everyone happy.
We are working to build community through open source technology. NB: members must have two-factor auth.
Open source projects and samples from Microsoft.
Google โค๏ธ Open Source for everyone.
Alibaba Open Source for everyone
Data-Driven Documents codes.
China tencent open source team.