From 0271fa1ba17cfb8d81cefb08c1266facf52b5b5e Mon Sep 17 00:00:00 2001 From: Kelvin Fichter Date: Wed, 2 Jun 2021 17:39:21 -0400 Subject: [PATCH 01/11] feat[contracts]: add L1ChugSplashProxy --- .../chugsplash/L1ChugSplashProxy.sol | 252 ++++++++++++++++++ 1 file changed, 252 insertions(+) create mode 100644 packages/contracts/contracts/chugsplash/L1ChugSplashProxy.sol diff --git a/packages/contracts/contracts/chugsplash/L1ChugSplashProxy.sol b/packages/contracts/contracts/chugsplash/L1ChugSplashProxy.sol new file mode 100644 index 000000000000..ad763d16ff43 --- /dev/null +++ b/packages/contracts/contracts/chugsplash/L1ChugSplashProxy.sol @@ -0,0 +1,252 @@ +// SPDX-License-Identifier: MIT +pragma solidity >0.5.0 <0.8.0; + +/** + * @title L1ChugSplashProxy + * @dev Basic ChugSplash proxy contract for L1. Very close to being a normal proxy but has added + * functions `setCode` and `setStorage` for changing the code or storage of the contract. Nifty! + */ +contract L1ChugSplashProxy { + + /************* + * Constants * + *************/ + + // "Magic" prefix. When prepended to some arbitrary bytecode and used to create a contract, the + // appended bytecode will be deployed as given. + bytes13 constant internal DEPLOY_CODE_PREFIX = 0x600D380380600D6000396000f3; + + // bytes32(uint256(keccak256('eip1967.proxy.implementation')) - 1) + bytes32 constant internal IMPLEMENTATION_KEY = 0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc; + + // bytes32(uint256(keccak256('eip1967.proxy.admin')) - 1) + bytes32 constant internal OWNER_KEY = 0xb53127684a568b3173ae13b9f8a6016e243e63b6e8ee1178d6a717850b5d6103; + + + /*************** + * Constructor * + ***************/ + + /** + * @param _owner Address of the initial contract owner. + */ + constructor( + address _owner + ) { + _setOwner(_owner); + } + + + /********************** + * Function Modifiers * + **********************/ + + /** + * Makes a proxy call instead of triggering the given function when the caller is either the + * owner or the zero address. Caller can only ever be the zero address if this function is + * being called off-chain via eth_call, which is totally fine and can be convenient for + * client-side tooling. Avoids situations where the proxy and implementation share a sighash + * and the proxy function ends up being called instead of the implementation one. + */ + modifier proxyCallIfNotOwner() { + if (msg.sender == _getOwner() || msg.sender == address(0)) { + _; + } else { + // This WILL halt the call frame on completion. + _doProxyCall(); + } + } + + + /********************* + * Fallback Function * + *********************/ + + fallback() + external + { + // Proxy call by default. + _doProxyCall(); + } + + + /******************** + * Public Functions * + ********************/ + + /** + * Sets the code that should be running behind this proxy. Note that this scheme is a bit + * different from the standard proxy scheme where one would typically deploy the code + * separately and then set the implementation address. We're doing it this way because it gives + * us a lot more freedom on the client side. Can only be triggered by the contract owner. + * @param _code New contract code to run inside this contract. + */ + function setCode( + bytes memory _code + ) + proxyCallIfNotOwner + public + { + // Get the code hash of the current implementation. + address implementation = _getImplementation(); + bytes32 currentCodeHash; + assembly { + currentCodeHash := extcodehash(implementation) + } + + // If the code hash matches the new implementation then we return early. + if (keccak256(_code) == currentCodeHash) { + return; + } + + // Create the deploycode by appending the magic prefix. + bytes memory deploycode = abi.encodePacked( + DEPLOY_CODE_PREFIX, + _code + ); + + // Deploy the code and set the new implementation address. + address newImplementation; + assembly { + newImplementation := create(0x0, add(deploycode, 0x20), mload(deploycode)) + } + _setImplementation(newImplementation); + } + + /** + * Modifies some storage slot within the proxy contract. Gives us a lot of power to perform + * upgrades in a more transparent way. Only callable by the owner. + * @param _key Storage key to modify. + * @param _value New value for the storage key. + */ + function setStorage( + bytes32 _key, + bytes32 _value + ) + proxyCallIfNotOwner + public + { + assembly { + sstore(_key, _value) + } + } + + /** + * Changes the owner of the proxy contract. Only callable by the owner. + * @param _owner New owner of the proxy contract. + */ + function setOwner( + address _owner + ) + proxyCallIfNotOwner + public + { + _setOwner(_owner); + } + + /** + * Queries the owner of the proxy contract. Can only be called by the owner OR by making an + * eth_call and setting the "from" address to address(0). + * @return Owner address. + */ + function getOwner() + proxyCallIfNotOwner + public + returns ( + address + ) + { + return _getOwner(); + } + + + /********************** + * Internal Functions * + **********************/ + + /** + * Sets the implementation address. + * @param _implementation New implementation address. + */ + function _setImplementation( + address _implementation + ) + internal + { + assembly { + sstore(IMPLEMENTATION_KEY, _implementation) + } + } + + /** + * Queries the implementation address. + * @return Implementation address. + */ + function _getImplementation() + internal + view + returns ( + address + ) + { + address implementation; + assembly { + implementation := sload(IMPLEMENTATION_KEY) + } + return implementation; + } + + /** + * Changes the owner of the proxy contract. + * @param _owner New owner of the proxy contract. + */ + function _setOwner( + address _owner + ) + internal + { + assembly { + sstore(OWNER_KEY, _owner) + } + } + + /** + * Queries the owner of the proxy contract. + * @return Owner address. + */ + function _getOwner() + internal + view + returns ( + address + ) + { + address owner; + assembly { + owner := sload(OWNER_KEY) + } + return owner; + } + + /** + * Performs the proxy call via a delegatecall. + */ + function _doProxyCall() + internal + { + address implementation = _getImplementation(); + + assembly { + calldatacopy(0x0, 0x0, calldatasize()) + let result := delegatecall(gas(), implementation, 0x0, calldatasize(), 0x0, 0x0) + returndatacopy(0x0, 0x0, returndatasize()) + switch result + case 0x0 { + revert(0x0, returndatasize()) + } + default { + return (0x0, returndatasize()) + } + } + } +} From f373d8c2a93e328bddfc66079fec9cd283fc93a0 Mon Sep 17 00:00:00 2001 From: Kelvin Fichter Date: Wed, 2 Jun 2021 17:47:06 -0400 Subject: [PATCH 02/11] improve comments slightly --- .../chugsplash/L1ChugSplashProxy.sol | 26 ++++++++++++++----- 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/packages/contracts/contracts/chugsplash/L1ChugSplashProxy.sol b/packages/contracts/contracts/chugsplash/L1ChugSplashProxy.sol index ad763d16ff43..52d1731f1092 100644 --- a/packages/contracts/contracts/chugsplash/L1ChugSplashProxy.sol +++ b/packages/contracts/contracts/chugsplash/L1ChugSplashProxy.sol @@ -5,6 +5,12 @@ pragma solidity >0.5.0 <0.8.0; * @title L1ChugSplashProxy * @dev Basic ChugSplash proxy contract for L1. Very close to being a normal proxy but has added * functions `setCode` and `setStorage` for changing the code or storage of the contract. Nifty! + * + * Note for future developers: do NOT make anything in this contract 'public' unless you know what + * you're doing. Anything public can potentially have a function signature that conflicts with a + * signature attached to the implementation contract. Public functions SHOULD always have the + * 'proxyCallIfNotOwner' modifier unless there's some *really* good reason not to have that + * modifier. And there almost certainly is not a good reason to not have that modifier. Beware! */ contract L1ChugSplashProxy { @@ -237,16 +243,24 @@ contract L1ChugSplashProxy { address implementation = _getImplementation(); assembly { + // Copy calldata into memory at 0x0....calldatasize. calldatacopy(0x0, 0x0, calldatasize()) - let result := delegatecall(gas(), implementation, 0x0, calldatasize(), 0x0, 0x0) + + // Perform the delegatecall, make sure to pass all available gas. + let success := delegatecall(gas(), implementation, 0x0, calldatasize(), 0x0, 0x0) + + // Copy returndata into memory at 0x0....returndatasize. Note that this *will* + // overwrite the calldata that we just copied into memory but that doesn't really + // matter because we'll be returning in a second anyway. returndatacopy(0x0, 0x0, returndatasize()) - switch result - case 0x0 { + + // Success == 0 means a revert. We'll revert too and pass the data up. + if iszero(success) { revert(0x0, returndatasize()) } - default { - return (0x0, returndatasize()) - } + + // Otherwise we'll just return and pass the data up. + return(0x0, returndatasize()) } } } From e42dc1c4e39b594642cbec2e27d012830cbe2249 Mon Sep 17 00:00:00 2001 From: Kelvin Fichter Date: Wed, 2 Jun 2021 20:59:57 -0400 Subject: [PATCH 03/11] start adding tests --- .../chugsplash/L1ChugSplashProxy.sol | 5 ++ .../chugsplash/L1ChugSplashProxy.spec.ts | 65 +++++++++++++++++++ 2 files changed, 70 insertions(+) create mode 100644 packages/contracts/test/contracts/chugsplash/L1ChugSplashProxy.spec.ts diff --git a/packages/contracts/contracts/chugsplash/L1ChugSplashProxy.sol b/packages/contracts/contracts/chugsplash/L1ChugSplashProxy.sol index 52d1731f1092..8a06b9dafc58 100644 --- a/packages/contracts/contracts/chugsplash/L1ChugSplashProxy.sol +++ b/packages/contracts/contracts/chugsplash/L1ChugSplashProxy.sol @@ -242,6 +242,11 @@ contract L1ChugSplashProxy { { address implementation = _getImplementation(); + require( + implementation != address(0), + "L1ChugSplashProxy: implementation is not set yet" + ); + assembly { // Copy calldata into memory at 0x0....calldatasize. calldatacopy(0x0, 0x0, calldatasize()) diff --git a/packages/contracts/test/contracts/chugsplash/L1ChugSplashProxy.spec.ts b/packages/contracts/test/contracts/chugsplash/L1ChugSplashProxy.spec.ts new file mode 100644 index 000000000000..adadf683fbe3 --- /dev/null +++ b/packages/contracts/test/contracts/chugsplash/L1ChugSplashProxy.spec.ts @@ -0,0 +1,65 @@ +import { expect } from '../../setup' + +/* Imports: External */ +import hre, { ethers } from 'hardhat' +import { Contract, Signer } from 'ethers' + +describe('L1ChugSplashProxy', () => { + let signer1: Signer + let signer2: Signer + before(async () => { + ;[signer1, signer2] = await hre.ethers.getSigners() + }) + + let L1ChugSplashProxy: Contract + beforeEach(async () => { + const Factory__L1ChugSplashProxy = await hre.ethers.getContractFactory( + 'L1ChugSplashProxy' + ) + L1ChugSplashProxy = await Factory__L1ChugSplashProxy.deploy( + await signer1.getAddress() + ) + }) + + describe('getOwner', () => { + it('should return the owner if called by the owner', async () => { + expect( + await L1ChugSplashProxy.connect(signer1).callStatic.getOwner() + ).to.equal(await signer1.getAddress()) + }) + + it('should return the owner if called by the zero address in an eth_call', async () => { + expect( + await L1ChugSplashProxy.connect(signer1.provider).callStatic.getOwner({ + from: hre.ethers.constants.AddressZero, + }) + ).to.equal(await signer1.getAddress()) + }) + + it('should otherwise pass through to the proxied contract', async () => { + await expect( + L1ChugSplashProxy.connect(signer2).callStatic.getOwner() + ).to.be.revertedWith('L1ChugSplashProxy: implementation is not set yet') + }) + }) + + describe('setOwner', () => { + it('should succeed if called by the owner', async () => { + await expect( + L1ChugSplashProxy.connect(signer1).setOwner(await signer2.getAddress()) + ).to.not.be.reverted + + expect( + await L1ChugSplashProxy.connect(signer2).callStatic.getOwner() + ).to.equal(await signer2.getAddress()) + }) + + it('should otherwise pass through to the proxied contract', async () => { + await expect( + L1ChugSplashProxy.connect(signer2).setOwner(await signer1.getAddress()) + ).to.be.revertedWith('L1ChugSplashProxy: implementation is not set yet') + }) + }) + + describe('setStorage', () => {}) +}) From f41319e11fe9c08563856fa0f1d00397d4940532 Mon Sep 17 00:00:00 2001 From: Kelvin Fichter Date: Thu, 3 Jun 2021 14:10:55 -0400 Subject: [PATCH 04/11] add more tests --- .../chugsplash/L1ChugSplashProxy.sol | 15 ++++ .../chugsplash/L1ChugSplashProxy.spec.ts | 82 ++++++++++++++++++- 2 files changed, 95 insertions(+), 2 deletions(-) diff --git a/packages/contracts/contracts/chugsplash/L1ChugSplashProxy.sol b/packages/contracts/contracts/chugsplash/L1ChugSplashProxy.sol index 8a06b9dafc58..cd102052e541 100644 --- a/packages/contracts/contracts/chugsplash/L1ChugSplashProxy.sol +++ b/packages/contracts/contracts/chugsplash/L1ChugSplashProxy.sol @@ -165,6 +165,21 @@ contract L1ChugSplashProxy { return _getOwner(); } + /** + * Queries the implementation address. Can only be called by the owner OR by making an + * eth_call and setting the "from" address to address(0). + * @return Implementation address. + */ + function getImplementation() + proxyCallIfNotOwner + public + returns ( + address + ) + { + return _getImplementation(); + } + /********************** * Internal Functions * diff --git a/packages/contracts/test/contracts/chugsplash/L1ChugSplashProxy.spec.ts b/packages/contracts/test/contracts/chugsplash/L1ChugSplashProxy.spec.ts index adadf683fbe3..f50d2e5f4379 100644 --- a/packages/contracts/test/contracts/chugsplash/L1ChugSplashProxy.spec.ts +++ b/packages/contracts/test/contracts/chugsplash/L1ChugSplashProxy.spec.ts @@ -1,7 +1,7 @@ import { expect } from '../../setup' /* Imports: External */ -import hre, { ethers } from 'hardhat' +import hre from 'hardhat' import { Contract, Signer } from 'ethers' describe('L1ChugSplashProxy', () => { @@ -61,5 +61,83 @@ describe('L1ChugSplashProxy', () => { }) }) - describe('setStorage', () => {}) + describe('getImplementation', () => { + it('should succeed if called by the owner', async () => { + expect( + await L1ChugSplashProxy.connect(signer1).callStatic.getImplementation() + ).to.equal(hre.ethers.constants.AddressZero) + }) + + it('should otherwise pass through to the proxied contract', async () => { + await expect( + L1ChugSplashProxy.connect(signer2).getImplementation() + ).to.be.revertedWith('L1ChugSplashProxy: implementation is not set yet') + }) + }) + + describe('setStorage', () => { + it('should succeed if called by the owner', async () => { + const storageKey = hre.ethers.utils.keccak256('0x1234') + const storageValue = hre.ethers.utils.keccak256('0x5678') + + await expect( + L1ChugSplashProxy.connect(signer1).setStorage(storageKey, storageValue) + ).to.not.be.reverted + + expect( + await hre.ethers.provider.getStorageAt( + L1ChugSplashProxy.address, + storageKey + ) + ).to.equal(storageValue) + }) + + it('should otherwise pass through to the proxied contract', async () => { + const storageKey = hre.ethers.utils.keccak256('0x1234') + const storageValue = hre.ethers.utils.keccak256('0x5678') + + await expect( + L1ChugSplashProxy.connect(signer2).setStorage(storageKey, storageValue) + ).to.be.revertedWith('L1ChugSplashProxy: implementation is not set yet') + }) + }) + + describe('setCode', () => { + it('should succeed if called by the owner', async () => { + const code = '0x1234' + + await expect(L1ChugSplashProxy.connect(signer1).setCode(code)).to.not.be + .reverted + + const implementation = await L1ChugSplashProxy.connect( + signer1 + ).callStatic.getImplementation() + + expect(await hre.ethers.provider.getCode(implementation)).to.equal(code) + }) + }) + + describe('fallback', () => { + it('should pass through to the proxied contract', async () => { + await expect( + signer1.sendTransaction({ + to: L1ChugSplashProxy.address, + data: '0x', + }) + ).to.be.revertedWith('L1ChugSplashProxy: implementation is not set yet') + }) + + it('should execute the proxied contract when the implementation is set', async () => { + const code = '0x00' // STOP + + await L1ChugSplashProxy.connect(signer1).setCode(code) + + await expect( + signer1.sendTransaction({ + to: L1ChugSplashProxy.address, + data: '0x', + }) + ).to.not.be.reverted + }) + }) }) From 6595d6b1c7fe1742a1a348e186f603cd9310e063 Mon Sep 17 00:00:00 2001 From: Kelvin Fichter Date: Thu, 3 Jun 2021 14:34:22 -0400 Subject: [PATCH 05/11] make the system pausable --- .../chugsplash/L1ChugSplashProxy.sol | 36 +++++++++++++++++++ .../interfaces/iL1ChugSplashDeployer.sol | 14 ++++++++ .../chugsplash/L1ChugSplashProxy.spec.ts | 21 +++++++++++ 3 files changed, 71 insertions(+) create mode 100644 packages/contracts/contracts/chugsplash/interfaces/iL1ChugSplashDeployer.sol diff --git a/packages/contracts/contracts/chugsplash/L1ChugSplashProxy.sol b/packages/contracts/contracts/chugsplash/L1ChugSplashProxy.sol index cd102052e541..e2368695d0d9 100644 --- a/packages/contracts/contracts/chugsplash/L1ChugSplashProxy.sol +++ b/packages/contracts/contracts/chugsplash/L1ChugSplashProxy.sol @@ -1,6 +1,8 @@ // SPDX-License-Identifier: MIT pragma solidity >0.5.0 <0.8.0; +import { iL1ChugSplashDeployer } from "./interfaces/iL1ChugSplashDeployer.sol"; + /** * @title L1ChugSplashProxy * @dev Basic ChugSplash proxy contract for L1. Very close to being a normal proxy but has added @@ -47,6 +49,39 @@ contract L1ChugSplashProxy { * Function Modifiers * **********************/ + /** + * Blocks a function from being called when the parent signals that the system should be paused + * via an isUpgrading function. + */ + modifier onlyWhenNotPaused() { + address owner = _getOwner(); + + // We do a low-level call because there's no guarantee that the owner actually *is* an + // L1ChugSplashDeployer contract and Solidity will throw errors if we do a normal call and + // it turns out that it isn't the right type of contract. + (bool success, bytes memory returndata) = owner.call( + abi.encodeWithSelector( + iL1ChugSplashDeployer.isUpgrading.selector + ) + ); + + // If the call was unsuccessful then we assume that there's no "isUpgrading" method and we + // can just continue as normal. We also expect that the return value is exactly 32 bytes + // long. If this isn't the case then we can safely ignore the result. + if (success && returndata.length == 32) { + // Although the expected value is a *boolean*, it's safer to decode as a uint256 in the + // case that the isUpgrading function returned something other than 0 or 1. But we only + // really care about the case where this value is 0 (= false). + uint256 ret = abi.decode(returndata, (uint256)); + require( + ret == 0, + "L1ChugSplashProxy: system is currently being upgraded" + ); + } + + _; + } + /** * Makes a proxy call instead of triggering the given function when the caller is either the * owner or the zero address. Caller can only ever be the zero address if this function is @@ -253,6 +288,7 @@ contract L1ChugSplashProxy { * Performs the proxy call via a delegatecall. */ function _doProxyCall() + onlyWhenNotPaused internal { address implementation = _getImplementation(); diff --git a/packages/contracts/contracts/chugsplash/interfaces/iL1ChugSplashDeployer.sol b/packages/contracts/contracts/chugsplash/interfaces/iL1ChugSplashDeployer.sol new file mode 100644 index 000000000000..4117b7701f9a --- /dev/null +++ b/packages/contracts/contracts/chugsplash/interfaces/iL1ChugSplashDeployer.sol @@ -0,0 +1,14 @@ +// SPDX-License-Identifier: MIT +pragma solidity >0.5.0 <0.8.0; + +/** + * @title iL1ChugSplashDeployer + */ +interface iL1ChugSplashDeployer { + function isUpgrading() + external + view + returns ( + bool + ); +} diff --git a/packages/contracts/test/contracts/chugsplash/L1ChugSplashProxy.spec.ts b/packages/contracts/test/contracts/chugsplash/L1ChugSplashProxy.spec.ts index f50d2e5f4379..e759a12ba510 100644 --- a/packages/contracts/test/contracts/chugsplash/L1ChugSplashProxy.spec.ts +++ b/packages/contracts/test/contracts/chugsplash/L1ChugSplashProxy.spec.ts @@ -3,6 +3,10 @@ import { expect } from '../../setup' /* Imports: External */ import hre from 'hardhat' import { Contract, Signer } from 'ethers' +import { smockit } from '@eth-optimism/smock' + +/* Imports: Internal */ +import { getContractInterface } from '../../../src' describe('L1ChugSplashProxy', () => { let signer1: Signer @@ -139,5 +143,22 @@ describe('L1ChugSplashProxy', () => { }) ).to.not.be.reverted }) + + it('should throw an error if the owner has signalled an upgrade', async () => { + const owner = await smockit(getContractInterface('iL1ChugSplashDeployer')) + const factory = await hre.ethers.getContractFactory('L1ChugSplashProxy') + const proxy = await factory.deploy(owner.address) + + owner.smocked.isUpgrading.will.return.with(true) + + await expect( + owner.wallet.sendTransaction({ + to: proxy.address, + data: '0x', + }) + ).to.be.revertedWith( + 'L1ChugSplashProxy: system is currently being upgraded' + ) + }) }) }) From 8fed68ab8414651f640dae285bce522f60aaa8ba Mon Sep 17 00:00:00 2001 From: Kelvin Fichter Date: Thu, 3 Jun 2021 14:39:24 -0400 Subject: [PATCH 06/11] added another test --- .../chugsplash/L1ChugSplashProxy.spec.ts | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/packages/contracts/test/contracts/chugsplash/L1ChugSplashProxy.spec.ts b/packages/contracts/test/contracts/chugsplash/L1ChugSplashProxy.spec.ts index e759a12ba510..71e312e57a2c 100644 --- a/packages/contracts/test/contracts/chugsplash/L1ChugSplashProxy.spec.ts +++ b/packages/contracts/test/contracts/chugsplash/L1ChugSplashProxy.spec.ts @@ -119,6 +119,22 @@ describe('L1ChugSplashProxy', () => { expect(await hre.ethers.provider.getCode(implementation)).to.equal(code) }) + + it('should not change the implementation if the code does not change', async () => { + const code = '0x1234' + + await L1ChugSplashProxy.connect(signer1).setCode(code) + + const implementation = await L1ChugSplashProxy.connect( + signer1 + ).callStatic.getImplementation() + + await L1ChugSplashProxy.connect(signer1).setCode(code) + + expect( + await L1ChugSplashProxy.connect(signer1).callStatic.getImplementation() + ).to.equal(implementation) + }) }) describe('fallback', () => { From 9cc6ed6c3250d33a6a6bff015014c515f0dea500 Mon Sep 17 00:00:00 2001 From: Kelvin Fichter Date: Thu, 3 Jun 2021 18:07:09 -0400 Subject: [PATCH 07/11] add some extra comments --- .../contracts/chugsplash/L1ChugSplashProxy.sol | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/packages/contracts/contracts/chugsplash/L1ChugSplashProxy.sol b/packages/contracts/contracts/chugsplash/L1ChugSplashProxy.sol index e2368695d0d9..859c6d4b4be1 100644 --- a/packages/contracts/contracts/chugsplash/L1ChugSplashProxy.sol +++ b/packages/contracts/contracts/chugsplash/L1ChugSplashProxy.sol @@ -59,7 +59,7 @@ contract L1ChugSplashProxy { // We do a low-level call because there's no guarantee that the owner actually *is* an // L1ChugSplashDeployer contract and Solidity will throw errors if we do a normal call and // it turns out that it isn't the right type of contract. - (bool success, bytes memory returndata) = owner.call( + (bool success, bytes memory returndata) = owner.staticcall( abi.encodeWithSelector( iL1ChugSplashDeployer.isUpgrading.selector ) @@ -88,6 +88,13 @@ contract L1ChugSplashProxy { * being called off-chain via eth_call, which is totally fine and can be convenient for * client-side tooling. Avoids situations where the proxy and implementation share a sighash * and the proxy function ends up being called instead of the implementation one. + * + * Note: msg.sender == address(0) can ONLY be triggered off-chain via eth_call. If there's a + * way for someone to send a transaction with msg.sender == address(0) in any real context then + * we have much bigger problems. Primary reason to include this additional allowed sender is + * because the owner address can be changed dynamically and we do not want clients to have to + * keep track of the current owner in order to make an eth_call that doesn't trigger the + * proxied contract. */ modifier proxyCallIfNotOwner() { if (msg.sender == _getOwner() || msg.sender == address(0)) { @@ -105,6 +112,7 @@ contract L1ChugSplashProxy { fallback() external + payable { // Proxy call by default. _doProxyCall(); From ffc8c38f2d5adee239d6570c6dcc0cffec555188 Mon Sep 17 00:00:00 2001 From: smartcontracts Date: Thu, 3 Jun 2021 18:33:41 -0400 Subject: [PATCH 08/11] Update packages/contracts/test/contracts/chugsplash/L1ChugSplashProxy.spec.ts Co-authored-by: Maurelian --- .../test/contracts/chugsplash/L1ChugSplashProxy.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/contracts/test/contracts/chugsplash/L1ChugSplashProxy.spec.ts b/packages/contracts/test/contracts/chugsplash/L1ChugSplashProxy.spec.ts index 71e312e57a2c..c7b580cccbe2 100644 --- a/packages/contracts/test/contracts/chugsplash/L1ChugSplashProxy.spec.ts +++ b/packages/contracts/test/contracts/chugsplash/L1ChugSplashProxy.spec.ts @@ -138,7 +138,7 @@ describe('L1ChugSplashProxy', () => { }) describe('fallback', () => { - it('should pass through to the proxied contract', async () => { + it('should revert if implementation is not set', async () => { await expect( signer1.sendTransaction({ to: L1ChugSplashProxy.address, From 9916b74bb699ef02f24c1560ab94eefde1bb5a13 Mon Sep 17 00:00:00 2001 From: smartcontracts Date: Thu, 3 Jun 2021 18:33:48 -0400 Subject: [PATCH 09/11] Update packages/contracts/test/contracts/chugsplash/L1ChugSplashProxy.spec.ts Co-authored-by: Maurelian --- .../test/contracts/chugsplash/L1ChugSplashProxy.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/contracts/test/contracts/chugsplash/L1ChugSplashProxy.spec.ts b/packages/contracts/test/contracts/chugsplash/L1ChugSplashProxy.spec.ts index c7b580cccbe2..c1096b86bb81 100644 --- a/packages/contracts/test/contracts/chugsplash/L1ChugSplashProxy.spec.ts +++ b/packages/contracts/test/contracts/chugsplash/L1ChugSplashProxy.spec.ts @@ -120,7 +120,7 @@ describe('L1ChugSplashProxy', () => { expect(await hre.ethers.provider.getCode(implementation)).to.equal(code) }) - it('should not change the implementation if the code does not change', async () => { + it('should not change the implementation address if the code does not change', async () => { const code = '0x1234' await L1ChugSplashProxy.connect(signer1).setCode(code) From 1f70981d75aecebe83ce641abd50a26b28199668 Mon Sep 17 00:00:00 2001 From: Kelvin Fichter Date: Thu, 3 Jun 2021 18:35:12 -0400 Subject: [PATCH 10/11] chore: add changeset --- .changeset/witty-horses-nail.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/witty-horses-nail.md diff --git a/.changeset/witty-horses-nail.md b/.changeset/witty-horses-nail.md new file mode 100644 index 000000000000..daed003f40a6 --- /dev/null +++ b/.changeset/witty-horses-nail.md @@ -0,0 +1,5 @@ +--- +'@eth-optimism/contracts': patch +--- + +Introduce the L1ChugSplashProxy contract From 4ef8954883112ba8c427a2a54c613c4977d38da3 Mon Sep 17 00:00:00 2001 From: Kelvin Fichter Date: Tue, 8 Jun 2021 19:03:39 -0400 Subject: [PATCH 11/11] address review feedback --- .../chugsplash/L1ChugSplashProxy.sol | 37 ++++++++++++++++--- .../chugsplash/L1ChugSplashProxy.spec.ts | 10 +++++ 2 files changed, 42 insertions(+), 5 deletions(-) diff --git a/packages/contracts/contracts/chugsplash/L1ChugSplashProxy.sol b/packages/contracts/contracts/chugsplash/L1ChugSplashProxy.sol index 859c6d4b4be1..61cb6a186523 100644 --- a/packages/contracts/contracts/chugsplash/L1ChugSplashProxy.sol +++ b/packages/contracts/contracts/chugsplash/L1ChugSplashProxy.sol @@ -138,13 +138,9 @@ contract L1ChugSplashProxy { { // Get the code hash of the current implementation. address implementation = _getImplementation(); - bytes32 currentCodeHash; - assembly { - currentCodeHash := extcodehash(implementation) - } // If the code hash matches the new implementation then we return early. - if (keccak256(_code) == currentCodeHash) { + if (keccak256(_code) == _getAccountCodeHash(implementation)) { return; } @@ -159,6 +155,16 @@ contract L1ChugSplashProxy { assembly { newImplementation := create(0x0, add(deploycode, 0x20), mload(deploycode)) } + + // Check that the code was actually deployed correctly. I'm not sure if you can ever + // actually fail this check. Should only happen if the contract creation from above runs + // out of gas but this parent execution thread does NOT run out of gas. Seems like we + // should be doing this check anyway though. + require( + _getAccountCodeHash(newImplementation) == keccak256(_code), + "L1ChugSplashProxy: code was not correctly deployed." + ); + _setImplementation(newImplementation); } @@ -292,6 +298,27 @@ contract L1ChugSplashProxy { return owner; } + /** + * Gets the code hash for a given account. + * @param _account Address of the account to get a code hash for. + * @return Code hash for the account. + */ + function _getAccountCodeHash( + address _account + ) + internal + view + returns ( + bytes32 + ) + { + bytes32 codeHash; + assembly { + codeHash := extcodehash(_account) + } + return codeHash; + } + /** * Performs the proxy call via a delegatecall. */ diff --git a/packages/contracts/test/contracts/chugsplash/L1ChugSplashProxy.spec.ts b/packages/contracts/test/contracts/chugsplash/L1ChugSplashProxy.spec.ts index c1096b86bb81..f581fcdd1a57 100644 --- a/packages/contracts/test/contracts/chugsplash/L1ChugSplashProxy.spec.ts +++ b/packages/contracts/test/contracts/chugsplash/L1ChugSplashProxy.spec.ts @@ -72,6 +72,16 @@ describe('L1ChugSplashProxy', () => { ).to.equal(hre.ethers.constants.AddressZero) }) + it('should succeed if called by the zero address in an eth_call', async () => { + expect( + await L1ChugSplashProxy.connect( + hre.ethers.provider + ).callStatic.getImplementation({ + from: hre.ethers.constants.AddressZero, + }) + ).to.equal(hre.ethers.constants.AddressZero) + }) + it('should otherwise pass through to the proxied contract', async () => { await expect( L1ChugSplashProxy.connect(signer2).getImplementation()