More rigorous reverts for unsafe bytecode (#369)

* basic functionality

* update smock, fix broken smod check

* address PR feedback

* fix final tests, lock plugin version

* linting

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

@@ -963,10 +963,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);
@@ -1034,83 +1048,69 @@ 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 callable is from within _createContract(),
+        // and it should DEFINITELY not be callable by a non-EM code contract.
+        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?")
             );
         }
 
         // We always need to initialize the contract with the default account values.
         _initPendingAccount(_address);
 
-        // Actually execute the EVM create message,
+        // Actually execute the EVM create message.
+        // NOTE: The inline assembly below means we can NOT make any evm calls between here and then.
         address ethAddress = Lib_EthUtils.createContract(_creationCode);
 
         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.")
             );
         }
 
@@ -1121,9 +1121,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'');
     }
 
     /******************************************

packages/contracts/package.json

@@ -51,8 +51,8 @@
   },
   "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/plugins": "1.0.0-alpha.2",
+    "@eth-optimism/smock": "1.0.0-alpha.6",
     "@ethersproject/abstract-signer": "^5.0.14",
     "@nomiclabs/hardhat-ethers": "^2.0.1",
     "@nomiclabs/hardhat-waffle": "^2.0.1",

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

packages/contracts/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,

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

@@ -312,7 +312,7 @@ describe('OVM_StateTransitioner', () => {
         )
       )
 
-      OVM_StateTransitioner.smodify.set({
+      await OVM_StateTransitioner.smodify.put({
         phase: 0,
         transactionHash,
       })
@@ -329,7 +329,7 @@ describe('OVM_StateTransitioner', () => {
 
   describe('commitContractState', () => {
     beforeEach(async () => {
-      OVM_StateTransitioner.smodify.set({
+      await OVM_StateTransitioner.smodify.put({
         phase: 1,
       })
     })
@@ -394,7 +394,7 @@ describe('OVM_StateTransitioner', () => {
           proof = test.accountTrieWitness
           postStateRoot = test.newAccountTrieRoot
 
-          OVM_StateTransitioner.smodify.put({
+          await OVM_StateTransitioner.smodify.put({
             postStateRoot: test.accountTrieRoot,
           })
         })
@@ -413,8 +413,8 @@ describe('OVM_StateTransitioner', () => {
   })
 
   describe('commitStorageSlot', () => {
-    beforeEach(() => {
-      OVM_StateTransitioner.smodify.set({
+    beforeEach(async () => {
+      await OVM_StateTransitioner.smodify.put({
         phase: 1,
       })
     })
@@ -510,7 +510,7 @@ describe('OVM_StateTransitioner', () => {
 
           storageTrieProof = storageTest.proof
 
-          OVM_StateTransitioner.smodify.put({
+          await OVM_StateTransitioner.smodify.put({
             postStateRoot: test.accountTrieRoot,
           })
         })
@@ -529,8 +529,8 @@ describe('OVM_StateTransitioner', () => {
   })
 
   describe('completeTransition', () => {
-    beforeEach(() => {
-      OVM_StateTransitioner.smodify.set({
+    beforeEach(async () => {
+      await OVM_StateTransitioner.smodify.put({
         phase: 1,
       })
     })

packages/contracts/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,17 @@ 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
+  /* tslint:disable:no-empty */
+} catch {}
+
+export const Helper_TestRunner_BYTELEN = len
 
 export const STORAGE_XOR =
   '0xfeedfacecafebeeffeedfacecafebeeffeedfacecafebeeffeedfacecafebeef'

packages/contracts/test/helpers/test-runner/test-runner.ts

@@ -121,8 +121,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,
@@ -133,11 +133,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
           )
         })