More rigorous reverts for unsafe bytecode

contracts/optimistic-ethereum/OVM/execution/OVM_ExecutionManager.sol

@@ -955,10 +955,24 @@ contract OVM_ExecutionManager is iOVM_ExecutionManager, Lib_AddressResolver {
         // revert data as to retrieve execution metadata that would normally be reverted out of
         // existence.
 
-        (bool success, bytes memory returndata) =
-            _isCreate
-            ? _handleContractCreation(_gasLimit, _data, _contract)
-            : _contract.call{gas: _gasLimit}(_data);
+        bool success; 
+        bytes memory returndata;
+
+        if (_isCreate) {
+            // safeCREATE() is a function which replicates a CREATE message, but uses return values
+            // Which match that of CALL (i.e. bool, bytes).  This allows many security checks to be
+            // to be shared between untrusted call and create call frames.
+            (success, returndata) = address(this).call(
+                abi.encodeWithSelector(
+                    this.safeCREATE.selector,
+                    _gasLimit,
+                    _data,
+                    _contract
+                )
+            );
+        } else {
+            (success, returndata) = _contract.call{gas: _gasLimit}(_data);
+        }
 
         // Switch back to the original message context now that we're out of the call.
         _switchMessageContext(_nextMessageContext, prevMessageContext);
@@ -1026,47 +1040,42 @@ contract OVM_ExecutionManager is iOVM_ExecutionManager, Lib_AddressResolver {
 
     /**
      * Handles the creation-specific safety measures required for OVM contract deployment.
-     * This function sanitizes the return types for creation messages to match calls (bool, bytes).
+     * This function sanitizes the return types for creation messages to match calls (bool, bytes),
+     * by being an external function which the EM can call, that mimics the success/fail case of the CREATE.
      * This allows for consistent handling of both types of messages in _handleExternalMessage().
+     * Having this step occur as a separate call frame also allows us to easily revert the 
+     * contract deployment in the event that the code is unsafe.
      * 
      * @param _gasLimit Amount of gas to be passed into this creation.
      * @param _creationCode Code to pass into CREATE for deployment.
      * @param _address OVM address being deployed to.
-     * @return Whether or not the call succeeded.
-     * @return If creation fails: revert data. Otherwise: empty.
      */
-    function _handleContractCreation(
+    function safeCREATE(
         uint _gasLimit,
         bytes memory _creationCode,
         address _address
     )
-        internal
-        returns(
-            bool,
-            bytes memory
-        )
+        external
     {
+        // The only way this should be callable is from within _createContract().
+        if (msg.sender != address(this)) {
+            return;
+        }
         // Check that there is not already code at this address.
         if (_hasEmptyAccount(_address) == false) {
             // Note: in the EVM, this case burns all allotted gas.  For improved
-            // developer experience, we do return the remaining ones.
-            return (
-                false,
-                _encodeRevertData(
-                    RevertFlag.CREATE_COLLISION,
-                    Lib_ErrorUtils.encodeRevertString("A contract has already been deployed to this address")
-                )
+            // developer experience, we do return the remaining gas.
+            _revertWithFlag(
+                RevertFlag.CREATE_COLLISION,
+                Lib_ErrorUtils.encodeRevertString("A contract has already been deployed to this address")
             );
         }
 
         // Check the creation bytecode against the OVM_SafetyChecker.
         if (ovmSafetyChecker.isBytecodeSafe(_creationCode) == false) {
-            return (
-                false,
-                _encodeRevertData(
-                    RevertFlag.UNSAFE_BYTECODE,
-                    Lib_ErrorUtils.encodeRevertString("Contract creation code contains unsafe opcodes. Did you use the right compiler or pass an unsafe constructor argument?")
-                )
+            _revertWithFlag(
+                RevertFlag.UNSAFE_BYTECODE,
+                Lib_ErrorUtils.encodeRevertString("Contract creation code contains unsafe opcodes. Did you use the right compiler or pass an unsafe constructor argument?")
             );
         }
 
@@ -1078,31 +1087,20 @@ contract OVM_ExecutionManager is iOVM_ExecutionManager, Lib_AddressResolver {
 
         if (ethAddress == address(0)) {
             // If the creation fails, the EVM lets us grab its revert data. This may contain a revert flag
-            // to be used above in _handleExternalMessage.
-            uint256 revertDataSize;
-            assembly { revertDataSize := returndatasize() }
-            bytes memory revertdata = new bytes(revertDataSize);
-            assembly {
-                returndatacopy(
-                    add(revertdata, 0x20),
-                    0,
-                    revertDataSize
-                )
+            // to be used above in _handleExternalMessage, so we pass the revert data back up unmodified.
+            assembly { 
+                returndatacopy(0,0,returndatasize())
+                revert(0, returndatasize())
             }
-            // Return that the creation failed, and the data it reverted with.
-            return (false, revertdata);
         }
 
         // Again simply checking that the deployed code is safe too. Contracts can generate
         // arbitrary deployment code, so there's no easy way to analyze this beforehand.
         bytes memory deployedCode = Lib_EthUtils.getCode(ethAddress);
         if (ovmSafetyChecker.isBytecodeSafe(deployedCode) == false) {
-            return (
-                false,
-                _encodeRevertData(
-                    RevertFlag.UNSAFE_BYTECODE,
-                    Lib_ErrorUtils.encodeRevertString("Constructor attempted to deploy unsafe bytecode.")
-                )
+            _revertWithFlag(
+                RevertFlag.UNSAFE_BYTECODE,
+                Lib_ErrorUtils.encodeRevertString("Constructor attempted to deploy unsafe bytecode.")
             );
         }
 
@@ -1113,9 +1111,6 @@ contract OVM_ExecutionManager is iOVM_ExecutionManager, Lib_AddressResolver {
             ethAddress,
             Lib_EthUtils.getCodeHash(ethAddress)
         );
-
-        // Successful deployments will not give access to returndata, in both the EVM and the OVM.
-        return (true, hex'');
     }
 
     /******************************************

package.json

@@ -52,7 +52,7 @@
   "devDependencies": {
     "@eth-optimism/dev": "^1.1.1",
     "@eth-optimism/plugins": "^1.0.0-alpha.2",
-    "@eth-optimism/smock": "^1.0.0-alpha.3",
+    "@eth-optimism/smock": "1.0.0-alpha.6",
     "@nomiclabs/hardhat-ethers": "^2.0.1",
     "@nomiclabs/hardhat-waffle": "^2.0.1",
     "@typechain/ethers-v5": "1.0.0",

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: 4531286,
+            nuisanceGasLeft: 4199829,
           },
         },
       },

test/contracts/OVM/verification/OVM_BondManager.spec.ts

@@ -212,7 +212,7 @@ describe('OVM_BondManager', () => {
         // smodify the canClaim value to true to test claiming
         const block = await provider.getBlock('latest')
         timestamp = block.timestamp
-        bondManager.smodify.set({
+        await bondManager.smodify.put({
           witnessProviders: {
             [preStateRoot]: {
               canClaim: true,

test/contracts/OVM/verification/OVM_StateTransitioner.spec.ts

@@ -312,7 +312,7 @@ describe('OVM_StateTransitioner', () => {
         )
       )
 
-      OVM_StateTransitioner.smodify.set({
+      OVM_StateTransitioner.smodify.put({
         phase: 0,
         transactionHash,
       })
@@ -329,7 +329,7 @@ describe('OVM_StateTransitioner', () => {
 
   describe('commitContractState', () => {
     beforeEach(async () => {
-      OVM_StateTransitioner.smodify.set({
+      OVM_StateTransitioner.smodify.put({
         phase: 1,
       })
     })
@@ -414,7 +414,7 @@ describe('OVM_StateTransitioner', () => {
 
   describe('commitStorageSlot', () => {
     beforeEach(() => {
-      OVM_StateTransitioner.smodify.set({
+      OVM_StateTransitioner.smodify.put({
         phase: 1,
       })
     })
@@ -530,7 +530,7 @@ describe('OVM_StateTransitioner', () => {
 
   describe('completeTransition', () => {
     beforeEach(() => {
-      OVM_StateTransitioner.smodify.set({
+      OVM_StateTransitioner.smodify.put({
         phase: 1,
       })
     })

test/helpers/constants.ts

@@ -4,6 +4,9 @@ import { defaultAccounts } from 'ethereum-waffle'
 import { fromHexString, toHexString } from '@eth-optimism/core-utils'
 import xor from 'buffer-xor'
 
+/* Internal Imports */
+import { getContractDefinition } from '../../src'
+
 export const DEFAULT_ACCOUNTS = defaultAccounts
 export const DEFAULT_ACCOUNTS_HARDHAT = defaultAccounts.map((account) => {
   return {
@@ -35,8 +38,16 @@ export const NUISANCE_GAS_COSTS = {
   MIN_GAS_FOR_INVALID_STATE_ACCESS: 30000,
 }
 
-// TODO: get this exported/imported somehow in a way that we can do math on it.  unfortunately using require('.....artifacts/contract.json') is erroring...
-export const Helper_TestRunner_BYTELEN = 3654
+let len
+// This is hacky, but `hardhat compile` evaluates this file for some reason.
+// Feels better to have something hacky then a constant we have to keep re-hardcoding.
+try {
+  len = fromHexString(
+    getContractDefinition('Helper_TestRunner').deployedBytecode
+  ).byteLength
+} catch {}
+
+export const Helper_TestRunner_BYTELEN = len
 
 export const STORAGE_XOR =
   '0xfeedfacecafebeeffeedfacecafebeeffeedfacecafebeeffeedfacecafebeef'

test/helpers/test-runner/test-runner.ts

@@ -116,8 +116,8 @@ export class ExecutionManagerTestRunner {
           replacedParameter = this.setPlaceholderStrings(parameter)
         })
 
-        beforeEach(() => {
-          this.contracts.OVM_StateManager.smodify.set({
+        beforeEach(async () => {
+          await this.contracts.OVM_StateManager.smodify.put({
             accounts: {
               [this.contracts.Helper_TestRunner.address]: {
                 nonce: 0,
@@ -128,11 +128,11 @@ export class ExecutionManagerTestRunner {
           })
         })
 
-        beforeEach(() => {
-          this.contracts.OVM_ExecutionManager.smodify.set(
+        beforeEach(async () => {
+          await this.contracts.OVM_ExecutionManager.smodify.put(
             replacedTest.preState.ExecutionManager
           )
-          this.contracts.OVM_StateManager.smodify.set(
+          await this.contracts.OVM_StateManager.smodify.put(
             replacedTest.preState.StateManager
           )
         })

yarn.lock

@@ -78,10 +78,10 @@
   dependencies:
     node-fetch "^2.6.1"
 
-"@eth-optimism/smock@^1.0.0-alpha.3":
-  version "1.0.0-alpha.3"
-  resolved "https://registry.yarnpkg.com/@eth-optimism/smock/-/smock-1.0.0-alpha.3.tgz#5f3e8f137407c4c62f06aed60bac3dc282632f89"
-  integrity sha512-TKqbmElCWQ0qM6qj8JajqOijZVKl47L/5v2NnEWBJERKZ6zkuFxT0Y8HtUCM3r4ZEURuXFbRxRLP/ZTrOG6axg==
+"@eth-optimism/smock@1.0.0-alpha.6":
+  version "1.0.0-alpha.6"
+  resolved "https://registry.yarnpkg.com/@eth-optimism/smock/-/smock-1.0.0-alpha.6.tgz#1206eb33344663a28c982dd1d046ff756705ed09"
+  integrity sha512-Hn3j2iQngX5DNmwVopGsYOvitHnvY+VUFQzS8v/Ghf2lKEih5a2TYEbe7inZ5AG3U8z5iXcxqR7OzXpBWgTqXg==
   dependencies:
     "@eth-optimism/core-utils" "^0.1.10"
     "@ethersproject/abi" "^5.0.13"