l2geth: Add support for system addresses

[mslipper]

Adds support for system addresses.

To deploy to a system address, the deployer must either be in the list of hardcoded addresses described in SystemAddressDeployers, or be specified via the SYSTEM_ADDRESS_0_DEPLOYER/SYSTEM_ADDRESS_1_DEPLOYER environment variables. The hardcoded system addresses deployers will always override those placed in the environment, so specifying the SYSTEM_ADDRESS_* env vars on mainnet, Kovan, or Goerli is a no-op. The env vars are available primarily for testing purposes.

The contract deployment must be the first transaction from the deployment address - i.e., it must have nonce zero.

In order to make the tests work, I had to change the integration tests chain ID to no longer conflict with Goerli. The new integration tests chain ID is 987.

Co-Authored-By: @Inphi

[changeset-bot]

🦋 Changeset detected

Latest commit: 962f36e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 5 packages

Name	Type
@eth-optimism/integration-tests	Patch
@eth-optimism/l2geth	Patch
@eth-optimism/contracts	Patch
@eth-optimism/data-transport-layer	Patch
@eth-optimism/sdk	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[mslipper]

This is a security critical PR. Specifically, the following risks are present:

• It modifies consensus rules.
• There is an escape hatch that reads consensus rules from the environment.
• System addresses are powerful, and can change the behavior of the system in profound ways. Once they are deployed, they should never be updated.

Additionally:

• The address generation rules for the system contracts are copied into two distinct places since the logic to generate the receipt contractAddress field is in multiple places.
• There may be additional consequences to changing contract address generation in this way. I'd like @tynes and @smartcontracts to take a look to make sure I'm not stepping on any landmines.

To try and mitigate these risks, I:

• Added unit and integration testing.
• Made sure that the hardcoded deployer values always override the values present in the environment.

[codecov-commenter]

Codecov Report

Merging #2256 (5ab3118) into develop (1efebf1) will decrease coverage by 0.88%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #2256      +/-   ##
===========================================
- Coverage    80.96%   80.08%   -0.89%     
===========================================
  Files           77       77              
  Lines         2432     2460      +28     
  Branches       434      450      +16     
===========================================
+ Hits          1969     1970       +1     
- Misses         463      490      +27     

Flag	Coverage Δ
contracts	99.29% <ø> (ø)
core-utils	86.77% <ø> (ø)
data-transport-layer	49.72% <ø> (ø)
sdk	55.75% <ø> (-1.97%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
packages/sdk/src/cross-chain-messenger.ts	64.85% <0.00%> (-5.07%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1efebf1...5ab3118. Read the comment docs.

[maurelian]

I reviewed this PR mostly from a meta perspective as we're implementing a new process which varies depending on the security risk.

Comments:

1. I added a new label SR-Risk to identify these.
2. The PR description should also provide the motivation. Presumably the reviewers are aware, but it's useful to make it explicit to ensure there is agreement on it. Of course it's also important for external readers.
3. The risks and mitigations comment is great. (We should standardize on a template, but not necessary here.

An item for further discussion:

I think there's an argument to be made that nominating two reviewers creates a risk of
'diffusion of responsibility', where the first reviewer approaches their review with the awareness that someone else will be coming through to catch anything they might miss.

Review status:
Submitting as Request Changes. I would approve if:

1. The motivation is added to the description.
2. My inline comments are addressed or rejected with explanation.

[maurelian]

Approving as my previous review has been satisfied.

In addition I thought about security considerations a bit more:

Risk considered:

This feels to me like the most consequential change.

I tried to think of a way that an existing contract could be over-written. I couldn't, but @tynes and @smartcontracts should also consider it.

[tynes]

This will be inconsistent with the address that the contract is actually deployed at in the case len(codesize) > 0

[mslipper]

Updated to detect a contract creation transaction at this nonce by adding a check for tx[i].To() == nil. The only possible address for a contract created at nonce 0 from a system address deployer is the system address.

[mslipper]

I took at look at the rest of evm.go - there's a check that will revert if the codesize is > 0:

	// Ensure there's no existing contract already at the designated address
	contractHash := evm.StateDB.GetCodeHash(address)
	if evm.StateDB.GetNonce(address) != 0 || (contractHash != (common.Hash{}) && contractHash != emptyCodeHash) {
		return nil, common.Address{}, 0, ErrContractAddressCollision
	}

So, I removed the additional check in the line above.

[karlfloersch]

Awesome this LGTM! 🔥

Successfully ran the tests locally