Add success check to message passing contracts (#18)

* Added fix for error in StateTransitioner

* Added tests for some precompiles

* Cleaned build and added typechain

* Linted files

* Added success check to message passing contracts

* Audit fix/message passer storage (#19)

* Made message passer use storage

* Fixed message hashing

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

@@ -14,7 +14,8 @@ contract OVM_BaseCrossDomainMessenger is iOVM_BaseCrossDomainMessenger {
      * Contract Variables *
      **********************/
 
-    mapping (bytes32 => bool) public receivedMessages;
+    mapping (bytes32 => bool) public relayedMessages;
+    mapping (bytes32 => bool) public successfulMessages;
     mapping (bytes32 => bool) public sentMessages;
     uint256 public messageNonce;
     address public xDomainMessageSender;

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

@@ -6,7 +6,6 @@ pragma experimental ABIEncoderV2;
 import { Lib_OVMCodec } from "../../libraries/codec/Lib_OVMCodec.sol";
 import { Lib_AddressResolver } from "../../libraries/resolver/Lib_AddressResolver.sol";
 import { Lib_SecureMerkleTrie } from "../../libraries/trie/Lib_SecureMerkleTrie.sol";
-import { Lib_BytesUtils } from "../../libraries/utils/Lib_BytesUtils.sol";
 
 /* Interface Imports */
 import { iOVM_L1CrossDomainMessenger } from "../../iOVM/bridge/iOVM_L1CrossDomainMessenger.sol";
@@ -83,19 +82,29 @@ contract OVM_L1CrossDomainMessenger is iOVM_L1CrossDomainMessenger, OVM_BaseCros
         );
 
         require(
-            receivedMessages[keccak256(xDomainCalldata)] == false,
+            successfulMessages[keccak256(xDomainCalldata)] == false,
             "Provided message has already been received."
         );
 
         xDomainMessageSender = _sender;
-        _target.call(_message);
-
-        // Messages are considered successfully executed if they complete
-        // without running out of gas (revert or not). As a result, we can
-        // ignore the result of the call and always mark the message as
-        // successfully executed because we won't get here unless we have
-        // enough gas left over.
-        receivedMessages[keccak256(xDomainCalldata)] = true;
+        (bool success, ) = _target.call(_message);
+
+        // Mark the message as received if the call was successful. Ensures that a message can be
+        // relayed multiple times in the case that the call reverted.
+        if (success == true) {
+            successfulMessages[keccak256(xDomainCalldata)] = true;
+        }
+
+        // Store an identifier that can be used to prove that the given message was relayed by some
+        // user. Gives us an easy way to pay relayers for their work.
+        bytes32 relayId = keccak256(
+            abi.encodePacked(
+                xDomainCalldata,
+                msg.sender,
+                block.number
+            )
+        );
+        relayedMessages[relayId] = true;
     }
 
     /**
@@ -189,15 +198,20 @@ contract OVM_L1CrossDomainMessenger is iOVM_L1CrossDomainMessenger, OVM_BaseCros
         L2MessageInclusionProof memory _proof
     )
         internal
-        pure
+        view
         returns (
             bool
         )
     {
         bytes32 storageKey = keccak256(
-            Lib_BytesUtils.concat(
-                abi.encodePacked(keccak256(_xDomainCalldata)),
-                abi.encodePacked(uint256(0))
+            abi.encodePacked(
+                keccak256(
+                    abi.encodePacked(
+                        _xDomainCalldata,
+                        resolve("OVM_L2CrossDomainMessenger")
+                    )
+                ),
+                uint256(0)
             )
         );
 

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

 // SPDX-License-Identifier: UNLICENSED
-pragma solidity ^0.7.0;
+pragma solidity >0.6.0 <0.8.0;
 pragma experimental ABIEncoderV2;
 
 /* Library Imports */
@@ -18,6 +18,7 @@ import { console } from "@nomiclabs/buidler/console.sol";
 
 /**
  * @title OVM_L2CrossDomainMessenger
+ * @dev L2 CONTRACT (COMPILED)
  */
 contract OVM_L2CrossDomainMessenger is iOVM_L2CrossDomainMessenger, OVM_BaseCrossDomainMessenger, Lib_AddressResolver {
 
@@ -76,19 +77,29 @@ contract OVM_L2CrossDomainMessenger is iOVM_L2CrossDomainMessenger, OVM_BaseCros
         );
 
         require(
-            receivedMessages[keccak256(xDomainCalldata)] == false,
+            successfulMessages[keccak256(xDomainCalldata)] == false,
             "Provided message has already been received."
         );
 
         xDomainMessageSender = _sender;
-        _target.call(_message);
-
-        // Messages are considered successfully executed if they complete
-        // without running out of gas (revert or not). As a result, we can
-        // ignore the result of the call and always mark the message as
-        // successfully executed because we won't get here unless we have
-        // enough gas left over.
-        receivedMessages[keccak256(xDomainCalldata)] = true;
+        (bool success, ) = _target.call(_message);
+
+        // Mark the message as received if the call was successful. Ensures that a message can be
+        // relayed multiple times in the case that the call reverted.
+        if (success == true) {
+            successfulMessages[keccak256(xDomainCalldata)] = true;
+        }
+
+        // Store an identifier that can be used to prove that the given message was relayed by some
+        // user. Gives us an easy way to pay relayers for their work.
+        bytes32 relayId = keccak256(
+            abi.encodePacked(
+                xDomainCalldata,
+                msg.sender,
+                block.number
+            )
+        );
+        relayedMessages[relayId] = true;
     }
 
 

packages/contracts/contracts/optimistic-ethereum/OVM/precompiles/OVM_DeployerWhitelist.sol

@@ -10,6 +10,7 @@ import { iOVM_ExecutionManager } from "../../iOVM/execution/iOVM_ExecutionManage
 
 /**
  * @title OVM_DeployerWhitelist
+ * @dev L2 CONTRACT (NOT COMPILED)
  */
 contract OVM_DeployerWhitelist is iOVM_DeployerWhitelist {
 

packages/contracts/contracts/optimistic-ethereum/OVM/precompiles/OVM_L1MessageSender.sol

@@ -7,6 +7,7 @@ import { iOVM_ExecutionManager } from "../../iOVM/execution/iOVM_ExecutionManage
 
 /**
  * @title OVM_L1MessageSender
+ * @dev L2 CONTRACT (NOT COMPILED)
  */
 contract OVM_L1MessageSender is iOVM_L1MessageSender {
 

packages/contracts/contracts/optimistic-ethereum/OVM/precompiles/OVM_L2ToL1MessagePasser.sol

 // SPDX-License-Identifier: UNLICENSED
-pragma solidity ^0.7.0;
+pragma solidity >0.6.0 <0.8.0;
 
 /* Interface Imports */
 import { iOVM_L2ToL1MessagePasser } from "../../iOVM/precompiles/iOVM_L2ToL1MessagePasser.sol";
@@ -7,6 +7,7 @@ import { iOVM_ExecutionManager } from "../../iOVM/execution/iOVM_ExecutionManage
 
 /**
  * @title OVM_L2ToL1MessagePasser
+ * @dev L2 CONTRACT (COMPILED)
  */
 contract OVM_L2ToL1MessagePasser is iOVM_L2ToL1MessagePasser {
 
@@ -14,7 +15,7 @@ contract OVM_L2ToL1MessagePasser is iOVM_L2ToL1MessagePasser {
      * Contract Variables *
      **********************/
 
-    uint256 internal nonce;
+    mapping (bytes32 => bool) public sentMessages;
 
 
     /********************
@@ -31,14 +32,11 @@ contract OVM_L2ToL1MessagePasser is iOVM_L2ToL1MessagePasser {
         override
         public
     {
-        // For now, to be trustfully relayed by sequencer to L1, so just emit
-        // an event for the sequencer to pick up.
-        emit L2ToL1Message(
-            nonce,
-            iOVM_ExecutionManager(msg.sender).ovmCALLER(),
-            _message
-        );
-
-        nonce = nonce + 1;
+        sentMessages[keccak256(
+            abi.encodePacked(
+                _message,
+                msg.sender
+            )
+        )] = true;
     }
 }

packages/contracts/test/contracts/OVM/bridge/OVM_L1CrossDomainMessenger.spec.ts

@@ -17,6 +17,8 @@ import {
   DUMMY_BATCH_PROOFS,
   TrieTestGenerator,
   toHexString,
+  getNextBlockNumber,
+  remove0x,
 } from '../../../helpers'
 import { getContractInterface } from '../../../../src'
 import { keccak256 } from 'ethers/lib/utils'
@@ -157,6 +159,7 @@ describe('OVM_L1CrossDomainMessenger', () => {
     let message: string
     let sender: string
     let proof: any
+    let calldata: string
     before(async () => {
       target = Mock__TargetContract.address
       message = Mock__TargetContract.interface.encodeFunctionData('setTarget', [
@@ -164,11 +167,15 @@ describe('OVM_L1CrossDomainMessenger', () => {
       ])
       sender = await signer.getAddress()
 
-      const calldata = getXDomainCalldata(sender, target, message, 0)
+      calldata = getXDomainCalldata(sender, target, message, 0)
 
       const precompile = '0x4200000000000000000000000000000000000000'
 
-      const storageKey = keccak256(keccak256(calldata) + '00'.repeat(32))
+      const storageKey = keccak256(
+        keccak256(
+          calldata + remove0x(Mock__OVM_L2CrossDomainMessenger.address)
+        ) + '00'.repeat(32)
+      )
       const storageGenerator = await TrieTestGenerator.fromNodes({
         nodes: [
           {
@@ -279,7 +286,9 @@ describe('OVM_L1CrossDomainMessenger', () => {
       ).to.be.reverted
     })
 
-    it('should send a call to the target contract', async () => {
+    it('should send a successful call to the target contract', async () => {
+      const blockNumber = await getNextBlockNumber(ethers.provider)
+
       await OVM_L1CrossDomainMessenger.relayMessage(
         target,
         sender,
@@ -288,9 +297,22 @@ describe('OVM_L1CrossDomainMessenger', () => {
         proof
       )
 
-      expect(Mock__TargetContract.smocked.setTarget.calls[0]).to.deep.equal([
-        NON_ZERO_ADDRESS,
-      ])
+      expect(
+        await OVM_L1CrossDomainMessenger.successfulMessages(keccak256(calldata))
+      ).to.equal(true)
+
+      expect(
+        await OVM_L1CrossDomainMessenger.relayedMessages(
+          keccak256(
+            calldata +
+              remove0x(await signer.getAddress()) +
+              remove0x(BigNumber.from(blockNumber).toHexString()).padStart(
+                64,
+                '0'
+              )
+          )
+        )
+      ).to.equal(true)
     })
 
     it('should revert if trying to send the same message twice', async () => {

packages/contracts/test/contracts/OVM/precompiles/OVM_L2ToL1MessagePasser.spec.ts

@@ -5,6 +5,8 @@ import { ethers } from '@nomiclabs/buidler'
 import { ContractFactory, Contract } from 'ethers'
 import { MockContract, smockit } from '@eth-optimism/smock'
 import { NON_ZERO_ADDRESS } from '../../../helpers/constants'
+import { keccak256 } from 'ethers/lib/utils'
+import { remove0x } from '../../../helpers'
 
 const ELEMENT_TEST_SIZES = [1, 2, 4, 8, 16]
 
@@ -61,16 +63,18 @@ describe('OVM_L2ToL1MessagePasser', () => {
         for (let i = 0; i < size; i++) {
           const message = '0x' + '12' + '34'.repeat(i)
 
-          await expect(
-            callPrecompile(
+          await callPrecompile(
             Helper_PrecompileCaller,
             OVM_L2ToL1MessagePasser,
             'passMessageToL1',
             [message]
           )
+
+          expect(
+            await OVM_L2ToL1MessagePasser.sentMessages(
+              keccak256(message + remove0x(Helper_PrecompileCaller.address))
             )
-            .to.emit(OVM_L2ToL1MessagePasser, 'L2ToL1Message')
-            .withArgs(i, NON_ZERO_ADDRESS, message)
+          ).to.equal(true)
         }
       })
     }