Add Safety Checker (#7)

* Made StateManager usage authenticated

* add safety checker contract

* update safety checker: blacklist revert, KALL + KOPY checks

* lint

* update testrunner: smock safety check, add EM params

* add safety checker constants python script

* set mock targetMessenger to internal
Co-authored-by: Kelvin Fichter <kelvinfichter@gmail.com>

packages/contracts/bin/gen_safety_checker_constants.py

+#!/usr/bin/env python3
+# pip3 install pyevmasm
+from pyevmasm import instruction_tables
+
+#print(instruction_tables.keys())
+
+def asm(x):
+  return [instruction_tables['istanbul'][i].opcode for i in x]
+
+push_opcodes = asm(["PUSH%d" % i for i in range(1,33)])
+stop_opcodes = asm(["STOP", "JUMP", "RETURN", "INVALID"])
+caller_opcodes = asm(["CALLER"])
+blacklist_ops = set([
+  "ADDRESS", "BALANCE", "BLOCKHASH",
+  "CALL", "CALLCODE", "CHAINID", "COINBASE",
+  "CREATE", "CREATE2", "DELEGATECALL", "DIFFICULTY",
+  "EXTCODESIZE", "EXTCODECOPY", "EXTCODEHASH",
+  "GASLIMIT", "GASPRICE", "NUMBER",
+  "ORIGIN", "REVERT", "SELFBALANCE", "SELFDESTRUCT",
+  "SLOAD", "SSTORE", "STATICCALL", "TIMESTAMP"])
+whitelist_opcodes = []
+for x in instruction_tables['istanbul']:
+  if x.name not in blacklist_ops:
+    whitelist_opcodes.append(x.opcode)
+
+pushmask = 0
+for x in push_opcodes:
+  pushmask |= 1 << x
+
+stopmask = 0
+for x in stop_opcodes:
+  stopmask |= 1 << x
+
+stoplist = [0]*256
+procmask = 0
+for i in range(256):
+  if i in whitelist_opcodes and \
+      i not in push_opcodes and \
+      i not in stop_opcodes and \
+      i not in caller_opcodes:
+    # can skip this opcode
+    stoplist[i] = 1
+  else:
+    procmask |= 1 << i
+
+# PUSH1 through PUSH4, can't skip in slow
+for i in range(0x60, 0x64):
+  stoplist[i] = i-0x5e
+rr = "uint256[8] memory opcodeSkippableBytes = [\n"
+for i in range(0, 0x100, 0x20):
+  ret = "uint256(0x"
+  for j in range(i, i+0x20, 1):
+    ret += ("%02X" % stoplist[j])
+  rr += ret+"),\n"
+  
+rr = rr[:-2] + "];"
+
+print(rr)
+print("// Mask to gate opcode specific cases")
+print("uint256 opcodeGateMask = ~uint256(0x%x);" % procmask)
+print("// Halting opcodes")
+print("uint256 opcodeHaltingMask = ~uint256(0x%x);" % stopmask)
+print("// PUSH opcodes")
+print("uint256 opcodePushMask = ~uint256(0x%x);" % pushmask)
+

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

@@ -27,8 +27,8 @@ contract OVM_ExecutionManager is iOVM_ExecutionManager, Lib_AddressResolver {
      * External Contract References *
      ********************************/
 
-    iOVM_SafetyChecker public ovmSafetyChecker;
-    iOVM_StateManager public ovmStateManager;
+    iOVM_SafetyChecker internal ovmSafetyChecker;
+    iOVM_StateManager internal ovmStateManager;
 
 
     /*******************************
@@ -124,10 +124,18 @@ contract OVM_ExecutionManager is iOVM_ExecutionManager, Lib_AddressResolver {
         override
         public
     {
-        // Store our OVM_StateManager instance (significantly easier than attempting to pass the address
-        // around in calldata).
+        // Store our OVM_StateManager instance (significantly easier than attempting to pass the
+        // address around in calldata).
         ovmStateManager = iOVM_StateManager(_ovmStateManager);
 
+        // Make sure this function can't be called by anyone except the owner of the
+        // OVM_StateManager (expected to be an OVM_StateTransitioner). We can revert here because
+        // this would make the `run` itself invalid.
+        require(
+            msg.sender == ovmStateManager.owner(),
+            "Only the owner of the ovmStateManager can call this function"
+        );
+
         // Check whether we need to start a new epoch, do so if necessary.
         _checkNeedsNewEpoch(_transaction.timestamp);
 
@@ -154,6 +162,9 @@ contract OVM_ExecutionManager is iOVM_ExecutionManager, Lib_AddressResolver {
 
         // Wipe the execution context.
         _resetContext();
+
+        // Reset the ovmStateManager.
+        ovmStateManager = iOVM_StateManager(address(0));
     }
 
 

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

@@ -12,22 +12,124 @@ contract OVM_SafetyChecker is iOVM_SafetyChecker {
     /********************
      * Public Functions *
      ********************/
-
     /**
-     * Checks that a given bytecode string is considered safe.
-     * @param _bytecode Bytecode string to check.
-     * @return _safe Whether or not the bytecode is safe.
+     * Returns whether or not all of the provided bytecode is safe.
+     * @param _bytecode The bytecode to safety check.
+     * @return `true` if the bytecode is safe, `false` otherwise.
      */
     function isBytecodeSafe(
         bytes memory _bytecode
     )
         override
-        public
+        external
         view
-        returns (
-            bool _safe
-        )
+        returns (bool)
     {
+        // autogenerated by gen_safety_checker_constants.py
+        // number of bytes to skip for each opcode
+        uint256[8] memory opcodeSkippableBytes = [
+            uint256(0x0001010101010101010101010000000001010101010101010101010101010000),
+            uint256(0x0100000000000000000000000000000000000000010101010101000000010100),
+            uint256(0x0000000000000000000000000000000001010101000000010101010100000000),
+            uint256(0x0203040500000000000000000000000000000000000000000000000000000000),
+            uint256(0x0101010101010101010101010101010101010101010101010101010101010101),
+            uint256(0x0101010101000000000000000000000000000000000000000000000000000000),
+            uint256(0x0000000000000000000000000000000000000000000000000000000000000000),
+            uint256(0x0000000000000000000000000000000000000000000000000000000000000000)
+        ];
+        // Mask to gate opcode specific cases
+        uint256 opcodeGateMask = ~uint256(0xffffffffffffffffffffffe000000000fffffffff070ffff9c0ffffec000f001);
+        // Halting opcodes
+        uint256 opcodeHaltingMask = ~uint256(0x4008000000000000000000000000000000000000004000000000000000000001);
+        // PUSH opcodes
+        uint256 opcodePushMask = ~uint256(0xffffffff000000000000000000000000);
+
+        uint256 codeLength;
+        uint256 _pc;
+        assembly {
+            _pc := add(_bytecode, 0x20)
+        }
+        codeLength = _pc + _bytecode.length;
+        do {
+            // current opcode: 0x00...0xff
+            uint256 opNum;
+
+            // inline assembly removes the extra add + bounds check
+            assembly {
+                let word := mload(_pc) //load the next 32 bytes at pc into word
+
+                // Look up number of bytes to skip from opcodeSkippableBytes and then update indexInWord
+                // E.g. the 02030405 in opcodeSkippableBytes is the number of bytes to skip for PUSH1->4
+                // We repeat this 6 times, thus we can only skip bytes for up to PUSH4 ((1+4) * 6 = 30 < 32).
+                // If we see an opcode that is listed as 0 skippable bytes e.g. PUSH5,
+                // then we will get stuck on that indexInWord and then opNum will be set to the PUSH5 opcode.
+                let indexInWord := byte(0, mload(add(opcodeSkippableBytes, byte(0, word))))
+                indexInWord := add(indexInWord, byte(0, mload(add(opcodeSkippableBytes, byte(indexInWord, word)))))
+                indexInWord := add(indexInWord, byte(0, mload(add(opcodeSkippableBytes, byte(indexInWord, word)))))
+                indexInWord := add(indexInWord, byte(0, mload(add(opcodeSkippableBytes, byte(indexInWord, word)))))
+                indexInWord := add(indexInWord, byte(0, mload(add(opcodeSkippableBytes, byte(indexInWord, word)))))
+                indexInWord := add(indexInWord, byte(0, mload(add(opcodeSkippableBytes, byte(indexInWord, word)))))
+                _pc := add(_pc, indexInWord)
+
+                opNum := byte(indexInWord, word)
+            }
+
+            // + push opcodes
+            // + stop opcodes [STOP(0x00),JUMP(0x56),RETURN(0xf3),INVALID(0xfe)]
+            // + caller opcode CALLER(0x33)
+            // + blacklisted opcodes
+            uint256 opBit = 1 << opNum;
+            if (opBit & opcodeGateMask == 0) {
+                if (opBit & opcodePushMask == 0) {
+                    // all pushes are valid opcodes
+                    // subsequent bytes are not opcodes. Skip them.
+                    _pc += (opNum - 0x5e); // PUSH1 is 0x60, so opNum-0x5f = PUSHed bytes and we +1 to
+                    // skip the _pc++; line below in order to save gas ((-0x5f + 1) = -0x5e)
+                    continue;
+                } else if (opBit & opcodeHaltingMask == 0) {
+                    // STOP or JUMP or RETURN or INVALID (Note: REVERT is blacklisted, so not included here)
+                    // We are now inside unreachable code until we hit a JUMPDEST!
+                    do {
+                        _pc++;
+                        assembly {
+                            opNum := byte(0, mload(_pc))
+                        }
+                        // encountered a JUMPDEST
+                        if (opNum == 0x5b) break;
+                        // skip PUSHed bytes
+                        if ((1 << opNum) & opcodePushMask == 0) _pc += (opNum - 0x5f); // opNum-0x5f = PUSHed bytes (PUSH1 is 0x60)
+                    } while (_pc < codeLength);
+                    // opNum is 0x5b, so we don't continue here since the pc++ is fine
+                } else if (opNum == 0x33) { // Caller opcode
+                    uint256 firstOps; // next 32 bytes of bytecode
+                    uint256 secondOps; // following 32 bytes of bytecode
+
+                    assembly {
+                        firstOps := mload(_pc)
+                        // 32 - 4 bytes = 28 bytes = 224 bits
+                        secondOps := shr(224, mload(add(_pc, 0x20)))
+                    }
+
+                    // Call identity precompile
+                    // CALLER POP PUSH1 0x00 PUSH1 0x04 GAS CALL
+                    // 32 - 8 bytes = 24 bytes = 192
+                    if ((firstOps >> 192) == 0x3350600060045af1) {
+                        _pc += 8;
+                    // Call EM and abort execution if instructed
+                    // CALLER PUSH1 0x00 SWAP1 GAS CALL PC PUSH1 0x1d ADD EQ JUMPI RETURNDATASIZE PUSH1 0x00 DUP1 RETURNDATACOPY PUSH1 0x00 REVERT JUMPDEST PUSH1 0x01 PUSH1 0x00 RETURN JUMPDEST
+                    } else if (firstOps == 0x336000905af158601d0157586012013d600114573d6000803e3d6000fd5b6001 && secondOps == 0x6000f35b) {
+                        _pc += 36;
+                    } else {
+                        return false;
+                    }
+                    continue;
+                } else {
+                    // encountered a non-whitelisted opcode!
+                    return false;
+                }
+            }
+            _pc++;
+        } while (_pc < codeLength);
         return true;
     }
 }

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

@@ -25,8 +25,8 @@ contract OVM_StateManager is iOVM_StateManager {
      * Contract Variables: Contract References *
      *******************************************/
 
-    address public owner;
-    address public ovmExecutionManager;
+    address override public owner;
+    address override public ovmExecutionManager;
 
 
     /****************************************

packages/contracts/contracts/optimistic-ethereum/OVM/verification/OVM_StateTransitioner.sol

@@ -304,6 +304,7 @@ contract OVM_StateTransitioner is iOVM_StateTransitioner, Lib_AddressResolver {
     )
         override
         public
+        onlyDuringPhase(TransitionPhase.PRE_EXECUTION)
     {
         require(
             Lib_OVMCodec.hashTransaction(_transaction) == transactionHash,

packages/contracts/contracts/optimistic-ethereum/iOVM/execution/iOVM_SafetyChecker.sol

@@ -10,5 +10,5 @@ interface iOVM_SafetyChecker {
      * Public Functions *
      ********************/
 
-    function isBytecodeSafe(bytes memory _bytecode) external view returns (bool _safe);
+    function isBytecodeSafe(bytes memory _bytecode) external view returns (bool);
 }

packages/contracts/contracts/optimistic-ethereum/iOVM/execution/iOVM_StateManager.sol

@@ -25,7 +25,9 @@ interface iOVM_StateManager {
     /***************************
      * Public Functions: Setup *
      ***************************/
-    
+
+    function owner() external view returns (address _owner); 
+    function ovmExecutionManager() external view returns (address _ovmExecutionManager); 
     function setExecutionManager(address _ovmExecutionManager) external;
 
 

packages/contracts/contracts/optimistic-ethereum/libraries/resolver/Lib_AddressManager.sol

 // SPDX-License-Identifier: UNLICENSED
 pragma solidity ^0.7.0;
 
+/* Contract Imports */
+import { Ownable } from "./Lib_Ownable.sol";
+
 /**
  * @title Lib_AddressManager
  */
-contract Lib_AddressManager {
+contract Lib_AddressManager is Ownable {
 
     /*******************************************
      * Contract Variables: Internal Accounting *
@@ -22,6 +25,7 @@ contract Lib_AddressManager {
         address _address
     )
         public
+        onlyOwner
     {
         addresses[_getNameHash(_name)] = _address;
     }

packages/contracts/contracts/optimistic-ethereum/libraries/resolver/Lib_Ownable.sol

+// SPDX-License-Identifier: MIT
+pragma solidity ^0.7.0;
+
+/**
+ * @title Ownable
+ * @dev Adapted from https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/access/Ownable.sol
+ */
+abstract contract Ownable {
+
+    /*************
+     * Variables *
+     *************/
+
+    address public owner;
+
+
+    /**********
+     * Events *
+     **********/
+
+    event OwnershipTransferred(
+        address indexed previousOwner,
+        address indexed newOwner
+    );
+
+
+    /***************
+     * Constructor *
+     ***************/
+
+    constructor()
+        internal
+    {
+        owner = msg.sender;
+        emit OwnershipTransferred(address(0), owner);
+    }
+
+
+    /**********************
+     * Function Modifiers *
+     **********************/
+
+    modifier onlyOwner() {
+        require(
+            owner == msg.sender,
+            "Ownable: caller is not the owner"
+        );
+        _;
+    }
+
+
+    /********************
+     * Public Functions *
+     ********************/
+
+    function renounceOwnership()
+        public
+        virtual
+        onlyOwner 
+    {
+        emit OwnershipTransferred(owner, address(0));
+        owner = address(0);
+    }
+
+    function transferOwnership(address _newOwner)
+        public
+        virtual
+        onlyOwner
+    {
+        require(
+            _newOwner != address(0),
+            "Ownable: new owner cannot be the zero address"
+        );
+
+        emit OwnershipTransferred(owner, _newOwner);
+        owner = _newOwner;
+    }
+}

packages/contracts/contracts/optimistic-ethereum/mockOVM/bridge/mockOVM_CrossDomainMessenger.sol

@@ -29,7 +29,7 @@ contract mockOVM_CrossDomainMessenger is OVM_BaseCrossDomainMessenger {
      **********************/
 
     ReceivedMessage[] internal fullReceivedMessages;
-    address public targetMessengerAddress;
+    address internal targetMessengerAddress;
     uint256 internal lastRelayedMessage;
     uint256 internal delay;
 

packages/contracts/test/contracts/OVM/execution/OVM_SafetyChecker.spec.ts

@@ -17,7 +17,7 @@ describe('OVM_SafetyChecker', () => {
     OVM_SafetyChecker = await Factory__OVM_SafetyChecker.deploy()
   })
 
-  describe.skip('isBytecodeSafe()', () => {
+  describe('isBytecodeSafe()', () => {
     for (const testName of Object.keys(SAFETY_CHECKER_TEST_JSON)) {
       const test = SAFETY_CHECKER_TEST_JSON[testName]
       it(`should correctly classify: ${testName}`, async () => {

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

@@ -4,7 +4,7 @@ import { expect } from '../../setup'
 import { ethers } from '@nomiclabs/buidler'
 import { Contract, BigNumber, ContractFactory } from 'ethers'
 import { cloneDeep, merge } from 'lodash'
-import { smoddit, ModifiableContract } from '@eth-optimism/smock'
+import { smoddit, smockit, ModifiableContract } from '@eth-optimism/smock'
 
 /* Internal Imports */
 import {
@@ -148,10 +148,15 @@ export class ExecutionManagerTestRunner {
       await ethers.getContractFactory('Lib_AddressManager')
     ).deploy()
 
-    this.contracts.OVM_SafetyChecker = await (
+    const SafetyChecker = await (
       await ethers.getContractFactory('OVM_SafetyChecker')
     ).deploy()
 
+    const MockSafetyChecker = smockit(SafetyChecker)
+    MockSafetyChecker.smocked.isBytecodeSafe.will.return.with(true)
+
+    this.contracts.OVM_SafetyChecker = MockSafetyChecker
+
     await AddressManager.setAddress(
       'OVM_SafetyChecker',
       this.contracts.OVM_SafetyChecker.address
@@ -174,7 +179,10 @@ export class ExecutionManagerTestRunner {
 
     this.contracts.OVM_StateManager = await (
       await smoddit('OVM_StateManager')
-    ).deploy(this.contracts.OVM_ExecutionManager.address)
+    ).deploy(await this.contracts.OVM_ExecutionManager.signer.getAddress())
+    await this.contracts.OVM_StateManager.setExecutionManager(
+      this.contracts.OVM_ExecutionManager.address
+    )
 
     this.contracts.Helper_TestRunner = await (
       await ethers.getContractFactory('Helper_TestRunner')