Merge pull request #5 from ethereum-optimism/audit/tob-message-passing

Add timestamp and auth to message passing contracts

packages/contracts/bin/take-dump.ts

@@ -4,7 +4,7 @@ import * as path from 'path'
 import * as mkdirp from 'mkdirp'
 
 /* Internal Imports */
-import { makeStateDump } from '../src'
+import { makeStateDump } from '../src/contract-dumps'
 ;(async () => {
   const outdir = path.resolve(__dirname, '../build/dumps')
   const outfile = path.join(outdir, 'state-dump.latest.json')

packages/contracts/contracts/optimistic-ethereum/OVM/bridge/OVM_BaseCrossDomainMessenger.sol

@@ -16,7 +16,6 @@ contract OVM_BaseCrossDomainMessenger is iOVM_BaseCrossDomainMessenger {
 
     mapping (bytes32 => bool) public receivedMessages;
     mapping (bytes32 => bool) public sentMessages;
-    address public targetMessengerAddress;
     uint256 public messageNonce;
     address public xDomainMessageSender;
 
@@ -25,22 +24,6 @@ contract OVM_BaseCrossDomainMessenger is iOVM_BaseCrossDomainMessenger {
      * Public Functions *
      ********************/
 
-    /**
-     * Sets the target messenger address.
-     * @dev Currently, this function is public and therefore allows anyone to modify the target
-     *      messenger for a given xdomain messenger contract. Obviously this shouldn't be allowed,
-     *      but we still need to determine an adequate mechanism for updating this address.
-     * @param _targetMessengerAddress New messenger address.
-     */
-    function setTargetMessengerAddress(
-        address _targetMessengerAddress
-    )
-        override
-        public
-    {
-        targetMessengerAddress = _targetMessengerAddress;
-    }
-
     /**
      * Sends a cross domain message to the target messenger.
      * @param _target Target contract address.
@@ -92,8 +75,8 @@ contract OVM_BaseCrossDomainMessenger is iOVM_BaseCrossDomainMessenger {
             bytes memory
         )
     {
-        return abi.encodeWithSelector(
-            bytes4(keccak256(bytes("relayMessage(address,address,bytes,uint256)"))),
+        return abi.encodeWithSignature(
+            "relayMessage(address,address,bytes,uint256)",
             _target,
             _sender,
             _message,

packages/contracts/contracts/optimistic-ethereum/OVM/bridge/OVM_L1CrossDomainMessenger.sol

@@ -163,15 +163,13 @@ contract OVM_L1CrossDomainMessenger is iOVM_L1CrossDomainMessenger, OVM_BaseCros
             bool
         )
     {
-        // TODO: We *must* verify that the batch timestamp is sufficiently old.
-        // However, this requires that we first add timestamps to state batches
-        // and account for that change in various tests. Change of that size is
-        // out of scope for this ticket, so "TODO" for now.
-
-        return ovmStateCommitmentChain.verifyElement(
-            abi.encodePacked(_proof.stateRoot),
-            _proof.stateRootBatchHeader,
-            _proof.stateRootProof
+        return (
+            ovmStateCommitmentChain.insideFraudProofWindow(_proof.stateRootBatchHeader) == false
+            && ovmStateCommitmentChain.verifyElement(
+                abi.encodePacked(_proof.stateRoot),
+                _proof.stateRootBatchHeader,
+                _proof.stateRootProof
+            )
         );
     }
 
@@ -236,7 +234,7 @@ contract OVM_L1CrossDomainMessenger is iOVM_L1CrossDomainMessenger, OVM_BaseCros
         internal
     {
         ovmL1ToL2TransactionQueue.enqueue(
-            targetMessengerAddress,
+            resolve("OVM_L2CrossDomainMessenger"),
             _gasLimit,
             _message
         );

packages/contracts/contracts/optimistic-ethereum/OVM/bridge/OVM_L2CrossDomainMessenger.sol

@@ -104,7 +104,7 @@ contract OVM_L2CrossDomainMessenger is iOVM_L2CrossDomainMessenger, OVM_BaseCros
         )
     {
         return (
-            ovmL1MessageSender.getL1MessageSender() == targetMessengerAddress
+            ovmL1MessageSender.getL1MessageSender() == resolve("OVM_L1CrossDomainMessenger")
         );
     }
 

packages/contracts/contracts/optimistic-ethereum/OVM/chain/OVM_BaseChain.sol

@@ -135,6 +135,7 @@ contract OVM_BaseChain is iOVM_BaseChain {
             batchRoot: Lib_MerkleUtils.getMerkleRoot(_elements),
             batchSize: _elements.length,
             prevTotalElements: totalElements,
+            timestamp: block.timestamp,
             extraData: _extraData
         });
 
@@ -202,6 +203,7 @@ contract OVM_BaseChain is iOVM_BaseChain {
             _batchHeader.batchRoot,
             _batchHeader.batchSize,
             _batchHeader.prevTotalElements,
+            _batchHeader.timestamp,
             _batchHeader.extraData
         ));
     }

packages/contracts/contracts/optimistic-ethereum/OVM/chain/OVM_CanonicalTransactionChain.sol

@@ -143,8 +143,8 @@ contract OVM_CanonicalTransactionChain is iOVM_CanonicalTransactionChain, OVM_Ba
             batchRoot: _queueElement.batchRoot,
             batchSize: _batchSize,
             prevTotalElements: getTotalElements(),
+            timestamp: _queueElement.timestamp,
             extraData: abi.encodePacked(
-                _queueElement.timestamp,
                 _queueElement.isL1ToL2Batch
             )
         });

packages/contracts/contracts/optimistic-ethereum/OVM/chain/OVM_StateCommitmentChain.sol

@@ -18,6 +18,13 @@ import { OVM_BaseChain } from "./OVM_BaseChain.sol";
  * @title OVM_StateCommitmentChain
  */
 contract OVM_StateCommitmentChain is iOVM_StateCommitmentChain, OVM_BaseChain, Lib_AddressResolver {
+
+    /*************
+     * Constants *
+     *************/
+
+    uint256 constant public FRAUD_PROOF_WINDOW = 7 days;
+
     
     /*******************************************
      * Contract Variables: Contract References *
@@ -73,10 +80,7 @@ contract OVM_StateCommitmentChain is iOVM_StateCommitmentChain, OVM_BaseChain, L
             elements[i] = abi.encodePacked(_batch[i]);
         }
 
-        _appendBatch(
-            elements,
-            abi.encodePacked(block.timestamp)
-        );
+        _appendBatch(elements);
     }
 
     /**
@@ -94,6 +98,34 @@ contract OVM_StateCommitmentChain is iOVM_StateCommitmentChain, OVM_BaseChain, L
             "State batches can only be deleted by the OVM_FraudVerifier."
         );
 
+        require(
+            insideFraudProofWindow(_batchHeader),
+            "State batches can only be deleted within the fraud proof window."
+        );
+
         _deleteBatch(_batchHeader);
     }
+
+
+    /**********************************
+     * Public Functions: Batch Status *
+     **********************************/
+
+    function insideFraudProofWindow(
+        Lib_OVMCodec.ChainBatchHeader memory _batchHeader
+    )
+        override
+        public
+        view
+        returns (
+            bool _inside
+        )
+    {
+        require(
+            _batchHeader.timestamp != 0,
+            "Batch header timestamp cannot be zero"
+        );
+
+        return _batchHeader.timestamp + FRAUD_PROOF_WINDOW > block.timestamp;
+    }
 }

packages/contracts/contracts/optimistic-ethereum/iOVM/bridge/iOVM_BaseCrossDomainMessenger.sol

@@ -11,14 +11,6 @@ interface iOVM_BaseCrossDomainMessenger {
      * Public Functions *
      ********************/
 
-    /**
-     * Sets the target messenger address.
-     * @param _targetMessengerAddress New messenger address.
-     */
-    function setTargetMessengerAddress(
-        address _targetMessengerAddress
-    ) external;
-
     /**
      * Sends a cross domain message to the target messenger.
      * @param _target Target contract address.

packages/contracts/contracts/optimistic-ethereum/iOVM/chain/iOVM_StateCommitmentChain.sol

@@ -19,4 +19,10 @@ interface iOVM_StateCommitmentChain is iOVM_BaseChain {
 
     function appendStateBatch(bytes32[] calldata _batch) external;
     function deleteStateBatch(Lib_OVMCodec.ChainBatchHeader memory _batchHeader) external;
+
+    /**********************************
+     * Public Functions: Batch Status *
+     **********************************/
+
+    function insideFraudProofWindow(Lib_OVMCodec.ChainBatchHeader memory _batchHeader) external view returns (bool _inside);
 }

packages/contracts/contracts/optimistic-ethereum/libraries/codec/Lib_OVMCodec.sol

@@ -62,6 +62,7 @@ library Lib_OVMCodec {
         bytes32 batchRoot;
         uint256 batchSize;
         uint256 prevTotalElements;
+        uint256 timestamp;
         bytes extraData;
     }
 

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

@@ -29,6 +29,7 @@ contract mockOVM_CrossDomainMessenger is OVM_BaseCrossDomainMessenger {
      **********************/
 
     ReceivedMessage[] internal fullReceivedMessages;
+    address public targetMessengerAddress;
     uint256 internal lastRelayedMessage;
     uint256 internal delay;
 
@@ -51,6 +52,21 @@ contract mockOVM_CrossDomainMessenger is OVM_BaseCrossDomainMessenger {
      * Public Functions *
      ********************/
 
+    /**
+     * Sets the target messenger address.
+     * @dev Currently, this function is public and therefore allows anyone to modify the target
+     *      messenger for a given xdomain messenger contract. Obviously this shouldn't be allowed,
+     *      but we still need to determine an adequate mechanism for updating this address.
+     * @param _targetMessengerAddress New messenger address.
+     */
+    function setTargetMessengerAddress(
+        address _targetMessengerAddress
+    )
+        public
+    {
+        targetMessengerAddress = _targetMessengerAddress;
+    }
+
     /**
      * Sends a message to another mock xdomain messenger.
      * @param _target Target for the message.

packages/contracts/package.json

 {
   "name": "@eth-optimism/contracts",
-  "version": "0.0.1",
+  "version": "0.0.2-alpha.1",
   "main": "build/src/index.js",
   "files": [
     "build/**/*.js",
@@ -23,6 +23,9 @@
     "lint:fix:typescript": "prettier --config prettier-config.json --write \"buidler.config.ts\" \"{src,test}/**/*.ts\"",
     "clean": "rm -rf ./artifacts ./build ./cache"
   },
+  "dependencies": {
+    "ethers": "5.0.0"
+  },
   "devDependencies": {
     "@eth-optimism/smock": "^0.0.2",
     "@nomiclabs/buidler": "^1.4.4",
@@ -37,7 +40,6 @@
     "chai": "^4.2.0",
     "copyfiles": "^2.3.0",
     "ethereum-waffle": "3.0.0",
-    "ethers": "5.0.0",
     "fs-extra": "^9.0.1",
     "ganache-core": "^2.12.1",
     "lodash": "^4.17.20",

packages/contracts/src/contract-deployment/config.ts

@@ -89,7 +89,7 @@ export const makeContractDeployConfig = async (
       params: [
         AddressManager.address,
         config.ovmGasMeteringConfig,
-        config.ovmGlobalContext
+        config.ovmGlobalContext,
       ],
     },
     OVM_StateManager: {
@@ -116,10 +116,10 @@ export const makeContractDeployConfig = async (
       factory: getContractFactory('OVM_StateTransitionerFactory'),
     },
     OVM_ECDSAContractAccount: {
-      factory: getContractFactory('OVM_ECDSAContractAccount')
+      factory: getContractFactory('OVM_ECDSAContractAccount'),
     },
     mockOVM_ECDSAContractAccount: {
-      factory: getContractFactory('mockOVM_ECDSAContractAccount')
-    }
+      factory: getContractFactory('mockOVM_ECDSAContractAccount'),
+    },
   }
 }

packages/contracts/src/contract-deployment/deploy.ts

@@ -57,7 +57,9 @@ export const deploy = async (
     }
   }
 
-  for (const [name, contractDeployParameters] of Object.entries(contractDeployConfig)) {
+  for (const [name, contractDeployParameters] of Object.entries(
+    contractDeployConfig
+  )) {
     if (config.dependencies && !config.dependencies.includes(name)) {
       continue
     }

packages/contracts/src/contract-dumps.ts

@@ -67,9 +67,9 @@ const sanitizeStorageDump = (
     deadAddress: string
   }>
 ): StorageDump => {
-  for (let i = 0; i < accounts.length; i++) {
-    accounts[i].originalAddress = remove0x(accounts[i].originalAddress).toLowerCase()
-    accounts[i].deadAddress = remove0x(accounts[i].deadAddress).toLowerCase()
+  for (const account of accounts) {
+    account.originalAddress = remove0x(account.originalAddress).toLowerCase()
+    account.deadAddress = remove0x(account.deadAddress).toLowerCase()
   }
 
   for (const [key, value] of Object.entries(storageDump)) {
@@ -116,7 +116,7 @@ export const makeStateDump = async (): Promise<any> => {
       secondsPerEpoch: 600,
     },
     ovmGlobalContext: {
-      ovmCHAINID: 420
+      ovmCHAINID: 420,
     },
     transactionChainConfig: {
       sequencer: signer,
@@ -135,21 +135,24 @@ export const makeStateDump = async (): Promise<any> => {
       'OVM_SafetyChecker',
       'OVM_ExecutionManager',
       'OVM_StateManager',
-      'mockOVM_ECDSAContractAccount'
-    ]
+      'mockOVM_ECDSAContractAccount',
+    ],
   }
 
   const precompiles = {
     OVM_L2ToL1MessagePasser: '0x4200000000000000000000000000000000000000',
     OVM_L1MessageSender: '0x4200000000000000000000000000000000000001',
-    OVM_DeployerWhitelist: '0x4200000000000000000000000000000000000002'
+    OVM_DeployerWhitelist: '0x4200000000000000000000000000000000000002',
   }
 
   const deploymentResult = await deploy(config)
-  deploymentResult.contracts['Lib_AddressManager'] = deploymentResult.AddressManager
+  deploymentResult.contracts['Lib_AddressManager'] =
+    deploymentResult.AddressManager
 
   if (deploymentResult.failedDeployments.length > 0) {
-    throw new Error(`Could not generate state dump, deploy failed for: ${deploymentResult.failedDeployments}`)
+    throw new Error(
+      `Could not generate state dump, deploy failed for: ${deploymentResult.failedDeployments}`
+    )
   }
 
   const pStateManager = ganache.engine.manager.state.blockchain.vm.pStateManager
@@ -163,24 +166,28 @@ export const makeStateDump = async (): Promise<any> => {
     const name = Object.keys(deploymentResult.contracts)[i]
     const contract = deploymentResult.contracts[name]
 
-    const codeBuf = await pStateManager.getContractCode(fromHexString(contract.address))
+    const codeBuf = await pStateManager.getContractCode(
+      fromHexString(contract.address)
+    )
     const code = toHexString(codeBuf)
 
-    const deadAddress = precompiles[name] || `0xdeaddeaddeaddeaddeaddeaddeaddeaddead${i.toString(16).padStart(4, '0')}`
+    const deadAddress =
+      precompiles[name] ||
+      `0xdeaddeaddeaddeaddeaddeaddeaddeaddead${i.toString(16).padStart(4, '0')}`
 
     dump.accounts[name] = {
       address: deadAddress,
       code,
       codeHash: keccak256(code),
       storage: await getStorageDump(cStateManager, contract.address),
-      abi: getContractDefinition(name.replace('Proxy__', '')).abi
+      abi: getContractDefinition(name.replace('Proxy__', '')).abi,
     }
   }
 
   const addressMap = Object.keys(dump.accounts).map((name) => {
     return {
       originalAddress: deploymentResult.contracts[name].address,
-      deadAddress: dump.accounts[name].address
+      deadAddress: dump.accounts[name].address,
     }
   })
 

packages/contracts/src/index.ts

 export * from './contract-defs'
-export * from './contract-dumps'
+export { getLatestStateDump, StateDump } from './contract-dumps'
 export * from './contract-deployment'

packages/contracts/test/contracts/OVM/chain/OVM_StateCommitmentChain.spec.ts

@@ -106,6 +106,7 @@ describe('OVM_StateCommitmentChain', () => {
       batchRoot: keccak256(NON_NULL_BYTES32),
       batchSize: 1,
       prevTotalElements: 0,
+      timestamp: 0,
       extraData: '0x',
     }
 
@@ -114,7 +115,7 @@ describe('OVM_StateCommitmentChain', () => {
         batch.length
       )
       await OVM_StateCommitmentChain.appendStateBatch(batch)
-      batchHeader.extraData = toHexString32(await getEthTime(ethers.provider))
+      batchHeader.timestamp = await getEthTime(ethers.provider)
     })
 
     describe('when the sender is not the OVM_FraudVerifier', () => {

packages/contracts/test/contracts/OVM/execution/OVM_ExecutionManager/ovmCREATE.spec.ts

@@ -553,7 +553,7 @@ const test_ovmCREATE: TestDefinition = {
                     {
                       functionName: 'ovmCREATE',
                       functionParams: {
-                        bytecode: '0x'
+                        bytecode: '0x',
                       },
                       expectedReturnStatus: true,
                       expectedReturnValue: ZERO_ADDRESS,
@@ -561,12 +561,12 @@ const test_ovmCREATE: TestDefinition = {
                   ],
                 },
                 expectedReturnStatus: true,
-                expectedReturnValue: CREATED_CONTRACT_BY_2_1
-              }
-            ]
+                expectedReturnValue: CREATED_CONTRACT_BY_2_1,
+              },
+            ],
           },
-          expectedReturnStatus: true
-        }
+          expectedReturnStatus: true,
+        },
       ],
     },
     {
@@ -618,7 +618,7 @@ const test_ovmCREATE: TestDefinition = {
                   subSteps: [
                     {
                       functionName: 'ovmADDRESS',
-                      expectedReturnValue: NESTED_CREATED_CONTRACT
+                      expectedReturnValue: NESTED_CREATED_CONTRACT,
                     },
                   ],
                 },
@@ -630,7 +630,7 @@ const test_ovmCREATE: TestDefinition = {
                 revertData: DUMMY_REVERT_DATA,
                 expectedReturnStatus: true,
                 expectedReturnValue: '0x00',
-              }
+              },
             ],
           },
           expectedReturnStatus: true,
@@ -655,7 +655,7 @@ const test_ovmCREATE: TestDefinition = {
         },
       ],
     },
-  ]
+  ],
 }
 
 const runner = new ExecutionManagerTestRunner()

packages/contracts/test/helpers/dummy/batches.ts

@@ -6,6 +6,7 @@ export const DUMMY_BATCH_HEADERS = [
     batchRoot: NULL_BYTES32,
     batchSize: 0,
     prevTotalElements: 0,
+    timestamp: 0,
     extraData: NULL_BYTES32,
   },
 ]

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

@@ -168,8 +168,8 @@ export class ExecutionManagerTestRunner {
         secondsPerEpoch: 600,
       },
       {
-        ovmCHAINID: 420
-      },
+        ovmCHAINID: 420,
+      }
     )
 
     this.contracts.OVM_StateManager = await (

packages/contracts/tsconfig.json

@@ -20,8 +20,7 @@
       "node_modules/@types"
     ]
   },
-  "include": ["*.ts", "**/*.ts", "artifacts/*.json"],
-  "exclude": ["./build", "node_modules", "test"],
+  "include": ["src/**/*.ts", "artifacts/*.json"],
   "files": [
     "./buidler.config.ts",
     "./buidler-env.d.ts",