From eebead8fa3cb83032c6c66c9a9697f0191a4b91b Mon Sep 17 00:00:00 2001 From: Ben Jones Date: Thu, 22 Apr 2021 16:59:47 -0700 Subject: [PATCH 1/5] feat: add OVM Upgrade Functions with stub executor --- .../OVM/execution/OVM_ExecutionManager.sol | 58 +++++++++++++++++++ .../OVM/execution/OVM_StateManager.sol | 17 ++++++ .../OVM/predeploys/OVM_UpgradeExecutor.sol | 24 ++++++++ .../iOVM/execution/iOVM_ExecutionManager.sol | 11 +++- .../iOVM/execution/iOVM_StateManager.sol | 1 + .../test/helpers/codec/revert-flags.ts | 1 + specs/protocol/data-structures.md | 1 + 7 files changed, 112 insertions(+), 1 deletion(-) create mode 100644 packages/contracts/contracts/optimistic-ethereum/OVM/predeploys/OVM_UpgradeExecutor.sol diff --git a/packages/contracts/contracts/optimistic-ethereum/OVM/execution/OVM_ExecutionManager.sol b/packages/contracts/contracts/optimistic-ethereum/OVM/execution/OVM_ExecutionManager.sol index ed2095b9eb9..2e64913577b 100644 --- a/packages/contracts/contracts/optimistic-ethereum/OVM/execution/OVM_ExecutionManager.sol +++ b/packages/contracts/contracts/optimistic-ethereum/OVM/execution/OVM_ExecutionManager.sol @@ -146,6 +146,18 @@ contract OVM_ExecutionManager is iOVM_ExecutionManager, Lib_AddressResolver { _; } + /** + * Modifies an EM method so that only a given address can invoke it. + */ + modifier onlyCallableBy( + address _allowed + ) { + if (ovmADDRESS() != _allowed) { + _revertWithFlag(RevertFlag.CALLER_NOT_ALLOWED); + } + _; + } + /************************************ * Transaction Execution Entrypoint * @@ -1872,6 +1884,52 @@ contract OVM_ExecutionManager is iOVM_ExecutionManager, Lib_AddressResolver { ovmStateManager = iOVM_StateManager(address(0)); } + + /********************* + * Upgrade Functions * + *********************/ + + /** + * Sets the code of an ovm contract. + * @param _address Address to update the code of. + * @param _code Bytecode to put into the ovm account. + */ + function ovmSETCODE( + address _address, + bytes memory _code + ) + override + external + onlyCallableBy(resolve("OVM_UpgradeExecutor")) + { + _checkAccountLoad(_address); + ovmStateManager.putAccountCode(_address, _code); + } + + + /** + * Sets the storage slot of an OVM contract. + * @param _address OVM account to set storage of. + * @param _key Key to set set. + * @param _value Value to store at the given key. + */ + function ovmSETSTORAGE( + address _address, + bytes32 _key, + bytes32 _value + ) + override + external + onlyCallableBy(resolve("OVM_UpgradeExecutor")) + { + _putContractStorage( + _address, + _key, + _value + ); + } + + /***************************** * L2-only Helper Functions * *****************************/ diff --git a/packages/contracts/contracts/optimistic-ethereum/OVM/execution/OVM_StateManager.sol b/packages/contracts/contracts/optimistic-ethereum/OVM/execution/OVM_StateManager.sol index ab18163f280..733a9bfd33e 100644 --- a/packages/contracts/contracts/optimistic-ethereum/OVM/execution/OVM_StateManager.sol +++ b/packages/contracts/contracts/optimistic-ethereum/OVM/execution/OVM_StateManager.sol @@ -143,6 +143,23 @@ contract OVM_StateManager is iOVM_StateManager { account.codeHash = EMPTY_ACCOUNT_CODE_HASH; } + /** + * Inserts the given bytecode into the given OVM account. + * @param _address Address of the account to overwrite code onto. + * @param _code Bytecode to put at the address. + */ + function putAccountCode( + address _address, + bytes memory _code + ) + override + public + authenticated + { + Lib_OVMCodec.Account storage account = accounts[_address]; + account.codeHash = keccak256(_code); + } + /** * Retrieves an account from the state. * @param _address Address of the account to retrieve. diff --git a/packages/contracts/contracts/optimistic-ethereum/OVM/predeploys/OVM_UpgradeExecutor.sol b/packages/contracts/contracts/optimistic-ethereum/OVM/predeploys/OVM_UpgradeExecutor.sol new file mode 100644 index 00000000000..a6a96e1774a --- /dev/null +++ b/packages/contracts/contracts/optimistic-ethereum/OVM/predeploys/OVM_UpgradeExecutor.sol @@ -0,0 +1,24 @@ +// SPDX-License-Identifier: MIT +// @unsupported: evm +pragma solidity >0.5.0 <0.8.0; + +/* Library Imports */ +import { Lib_ExecutionManagerWrapper } from "../../libraries/wrappers/Lib_ExecutionManagerWrapper.sol"; + +/** + * @title OVM_UpgradeExecutor + * @dev The OVM_UpgradeExecutor is the contract which authenticates and executes (i.e. + * calls the relevant Execution Manager upgrade functions) upgrades to the OVM State. + * This enables us to update the predeploy and execution contracts directly from within + * L2 when there is a new release. + * + * Compiler used: optimistic-solc + * Runtime target: OVM + */ +contract OVM_UpgradeExecutor { + fallback() + external + { + revert("Not yet implemented."); + } +} diff --git a/packages/contracts/contracts/optimistic-ethereum/iOVM/execution/iOVM_ExecutionManager.sol b/packages/contracts/contracts/optimistic-ethereum/iOVM/execution/iOVM_ExecutionManager.sol index 0ecbc2274bf..b88881094e3 100644 --- a/packages/contracts/contracts/optimistic-ethereum/iOVM/execution/iOVM_ExecutionManager.sol +++ b/packages/contracts/contracts/optimistic-ethereum/iOVM/execution/iOVM_ExecutionManager.sol @@ -18,7 +18,8 @@ interface iOVM_ExecutionManager { UNSAFE_BYTECODE, CREATE_COLLISION, STATIC_VIOLATION, - CREATOR_NOT_ALLOWED + CREATOR_NOT_ALLOWED, + CALLER_NOT_ALLOWED } enum GasMetadataKey { @@ -153,4 +154,12 @@ interface iOVM_ExecutionManager { ***************************************/ function getMaxTransactionGasLimit() external view returns (uint _maxTransactionGasLimit); + + + /********************* + * Upgrade Functions * + *********************/ + + function ovmSETCODE(address _address, bytes memory _code) external; + function ovmSETSTORAGE(address _address, bytes32 _key, bytes32 _value) external; } diff --git a/packages/contracts/contracts/optimistic-ethereum/iOVM/execution/iOVM_StateManager.sol b/packages/contracts/contracts/optimistic-ethereum/iOVM/execution/iOVM_StateManager.sol index 1c4870a5841..d2771b483b4 100644 --- a/packages/contracts/contracts/optimistic-ethereum/iOVM/execution/iOVM_StateManager.sol +++ b/packages/contracts/contracts/optimistic-ethereum/iOVM/execution/iOVM_StateManager.sol @@ -42,6 +42,7 @@ interface iOVM_StateManager { function putAccount(address _address, Lib_OVMCodec.Account memory _account) external; function putEmptyAccount(address _address) external; + function putAccountCode(address _address, bytes memory _code) external; function getAccount(address _address) external view returns (Lib_OVMCodec.Account memory _account); function hasAccount(address _address) external view returns (bool _exists); function hasEmptyAccount(address _address) external view returns (bool _exists); diff --git a/packages/contracts/test/helpers/codec/revert-flags.ts b/packages/contracts/test/helpers/codec/revert-flags.ts index c0326b25dd9..0486b5f9595 100644 --- a/packages/contracts/test/helpers/codec/revert-flags.ts +++ b/packages/contracts/test/helpers/codec/revert-flags.ts @@ -42,4 +42,5 @@ export const REVERT_FLAGS = { CREATE_COLLISION: 5, STATIC_VIOLATION: 6, CREATOR_NOT_ALLOWED: 7, + CALLER_NOT_ALLOWED: 8, } diff --git a/specs/protocol/data-structures.md b/specs/protocol/data-structures.md index 43fb7c99d47..80770d21262 100644 --- a/specs/protocol/data-structures.md +++ b/specs/protocol/data-structures.md @@ -211,6 +211,7 @@ Enum encoding the reason for reverting during execution in the OVM. | CREATE_COLLISION | | | STATIC_VIOLATION | | | CREATOR_NOT_ALLOWED | | +| CALLER_NOT_ALLOWED | | ### GasMetadataKey (enum) From 111b63f967d4ec8f64d4c308f515a2849841c148 Mon Sep 17 00:00:00 2001 From: Ben Jones Date: Thu, 22 Apr 2021 17:30:11 -0700 Subject: [PATCH 2/5] test: smocked unit tests for upgrade paths get tests passing --- .../wrappers/Lib_ExecutionManagerWrapper.sol | 36 +++ .../src/contract-deployment/config.ts | 4 + packages/contracts/src/contract-dumps.ts | 2 + packages/contracts/src/predeploys.ts | 1 + .../OVM_ExecutionManager/nuisance-gas.spec.ts | 2 +- .../OVM_ExecutionManager/ovmSETCODE.spec.ts | 189 ++++++++++++++ .../ovmSETSTORAGE.spec.ts | 237 ++++++++++++++++++ .../test/helpers/test-runner/test-runner.ts | 13 +- .../test/helpers/test-runner/test.types.ts | 35 +++ 9 files changed, 516 insertions(+), 3 deletions(-) create mode 100644 packages/contracts/test/contracts/OVM/execution/OVM_ExecutionManager/ovmSETCODE.spec.ts create mode 100644 packages/contracts/test/contracts/OVM/execution/OVM_ExecutionManager/ovmSETSTORAGE.spec.ts diff --git a/packages/contracts/contracts/optimistic-ethereum/libraries/wrappers/Lib_ExecutionManagerWrapper.sol b/packages/contracts/contracts/optimistic-ethereum/libraries/wrappers/Lib_ExecutionManagerWrapper.sol index 80fa0a86d85..148ec3d24a3 100644 --- a/packages/contracts/contracts/optimistic-ethereum/libraries/wrappers/Lib_ExecutionManagerWrapper.sol +++ b/packages/contracts/contracts/optimistic-ethereum/libraries/wrappers/Lib_ExecutionManagerWrapper.sol @@ -137,6 +137,42 @@ library Lib_ExecutionManagerWrapper { return abi.decode(returndata, (uint256)); } + /* + * Performs a safe ovmSETCODE call. + */ + function safeSETCODE( + address _address, + bytes memory _code + ) + internal + { + _safeExecutionManagerInteraction( + abi.encodeWithSignature( + "ovmSETCODE(address,bytes)", + _address, + _code + ) + ); + } + + /** + * Performs a safe ovmSETSTORAGE call. + */ + function ovmSETSTORAGE( + address _address, + bytes memory _code + ) + internal + { + _safeExecutionManagerInteraction( + abi.encodeWithSignature( + "ovmSETSTORAGE(address,bytes)", + _address, + _code + ) + ); + } + /********************* * Private Functions * diff --git a/packages/contracts/src/contract-deployment/config.ts b/packages/contracts/src/contract-deployment/config.ts index 4f1ad42a9b7..5e41caf859b 100644 --- a/packages/contracts/src/contract-deployment/config.ts +++ b/packages/contracts/src/contract-deployment/config.ts @@ -233,6 +233,10 @@ export const makeContractDeployConfig = async ( '0x0000000000000000000000000000000000000000', // will be overridden by geth when state dump is ingested. Storage key: 0x0000000000000000000000000000000000000000000000000000000000000008 ], }, + OVM_UpgradeExecutor: { + factory: getContractFactory('OVM_UpgradeExecutor'), + params: [], + }, 'OVM_ChainStorageContainer:CTC:batches': { factory: getContractFactory('OVM_ChainStorageContainer'), params: [AddressManager.address, 'OVM_CanonicalTransactionChain'], diff --git a/packages/contracts/src/contract-dumps.ts b/packages/contracts/src/contract-dumps.ts index b9fdc941702..a23e2d27f68 100644 --- a/packages/contracts/src/contract-dumps.ts +++ b/packages/contracts/src/contract-dumps.ts @@ -152,6 +152,7 @@ export const makeStateDump = async (cfg: RollupDeployConfig): Promise => { 'OVM_ExecutionManager', 'OVM_StateManager', 'OVM_ETH', + 'OVM_Upgrader', ], deployOverrides: {}, waitForReceipts: false, @@ -168,6 +169,7 @@ export const makeStateDump = async (cfg: RollupDeployConfig): Promise => { 'OVM_ETH', 'OVM_ECDSAContractAccount', 'OVM_ProxyEOA', + 'OVM_Upgrader', ] const deploymentResult = await deploy(config) diff --git a/packages/contracts/src/predeploys.ts b/packages/contracts/src/predeploys.ts index 05bc306ea75..ab9ad20caf4 100644 --- a/packages/contracts/src/predeploys.ts +++ b/packages/contracts/src/predeploys.ts @@ -17,5 +17,6 @@ export const predeploys = { OVM_L2CrossDomainMessenger: '0x4200000000000000000000000000000000000007', Lib_AddressManager: '0x4200000000000000000000000000000000000008', OVM_ProxyEOA: '0x4200000000000000000000000000000000000009', + OVM_UpgradeExecutor: '0x420000000000000000000000000000000000000a', ERC1820Registry: '0x1820a4B7618BdE71Dce8cdc73aAB6C95905faD24', } diff --git a/packages/contracts/test/contracts/OVM/execution/OVM_ExecutionManager/nuisance-gas.spec.ts b/packages/contracts/test/contracts/OVM/execution/OVM_ExecutionManager/nuisance-gas.spec.ts index cfa5a449f38..9c01fbd0eb2 100644 --- a/packages/contracts/test/contracts/OVM/execution/OVM_ExecutionManager/nuisance-gas.spec.ts +++ b/packages/contracts/test/contracts/OVM/execution/OVM_ExecutionManager/nuisance-gas.spec.ts @@ -188,7 +188,7 @@ const test_nuisanceGas: TestDefinition = { // This is because there is natural gas consumption between the ovmCALL(GAS/2) and ovmCREATE, which allots nuisance gas via _getNuisanceGasLimit. // This means that the ovmCREATE exception, DOES consumes all nuisance gas allotted, but that allotment // is less than the full OVM_TX_GAS_LIMIT / 2 which is alloted to the parent ovmCALL. - nuisanceGasLeft: 0x3f9e7f, + nuisanceGasLeft: 0x3f9fc9, }, }, }, diff --git a/packages/contracts/test/contracts/OVM/execution/OVM_ExecutionManager/ovmSETCODE.spec.ts b/packages/contracts/test/contracts/OVM/execution/OVM_ExecutionManager/ovmSETCODE.spec.ts new file mode 100644 index 00000000000..c7a3bd40950 --- /dev/null +++ b/packages/contracts/test/contracts/OVM/execution/OVM_ExecutionManager/ovmSETCODE.spec.ts @@ -0,0 +1,189 @@ +/* External Imports */ +import { ethers } from 'ethers' +// import { merge } from 'lodash' + +/* Internal Imports */ +import { + ExecutionManagerTestRunner, + TestDefinition, + OVM_TX_GAS_LIMIT, + NON_NULL_BYTES32, + getStorageXOR, + REVERT_FLAGS, +} from '../../../../helpers' +import { predeploys } from '../../../../../src/predeploys' + +const UPGRADE_EXECUTOR_ADDRESS = predeploys.OVM_UpgradeExecutor +const UPGRADED_ADDRESS = '0x1234123412341234123412341234123412341234' +const UPGRADED_CODE = '0x1234' +const UPGRADED_CODEHASH = ethers.utils.keccak256(UPGRADED_CODE) + +const sharedPreState = { + ExecutionManager: { + ovmStateManager: '$OVM_STATE_MANAGER', + ovmSafetyChecker: '$OVM_SAFETY_CHECKER', + messageRecord: { + nuisanceGasLeft: OVM_TX_GAS_LIMIT, + }, + }, + StateManager: { + owner: '$OVM_EXECUTION_MANAGER', + accounts: { + $DUMMY_OVM_ADDRESS_1: { + codeHash: NON_NULL_BYTES32, + ethAddress: '$OVM_CALL_HELPER', + }, + [UPGRADE_EXECUTOR_ADDRESS]: { + codeHash: NON_NULL_BYTES32, + ethAddress: '$OVM_CALL_HELPER', + }, + }, + contractStorage: { + [UPGRADED_ADDRESS]: { + [NON_NULL_BYTES32]: getStorageXOR(ethers.constants.HashZero), + }, + }, + }, +} + +const verifiedUpgradePreState = { + StateManager: { + accounts: { + [UPGRADED_ADDRESS]: { + codeHash: NON_NULL_BYTES32, + ethAddress: '$OVM_CALL_HELPER', + }, + }, + // verifiedContractStorage: { + // [UPGRADED_ADDRESS]: { + // [NON_NULL_BYTES32]: true, + // }, + // }, + }, +} + +const test_ovmSETCODEFunctionality: TestDefinition = { + name: 'Functionality tests for ovmSETCODE', + preState: sharedPreState, + subTests: [ + { + name: 'ovmSETCODE -- success case', + preState: verifiedUpgradePreState, + postState: { + StateManager: { + accounts: { + [UPGRADED_ADDRESS]: { + codeHash: UPGRADED_CODEHASH, + ethAddress: '$OVM_CALL_HELPER', + }, + }, + }, + }, + parameters: [ + { + name: 'success case', + steps: [ + { + functionName: 'ovmCALL', + functionParams: { + gasLimit: OVM_TX_GAS_LIMIT, + target: UPGRADE_EXECUTOR_ADDRESS, + subSteps: [ + { + functionName: 'ovmSETCODE', + functionParams: { + address: UPGRADED_ADDRESS, + code: UPGRADED_CODE, + }, + expectedReturnStatus: true, + }, + ], + }, + expectedReturnStatus: true, + }, + ], + }, + ], + }, + { + name: 'ovmSETCODE -- unauthorized case', + preState: verifiedUpgradePreState, + parameters: [ + { + name: 'unauthorized case', + steps: [ + { + functionName: 'ovmCALL', + functionParams: { + gasLimit: OVM_TX_GAS_LIMIT, + target: '$DUMMY_OVM_ADDRESS_1', + subSteps: [ + { + functionName: 'ovmSETCODE', + functionParams: { + address: UPGRADED_ADDRESS, + code: UPGRADED_CODE, + }, + expectedReturnStatus: false, + expectedReturnValue: { + flag: REVERT_FLAGS.CALLER_NOT_ALLOWED, + onlyValidateFlag: true, + }, + }, + ], + }, + expectedReturnStatus: false, + }, + ], + }, + ], + }, + ], +} + +const test_ovmSETCODEAccess: TestDefinition = { + name: 'State access compliance tests for ovmSETCODE', + preState: sharedPreState, + subTests: [ + { + name: 'ovmSETSTORAGE (UNVERIFIED_ACCOUNT)', + parameters: [ + { + name: 'ovmSETSTORAGE with a missing account', + expectInvalidStateAccess: true, + steps: [ + { + functionName: 'ovmCALL', + functionParams: { + gasLimit: OVM_TX_GAS_LIMIT, + target: UPGRADE_EXECUTOR_ADDRESS, + subSteps: [ + { + functionName: 'ovmSETCODE', + functionParams: { + address: UPGRADED_ADDRESS, + code: UPGRADED_CODE, + }, + expectedReturnStatus: false, + expectedReturnValue: { + flag: REVERT_FLAGS.INVALID_STATE_ACCESS, + onlyValidateFlag: true, + }, + }, + ], + }, + expectedReturnStatus: false, + expectedReturnValue: { + flag: REVERT_FLAGS.INVALID_STATE_ACCESS, + onlyValidateFlag: true, + }, + }, + ], + }, + ], + }, + ], +} +const runner = new ExecutionManagerTestRunner() +runner.run(test_ovmSETCODEFunctionality) +runner.run(test_ovmSETCODEAccess) diff --git a/packages/contracts/test/contracts/OVM/execution/OVM_ExecutionManager/ovmSETSTORAGE.spec.ts b/packages/contracts/test/contracts/OVM/execution/OVM_ExecutionManager/ovmSETSTORAGE.spec.ts new file mode 100644 index 00000000000..85aa2143865 --- /dev/null +++ b/packages/contracts/test/contracts/OVM/execution/OVM_ExecutionManager/ovmSETSTORAGE.spec.ts @@ -0,0 +1,237 @@ +/* External Imports */ +import { ethers } from 'ethers' +// import { merge } from 'lodash' + +/* Internal Imports */ +import { + ExecutionManagerTestRunner, + TestDefinition, + OVM_TX_GAS_LIMIT, + NON_NULL_BYTES32, + getStorageXOR, + REVERT_FLAGS, +} from '../../../../helpers' +import { predeploys } from '../../../../../src/predeploys' + +const UPGRADE_EXECUTOR_ADDRESS = predeploys.OVM_UpgradeExecutor +const UPGRADED_ADDRESS = '0x1234123412341234123412341234123412341234' + +const sharedPreState = { + ExecutionManager: { + ovmStateManager: '$OVM_STATE_MANAGER', + ovmSafetyChecker: '$OVM_SAFETY_CHECKER', + messageRecord: { + nuisanceGasLeft: OVM_TX_GAS_LIMIT, + }, + }, + StateManager: { + owner: '$OVM_EXECUTION_MANAGER', + accounts: { + $DUMMY_OVM_ADDRESS_1: { + codeHash: NON_NULL_BYTES32, + ethAddress: '$OVM_CALL_HELPER', + }, + [UPGRADE_EXECUTOR_ADDRESS]: { + codeHash: NON_NULL_BYTES32, + ethAddress: '$OVM_CALL_HELPER', + }, + }, + contractStorage: { + [UPGRADED_ADDRESS]: { + [NON_NULL_BYTES32]: getStorageXOR(ethers.constants.HashZero), + }, + }, + }, +} + +const verifiedUpgradePreState = { + StateManager: { + accounts: { + [UPGRADED_ADDRESS]: { + codeHash: NON_NULL_BYTES32, + ethAddress: '$OVM_CALL_HELPER', + }, + }, + verifiedContractStorage: { + [UPGRADED_ADDRESS]: { + [NON_NULL_BYTES32]: true, + }, + }, + }, +} + +const test_ovmSETSTORAGEFunctionality: TestDefinition = { + name: 'Functionality tests for ovmSETSTORAGE', + preState: sharedPreState, + subTests: [ + { + name: 'ovmSETSTORAGE -- success case', + preState: verifiedUpgradePreState, + postState: { + StateManager: { + contractStorage: { + [UPGRADED_ADDRESS]: { + [NON_NULL_BYTES32]: getStorageXOR(NON_NULL_BYTES32), + }, + }, + }, + }, + parameters: [ + { + name: 'success case', + steps: [ + { + functionName: 'ovmCALL', + functionParams: { + gasLimit: OVM_TX_GAS_LIMIT, + target: UPGRADE_EXECUTOR_ADDRESS, + subSteps: [ + { + functionName: 'ovmSETSTORAGE', + functionParams: { + address: UPGRADED_ADDRESS, + key: NON_NULL_BYTES32, + value: NON_NULL_BYTES32, + }, + expectedReturnStatus: true, + }, + ], + }, + expectedReturnStatus: true, + }, + ], + }, + ], + }, + { + name: 'ovmSETSTORAGE -- unauthorized case', + preState: verifiedUpgradePreState, + parameters: [ + { + name: 'unauthorized case', + steps: [ + { + functionName: 'ovmCALL', + functionParams: { + gasLimit: OVM_TX_GAS_LIMIT, + target: '$DUMMY_OVM_ADDRESS_1', + subSteps: [ + { + functionName: 'ovmSETSTORAGE', + functionParams: { + address: UPGRADED_ADDRESS, + key: NON_NULL_BYTES32, + value: NON_NULL_BYTES32, + }, + expectedReturnStatus: false, + expectedReturnValue: { + flag: REVERT_FLAGS.CALLER_NOT_ALLOWED, + onlyValidateFlag: true, + }, + }, + ], + }, + expectedReturnStatus: false, + }, + ], + }, + ], + }, + ], +} + +const test_ovmSETSTORAGEAccess: TestDefinition = { + name: 'State access compliance tests for ovmSETSTORAGE', + preState: sharedPreState, + subTests: [ + { + name: 'ovmSETSTORAGE (UNVERIFIED_ACCOUNT)', + parameters: [ + { + name: 'ovmSETSTORAGE with a missing account', + expectInvalidStateAccess: true, + steps: [ + { + functionName: 'ovmCALL', + functionParams: { + gasLimit: OVM_TX_GAS_LIMIT, + target: UPGRADE_EXECUTOR_ADDRESS, + subSteps: [ + { + functionName: 'ovmSETSTORAGE', + functionParams: { + address: UPGRADED_ADDRESS, + key: NON_NULL_BYTES32, + value: NON_NULL_BYTES32, + }, + expectedReturnStatus: false, + expectedReturnValue: { + flag: REVERT_FLAGS.INVALID_STATE_ACCESS, + onlyValidateFlag: true, + }, + }, + ], + }, + expectedReturnStatus: false, + expectedReturnValue: { + flag: REVERT_FLAGS.INVALID_STATE_ACCESS, + onlyValidateFlag: true, + }, + }, + ], + }, + ], + }, + { + name: 'ovmSETSTORAGE (UNVERIFIED_SLOT)', + preState: { + StateManager: { + accounts: { + [UPGRADED_ADDRESS]: { + codeHash: NON_NULL_BYTES32, + ethAddress: '$OVM_CALL_HELPER', + }, + }, + }, + }, + parameters: [ + { + name: 'ovmSETSTORAGE with a missing storage slot', + expectInvalidStateAccess: true, + steps: [ + { + functionName: 'ovmCALL', + functionParams: { + gasLimit: OVM_TX_GAS_LIMIT, + target: UPGRADE_EXECUTOR_ADDRESS, + subSteps: [ + { + functionName: 'ovmSETSTORAGE', + functionParams: { + address: UPGRADED_ADDRESS, + key: NON_NULL_BYTES32, + value: NON_NULL_BYTES32, + }, + expectedReturnStatus: false, + expectedReturnValue: { + flag: REVERT_FLAGS.INVALID_STATE_ACCESS, + onlyValidateFlag: true, + }, + }, + ], + }, + expectedReturnStatus: false, + expectedReturnValue: { + flag: REVERT_FLAGS.INVALID_STATE_ACCESS, + onlyValidateFlag: true, + }, + }, + ], + }, + ], + }, + ], +} +const runner = new ExecutionManagerTestRunner() +runner.run(test_ovmSETSTORAGEFunctionality) +runner.run(test_ovmSETSTORAGEAccess) diff --git a/packages/contracts/test/helpers/test-runner/test-runner.ts b/packages/contracts/test/helpers/test-runner/test-runner.ts index fdb9e074a2d..7b7ba1874bc 100644 --- a/packages/contracts/test/helpers/test-runner/test-runner.ts +++ b/packages/contracts/test/helpers/test-runner/test-runner.ts @@ -28,6 +28,8 @@ import { isTestStep_EXTCODEHASH, isTestStep_EXTCODECOPY, isTestStep_REVERT, + isTestStep_SETCODE, + isTestStep_SETSTORAGE, } from './test.types' import { encodeRevertData, REVERT_FLAGS } from '../codec' import { @@ -37,7 +39,7 @@ import { } from '../constants' import { getStorageXOR } from '../' import { UNSAFE_BYTECODE } from '../dummy' -import { getContractFactory } from '../../../src' +import { getContractFactory, predeploys } from '../../../src' export class ExecutionManagerTestRunner { private snapshot: string @@ -218,6 +220,11 @@ export class ExecutionManagerTestRunner { this.contracts.OVM_SafetyChecker.address ) + await AddressManager.setAddress( + 'OVM_UpgradeExecutor', + predeploys.OVM_UpgradeExecutor + ) + const DeployerWhitelist = await getContractFactory( 'OVM_DeployerWhitelist', AddressManager.signer, @@ -426,7 +433,9 @@ export class ExecutionManagerTestRunner { isTestStep_EXTCODESIZE(step) || isTestStep_EXTCODEHASH(step) || isTestStep_EXTCODECOPY(step) || - isTestStep_CREATEEOA(step) + isTestStep_CREATEEOA(step) || + isTestStep_SETCODE(step) || + isTestStep_SETSTORAGE(step) ) { functionParams = Object.values(step.functionParams) } else if (isTestStep_CALL(step)) { diff --git a/packages/contracts/test/helpers/test-runner/test.types.ts b/packages/contracts/test/helpers/test-runner/test.types.ts index fa799dd6b4e..79e8dc9af98 100644 --- a/packages/contracts/test/helpers/test-runner/test.types.ts +++ b/packages/contracts/test/helpers/test-runner/test.types.ts @@ -169,6 +169,27 @@ export interface TestStep_Run { expectedRevertValue?: string } +export interface TestStep_SETCODE { + functionName: 'ovmSETCODE' + functionParams: { + address: string + code: string + } + expectedReturnStatus: boolean + expectedReturnValue?: RevertFlagError +} + +export interface TestStep_SETSTORAGE { + functionName: 'ovmSETSTORAGE' + functionParams: { + address: string + key: string + value: string + } + expectedReturnStatus: boolean + expectedReturnValue?: RevertFlagError +} + export type TestStep = | TestStep_Context | TestStep_SSTORE @@ -183,6 +204,8 @@ export type TestStep = | TestStep_EXTCODECOPY | TestStep_REVERT | TestStep_evm + | TestStep_SETCODE + | TestStep_SETSTORAGE export interface ParsedTestStep { functionName: string @@ -281,6 +304,18 @@ export const isTestStep_CREATE2 = ( return step.functionName === 'ovmCREATE2' } +export const isTestStep_SETCODE = ( + step: TestStep | TestStep_Run +): step is TestStep_SETCODE => { + return step.functionName === 'ovmSETCODE' +} + +export const isTestStep_SETSTORAGE = ( + step: TestStep | TestStep_Run +): step is TestStep_SETSTORAGE => { + return step.functionName === 'ovmSETSTORAGE' +} + export const isTestStep_Run = ( step: TestStep | TestStep_Run ): step is TestStep_Run => { From 403129210111830e622ade942a44062f1683f8f4 Mon Sep 17 00:00:00 2001 From: Ben Jones Date: Thu, 22 Apr 2021 23:40:34 -0700 Subject: [PATCH 3/5] feat(l2geth): support putAccountCode --- l2geth/core/vm/ovm_state_manager.go | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/l2geth/core/vm/ovm_state_manager.go b/l2geth/core/vm/ovm_state_manager.go index 1e85e69af14..0fd316527cc 100644 --- a/l2geth/core/vm/ovm_state_manager.go +++ b/l2geth/core/vm/ovm_state_manager.go @@ -19,6 +19,7 @@ var funcs = map[string]stateManagerFunction{ "getAccountEthAddress": getAccountEthAddress, "getContractStorage": getContractStorage, "putContractStorage": putContractStorage, + "putAccountCode": putAccountCode, "isAuthenticated": nativeFunctionTrue, "hasAccount": nativeFunctionTrue, "hasEmptyAccount": hasEmptyAccount, @@ -156,6 +157,32 @@ func putContractStorage(evm *EVM, contract *Contract, args map[string]interface{ return []interface{}{}, nil } +func putAccountCode(evm *EVM, contract *Contract, args map[string]interface{}) ([]interface{}, error) { + address, ok := args["_address"].(common.Address) + if !ok { + return nil, errors.New("Could not parse address arg in putAccountCode") + } + code, ok := args["_code"].([]byte) + if !ok { + return nil, errors.New("Could not parse code arg in putAccountCode") + } + + // save the block number and address with modified key if it's not an eth_call + if evm.Context.EthCallSender == nil { + err := evm.StateDB.SetDiffAccount( + evm.Context.BlockNumber, + address, + ) + if err != nil { + log.Error("Cannot set diff key", "err", err) + } + } + evm.StateDB.SetCode(address, code) + log.Debug("Put account code", "address", address.Hex(), "code", code) + + return []interface{}{}, nil +} + func testAndSetAccount(evm *EVM, contract *Contract, args map[string]interface{}) ([]interface{}, error) { address, ok := args["_address"].(common.Address) if !ok { From 9c94a185d28ba3053d3be45095c7119cbd14222a Mon Sep 17 00:00:00 2001 From: Ben Jones Date: Thu, 22 Apr 2021 23:42:32 -0700 Subject: [PATCH 4/5] test: integration test unathenticated executor --- .../test/ovm-self-upgrades.spec.ts | 131 ++++++++++++++++++ l2geth/core/vm/ovm_state_manager.go | 3 +- .../OVM/execution/OVM_ExecutionManager.sol | 4 +- .../OVM/predeploys/OVM_UpgradeExecutor.sol | 24 +++- .../wrappers/Lib_ExecutionManagerWrapper.sol | 10 +- .../src/contract-deployment/config.ts | 2 +- packages/contracts/src/contract-dumps.ts | 4 +- 7 files changed, 166 insertions(+), 12 deletions(-) create mode 100644 integration-tests/test/ovm-self-upgrades.spec.ts diff --git a/integration-tests/test/ovm-self-upgrades.spec.ts b/integration-tests/test/ovm-self-upgrades.spec.ts new file mode 100644 index 00000000000..2f07ddc38fe --- /dev/null +++ b/integration-tests/test/ovm-self-upgrades.spec.ts @@ -0,0 +1,131 @@ +import { expect } from 'chai' +import { Wallet, utils, BigNumber, Contract } from 'ethers' +import { Direction } from './shared/watcher-utils' + +import { OptimismEnv } from './shared/env' + +import { getContractInterface } from '@eth-optimism/contracts' +import { l2Provider, OVM_ETH_ADDRESS } from './shared/utils' +import { ethers } from 'hardhat' + + +// TODO: use actual imported Chugsplash type + +interface SetCodeInstruction { + target: string // address + code: string // bytes memory +} + +interface SetStorageInstruction { + target: string // address + key: string // bytes32 + value: string // bytes32 +} + +type ChugsplashInstruction = SetCodeInstruction | SetStorageInstruction + +// Just an array of the above two instruction types. +type ChugSplashInstructions = Array + +const isSetStorageInstruction = (instr: ChugsplashInstruction): instr is SetStorageInstruction => { + return !instr["code"] +} + +describe('OVM Self-Upgrades', async () => { + let env: OptimismEnv + let l2Wallet: Wallet + let OVM_UpgradeExecutor: Contract + + const applyChugsplashInstructions = async (instructions: ChugSplashInstructions) => { + for (const instruction of instructions) { + let res + if (isSetStorageInstruction(instruction)) { + res = await OVM_UpgradeExecutor.setStorage( + instruction.target, + instruction.key, + instruction.value + ) + } else { + res = await OVM_UpgradeExecutor.setCode( + instruction.target, + instruction.code + ) + } + await res.wait() // TODO: promise.all + } + } + + const checkChugsplashInstructionsApplied = async (instructions: ChugSplashInstructions) => { + for (const instruction of instructions) { + // TODO: promise.all this for with a map for efficiency + if (isSetStorageInstruction(instruction)) { + const actualStorage = await l2Provider.getStorageAt( + instruction.target, + instruction.key + ) + expect(actualStorage).to.deep.eq(instruction.value) + } else { + const actualCode = await l2Provider.getCode( + instruction.target + ) + expect(actualCode).to.deep.eq(instruction.code) + } + } + } + + const applyAndVerifyUpgrade = async (instructions: ChugSplashInstructions) => { + await applyChugsplashInstructions(instructions) + await checkChugsplashInstructionsApplied(instructions) + } + + before(async () => { + env = await OptimismEnv.new() + l2Wallet = env.l2Wallet + + OVM_UpgradeExecutor = new Contract( + '0x420000000000000000000000000000000000000a', + getContractInterface('OVM_UpgradeExecutor', true), + l2Wallet + ) + }) + + describe('setStorage and setCode are correctly applied', () => { + it('Should execute a basic storage upgrade', async () => { + const basicStorageUpgrade: ChugSplashInstructions = [ + { + target: OVM_ETH_ADDRESS, + key: '0x1234123412341234123412341234123412341234123412341234123412341234', + value: '0x6789123412341234123412341234123412341234123412341234678967896789', + } + ] + await applyAndVerifyUpgrade(basicStorageUpgrade) + }) + + it('Should execute a basic upgrade overwriting existing deployed code', async () => { + const DummyContract = await ( + await ethers.getContractFactory('SimpleStorage', l2Wallet) + ).deploy() + await DummyContract.deployTransaction.wait() + + const basicCodeUpgrade: ChugSplashInstructions = [ + { + target: DummyContract.address, + code: '0x1234123412341234123412341234123412341234123412341234123412341234', + } + ] + await applyAndVerifyUpgrade(basicCodeUpgrade) + }) + + it('Should execute a basic code upgrade which is not overwriting an existing account', async () => { + // TODO: fix me? Currently breaks due to nil pointer dereference; triggerd by evm.StateDB.SetCode(...) in ovm_state_manager.go ? + // More recent update: I cannot get this to error out any more. + const emptyAccountCodeUpgrade: ChugSplashInstructions = [ + { + target: '0x5678657856785678567856785678567856785678', + code: '0x1234123412341234123412341234123412341234123412341234123412341234', + } + ] + await applyAndVerifyUpgrade(emptyAccountCodeUpgrade) + }) + }) +}) diff --git a/l2geth/core/vm/ovm_state_manager.go b/l2geth/core/vm/ovm_state_manager.go index 0fd316527cc..5e9c91740c0 100644 --- a/l2geth/core/vm/ovm_state_manager.go +++ b/l2geth/core/vm/ovm_state_manager.go @@ -1,6 +1,7 @@ package vm import ( + "encoding/hex" "errors" "fmt" "math/big" @@ -178,7 +179,7 @@ func putAccountCode(evm *EVM, contract *Contract, args map[string]interface{}) ( } } evm.StateDB.SetCode(address, code) - log.Debug("Put account code", "address", address.Hex(), "code", code) + log.Debug("Put account code", "address", address.Hex(), "code", hex.EncodeToString(code)) return []interface{}{}, nil } diff --git a/packages/contracts/contracts/optimistic-ethereum/OVM/execution/OVM_ExecutionManager.sol b/packages/contracts/contracts/optimistic-ethereum/OVM/execution/OVM_ExecutionManager.sol index 2e64913577b..206f2b3d62a 100644 --- a/packages/contracts/contracts/optimistic-ethereum/OVM/execution/OVM_ExecutionManager.sol +++ b/packages/contracts/contracts/optimistic-ethereum/OVM/execution/OVM_ExecutionManager.sol @@ -1900,7 +1900,7 @@ contract OVM_ExecutionManager is iOVM_ExecutionManager, Lib_AddressResolver { ) override external - onlyCallableBy(resolve("OVM_UpgradeExecutor")) + onlyCallableBy(0x420000000000000000000000000000000000000A) { _checkAccountLoad(_address); ovmStateManager.putAccountCode(_address, _code); @@ -1920,7 +1920,7 @@ contract OVM_ExecutionManager is iOVM_ExecutionManager, Lib_AddressResolver { ) override external - onlyCallableBy(resolve("OVM_UpgradeExecutor")) + onlyCallableBy(0x420000000000000000000000000000000000000A) { _putContractStorage( _address, diff --git a/packages/contracts/contracts/optimistic-ethereum/OVM/predeploys/OVM_UpgradeExecutor.sol b/packages/contracts/contracts/optimistic-ethereum/OVM/predeploys/OVM_UpgradeExecutor.sol index a6a96e1774a..f08d63d6642 100644 --- a/packages/contracts/contracts/optimistic-ethereum/OVM/predeploys/OVM_UpgradeExecutor.sol +++ b/packages/contracts/contracts/optimistic-ethereum/OVM/predeploys/OVM_UpgradeExecutor.sol @@ -16,9 +16,29 @@ import { Lib_ExecutionManagerWrapper } from "../../libraries/wrappers/Lib_Execut * Runtime target: OVM */ contract OVM_UpgradeExecutor { - fallback() + function setCode( + address _address, + bytes memory _code + ) external { - revert("Not yet implemented."); + Lib_ExecutionManagerWrapper.ovmSETCODE( + _address, + _code + ); + } + + function setStorage( + address _address, + bytes32 _key, + bytes32 _value + ) + external + { + Lib_ExecutionManagerWrapper.ovmSETSTORAGE( + _address, + _key, + _value + ); } } diff --git a/packages/contracts/contracts/optimistic-ethereum/libraries/wrappers/Lib_ExecutionManagerWrapper.sol b/packages/contracts/contracts/optimistic-ethereum/libraries/wrappers/Lib_ExecutionManagerWrapper.sol index 148ec3d24a3..f0e2337215f 100644 --- a/packages/contracts/contracts/optimistic-ethereum/libraries/wrappers/Lib_ExecutionManagerWrapper.sol +++ b/packages/contracts/contracts/optimistic-ethereum/libraries/wrappers/Lib_ExecutionManagerWrapper.sol @@ -140,7 +140,7 @@ library Lib_ExecutionManagerWrapper { /* * Performs a safe ovmSETCODE call. */ - function safeSETCODE( + function ovmSETCODE( address _address, bytes memory _code ) @@ -160,15 +160,17 @@ library Lib_ExecutionManagerWrapper { */ function ovmSETSTORAGE( address _address, - bytes memory _code + bytes32 _key, + bytes32 _value ) internal { _safeExecutionManagerInteraction( abi.encodeWithSignature( - "ovmSETSTORAGE(address,bytes)", + "ovmSETSTORAGE(address,bytes32,bytes32)", _address, - _code + _key, + _value ) ); } diff --git a/packages/contracts/src/contract-deployment/config.ts b/packages/contracts/src/contract-deployment/config.ts index 5e41caf859b..8eaf7f3076f 100644 --- a/packages/contracts/src/contract-deployment/config.ts +++ b/packages/contracts/src/contract-deployment/config.ts @@ -234,7 +234,7 @@ export const makeContractDeployConfig = async ( ], }, OVM_UpgradeExecutor: { - factory: getContractFactory('OVM_UpgradeExecutor'), + factory: getContractFactory('OVM_UpgradeExecutor', undefined, true), params: [], }, 'OVM_ChainStorageContainer:CTC:batches': { diff --git a/packages/contracts/src/contract-dumps.ts b/packages/contracts/src/contract-dumps.ts index a23e2d27f68..c42171f4cd9 100644 --- a/packages/contracts/src/contract-dumps.ts +++ b/packages/contracts/src/contract-dumps.ts @@ -152,7 +152,7 @@ export const makeStateDump = async (cfg: RollupDeployConfig): Promise => { 'OVM_ExecutionManager', 'OVM_StateManager', 'OVM_ETH', - 'OVM_Upgrader', + 'OVM_UpgradeExecutor', ], deployOverrides: {}, waitForReceipts: false, @@ -169,7 +169,7 @@ export const makeStateDump = async (cfg: RollupDeployConfig): Promise => { 'OVM_ETH', 'OVM_ECDSAContractAccount', 'OVM_ProxyEOA', - 'OVM_Upgrader', + 'OVM_UpgradeExecutor', ] const deploymentResult = await deploy(config) From 12d63cc1b2d58a9f57dbf7b609f73b0babbdbfbd Mon Sep 17 00:00:00 2001 From: Kelvin Fichter Date: Thu, 29 Apr 2021 13:44:52 -0400 Subject: [PATCH 5/5] fix: update gas estimates in integration tests --- integration-tests/test/native-eth.spec.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/integration-tests/test/native-eth.spec.ts b/integration-tests/test/native-eth.spec.ts index 96ec0fd5fe4..5be9e29bf1e 100644 --- a/integration-tests/test/native-eth.spec.ts +++ b/integration-tests/test/native-eth.spec.ts @@ -45,13 +45,13 @@ describe('Native ETH Integration Tests', async () => { const amount = utils.parseEther('0.5') const addr = '0x' + '1234'.repeat(10) const gas = await env.ovmEth.estimateGas.transfer(addr, amount) - expect(gas).to.be.deep.eq(BigNumber.from(213546)) + expect(gas).to.be.deep.eq(BigNumber.from(213545)) }) it('Should estimate gas for ETH withdraw', async () => { const amount = utils.parseEther('0.5') const gas = await env.ovmEth.estimateGas.withdraw(amount) - expect(gas).to.be.deep.eq(BigNumber.from(503789)) + expect(gas).to.be.deep.eq(BigNumber.from(503784)) }) })