feat[contracts]: Add block/allow and pausing to the L1 Messenger (#579)

* feat(contracts): add pausable.sol

This is identical to Pausable, except for replacing Context._msgSender() with just msg.sender

* feat: add pause, block and allow to L1 messenger

* test(l1-messenger): access control, block, allow

* chore: add changeset

* chore: yarn lint

* test[contracts]: maintain previous test order

* fix[contracts]: add upgradable OZ contracts

* Revert "test[contracts]: maintain previous test order"

This reverts commit 4bbfe60f32e5785debf3cfbe2b3b125f54ed6798.

* test[contracts]: add initializer modifier

* fix(contracts): delete unused file
Co-authored-by: Georgios Konstantopoulos <me@gakonst.com>

.changeset/wild-cycles-care.md

+---
+"@eth-optimism/contracts": patch
+---
+
+Add pause(), blockMessage() and allowMessage() to L1 messenger

packages/contracts/contracts/optimistic-ethereum/OVM/bridge/messaging/Abs_BaseCrossDomainMessenger.sol

@@ -5,9 +5,6 @@ pragma experimental ABIEncoderV2;
 /* Interface Imports */
 import { iAbs_BaseCrossDomainMessenger } from "../../../iOVM/bridge/messaging/iAbs_BaseCrossDomainMessenger.sol";
 
-/* External Imports */
-import { ReentrancyGuard } from "@openzeppelin/contracts/utils/ReentrancyGuard.sol";
-
 /**
  * @title Abs_BaseCrossDomainMessenger
  * @dev The Base Cross Domain Messenger is an abstract contract providing the interface and common
@@ -17,7 +14,7 @@ import { ReentrancyGuard } from "@openzeppelin/contracts/utils/ReentrancyGuard.s
  * Compiler used: defined by child contract
  * Runtime target: defined by child contract
  */
-abstract contract Abs_BaseCrossDomainMessenger is iAbs_BaseCrossDomainMessenger, ReentrancyGuard {
+abstract contract Abs_BaseCrossDomainMessenger is iAbs_BaseCrossDomainMessenger {
 
     /*************
      * Constants *
@@ -44,7 +41,7 @@ abstract contract Abs_BaseCrossDomainMessenger is iAbs_BaseCrossDomainMessenger,
      * Constructor *
      ***************/
 
-    constructor() ReentrancyGuard() {}
+    constructor() {}
 
 
     /********************

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

@@ -16,6 +16,11 @@ import { iOVM_StateCommitmentChain } from "../../../iOVM/chain/iOVM_StateCommitm
 /* Contract Imports */
 import { Abs_BaseCrossDomainMessenger } from "./Abs_BaseCrossDomainMessenger.sol";
 
+/* External Imports */
+import { OwnableUpgradeable } from "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";
+import { PausableUpgradeable } from "@openzeppelin/contracts-upgradeable/utils/PausableUpgradeable.sol";
+import { ReentrancyGuardUpgradeable } from "@openzeppelin/contracts-upgradeable/utils/ReentrancyGuardUpgradeable.sol";
+
 /**
  * @title OVM_L1CrossDomainMessenger
  * @dev The L1 Cross Domain Messenger contract sends messages from L1 to L2, and relays messages
@@ -25,20 +30,46 @@ import { Abs_BaseCrossDomainMessenger } from "./Abs_BaseCrossDomainMessenger.sol
  * Compiler used: solc
  * Runtime target: EVM
  */
-contract OVM_L1CrossDomainMessenger is iOVM_L1CrossDomainMessenger, Abs_BaseCrossDomainMessenger, Lib_AddressResolver {
+contract OVM_L1CrossDomainMessenger is
+        iOVM_L1CrossDomainMessenger,
+        Abs_BaseCrossDomainMessenger,
+        Lib_AddressResolver,
+        OwnableUpgradeable,
+        PausableUpgradeable,
+        ReentrancyGuardUpgradeable
+{
+
+    /**********
+     * Events *
+     **********/
+
+    event MessageBlocked(
+        bytes32 indexed _xDomainCalldataHash
+    );
+
+    event MessageAllowed(
+        bytes32 indexed _xDomainCalldataHash
+    );
+
+    /**********************
+     * Contract Variables *
+     **********************/
+
+    mapping (bytes32 => bool) public blockedMessages;
 
     /***************
      * Constructor *
      ***************/
 
     /**
-     * Pass a default zero address to the address resolver. This will be updated when initialized.
+     * This contract is intended to be behind a delegate proxy.
+     * We pass the zero address to the address resolver just to satisfy the constructor.
+     * We still need to set this value in initialize().
      */
     constructor()
         Lib_AddressResolver(address(0))
     {}
 
-
     /**********************
      * Function Modifiers *
      **********************/
@@ -70,14 +101,58 @@ contract OVM_L1CrossDomainMessenger is iOVM_L1CrossDomainMessenger, Abs_BaseCros
         address _libAddressManager
     )
         public
+        initializer
     {
         require(
             address(libAddressManager) == address(0),
             "L1CrossDomainMessenger already intialized."
         );
-
         libAddressManager = Lib_AddressManager(_libAddressManager);
         xDomainMsgSender = DEFAULT_XDOMAIN_SENDER;
+
+        // Initialize upgradable OZ contracts
+        __Context_init_unchained(); // Context is a dependency for both Ownable and Pausable
+        __Ownable_init_unchained();
+        __Pausable_init_unchained();
+        __ReentrancyGuard_init_unchained();
+    }
+
+    /**
+     * Pause relaying.
+     */
+    function pause()
+        external
+        onlyOwner
+    {
+        _pause();
+    }
+
+    /**
+     * Block a message.
+     * @param _xDomainCalldataHash Hash of the message to block.
+     */
+    function blockMessage(
+        bytes32 _xDomainCalldataHash
+    )
+        external
+        onlyOwner
+    {
+        blockedMessages[_xDomainCalldataHash] = true;
+        emit MessageBlocked(_xDomainCalldataHash);
+    }
+
+    /**
+     * Allow a message.
+     * @param _xDomainCalldataHash Hash of the message to block.
+     */
+    function allowMessage(
+        bytes32 _xDomainCalldataHash
+    )
+        external
+        onlyOwner
+    {
+        blockedMessages[_xDomainCalldataHash] = false;
+        emit MessageAllowed(_xDomainCalldataHash);
     }
 
     /**
@@ -94,7 +169,8 @@ contract OVM_L1CrossDomainMessenger is iOVM_L1CrossDomainMessenger, Abs_BaseCros
         override
         public
         nonReentrant
-        onlyRelayer()
+        onlyRelayer
+        whenNotPaused
     {
         bytes memory xDomainCalldata = _getXDomainCalldata(
             _target,
@@ -118,6 +194,11 @@ contract OVM_L1CrossDomainMessenger is iOVM_L1CrossDomainMessenger, Abs_BaseCros
             "Provided message has already been received."
         );
 
+        require(
+            blockedMessages[xDomainCalldataHash] == false,
+            "Provided message has been blocked."
+        );
+
         xDomainMsgSender = _sender;
         (bool success, ) = _target.call(_message);
         xDomainMsgSender = DEFAULT_XDOMAIN_SENDER;

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

@@ -13,6 +13,9 @@ import { iOVM_L2ToL1MessagePasser } from "../../../iOVM/predeploys/iOVM_L2ToL1Me
 /* Contract Imports */
 import { Abs_BaseCrossDomainMessenger } from "./Abs_BaseCrossDomainMessenger.sol";
 
+/* External Imports */
+import { ReentrancyGuard } from "@openzeppelin/contracts/utils/ReentrancyGuard.sol";
+
 /**
  * @title OVM_L2CrossDomainMessenger
  * @dev The L2 Cross Domain Messenger contract sends messages from L2 to L1, and is the entry point
@@ -21,7 +24,12 @@ import { Abs_BaseCrossDomainMessenger } from "./Abs_BaseCrossDomainMessenger.sol
  * Compiler used: optimistic-solc
  * Runtime target: OVM
   */
-contract OVM_L2CrossDomainMessenger is iOVM_L2CrossDomainMessenger, Abs_BaseCrossDomainMessenger, Lib_AddressResolver {
+contract OVM_L2CrossDomainMessenger is
+    iOVM_L2CrossDomainMessenger,
+    Abs_BaseCrossDomainMessenger,
+    Lib_AddressResolver,
+    ReentrancyGuard
+{
 
     /***************
      * Constructor *
@@ -34,6 +42,7 @@ contract OVM_L2CrossDomainMessenger is iOVM_L2CrossDomainMessenger, Abs_BaseCros
         address _libAddressManager
     )
         Lib_AddressResolver(_libAddressManager)
+        ReentrancyGuard()
     {}
 
 

packages/contracts/package.json

@@ -48,6 +48,7 @@
     "@ethersproject/abstract-provider": "^5.0.8",
     "@ethersproject/contracts": "^5.0.5",
     "@openzeppelin/contracts": "^3.3.0",
+    "@openzeppelin/contracts-upgradeable": "^3.3.0",
     "@typechain/hardhat": "^1.0.1",
     "ganache-core": "^2.13.2",
     "glob": "^7.1.6"

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

@@ -36,8 +36,9 @@ const deployProxyXDomainMessenger = async (
 
 describe('OVM_L1CrossDomainMessenger', () => {
   let signer: Signer
+  let signer2: Signer
   before(async () => {
-    ;[signer] = await ethers.getSigners()
+    ;[signer, signer2] = await ethers.getSigners()
   })
 
   let AddressManager: Contract
@@ -89,15 +90,33 @@ describe('OVM_L1CrossDomainMessenger', () => {
 
   let OVM_L1CrossDomainMessenger: Contract
   beforeEach(async () => {
-    const xDomainMessenerImpl = await Factory__OVM_L1CrossDomainMessenger.deploy()
+    const xDomainMessengerImpl = await Factory__OVM_L1CrossDomainMessenger.deploy()
     // We use an upgradable proxy for the XDomainMessenger--deploy & set up the proxy.
     OVM_L1CrossDomainMessenger = await deployProxyXDomainMessenger(
       AddressManager,
-      xDomainMessenerImpl
+      xDomainMessengerImpl
     )
     await OVM_L1CrossDomainMessenger.initialize(AddressManager.address)
   })
 
+  describe('pause', () => {
+    describe('when called by the current owner', () => {
+      it('should pause the contract', async () => {
+        await OVM_L1CrossDomainMessenger.pause()
+
+        expect(await OVM_L1CrossDomainMessenger.paused()).to.be.true
+      })
+    })
+
+    describe('when called by account other than the owner', () => {
+      it('should not pause the contract', async () => {
+        await expect(
+          OVM_L1CrossDomainMessenger.connect(signer2).pause()
+        ).to.be.revertedWith('Ownable: caller is not the owner')
+      })
+    })
+  })
+
   describe('sendMessage', () => {
     const target = NON_ZERO_ADDRESS
     const message = NON_NULL_BYTES32
@@ -355,12 +374,8 @@ describe('OVM_L1CrossDomainMessenger', () => {
       ).to.be.revertedWith('Provided message has already been received.')
     })
 
-    it('when the OVM_L2MessageRelayer address is set, should revert if called by a different account', async () => {
-      // set to a random NON-ZERO address
-      await AddressManager.setAddress(
-        'OVM_L2MessageRelayer',
-        '0x1234123412341234123412341234123412341234'
-      )
+    it('should revert if paused', async () => {
+      await OVM_L1CrossDomainMessenger.pause()
 
       await expect(
         OVM_L1CrossDomainMessenger.relayMessage(
@@ -370,9 +385,84 @@ describe('OVM_L1CrossDomainMessenger', () => {
           0,
           proof
         )
-      ).to.be.revertedWith(
-        'Only OVM_L2MessageRelayer can relay L2-to-L1 messages.'
-      )
+      ).to.be.revertedWith('Pausable: paused')
+    })
+
+    describe('blockMessage and allowMessage', () => {
+      it('should revert if called by an account other than the owner', async () => {
+        const OVM_L1CrossDomainMessenger2 = OVM_L1CrossDomainMessenger.connect(
+          signer2
+        )
+        await expect(
+          OVM_L1CrossDomainMessenger2.blockMessage(keccak256(calldata))
+        ).to.be.revertedWith('Ownable: caller is not the owner')
+
+        await expect(
+          OVM_L1CrossDomainMessenger2.allowMessage(keccak256(calldata))
+        ).to.be.revertedWith('Ownable: caller is not the owner')
+      })
+
+      it('should revert if the message is blocked', async () => {
+        await OVM_L1CrossDomainMessenger.blockMessage(keccak256(calldata))
+
+        await expect(
+          OVM_L1CrossDomainMessenger.relayMessage(
+            target,
+            sender,
+            message,
+            0,
+            proof
+          )
+        ).to.be.revertedWith('Provided message has been blocked.')
+      })
+
+      it('should succeed if the message is blocked, then unblocked', async () => {
+        await OVM_L1CrossDomainMessenger.blockMessage(keccak256(calldata))
+
+        await expect(
+          OVM_L1CrossDomainMessenger.relayMessage(
+            target,
+            sender,
+            message,
+            0,
+            proof
+          )
+        ).to.be.revertedWith('Provided message has been blocked.')
+
+        await OVM_L1CrossDomainMessenger.allowMessage(keccak256(calldata))
+
+        await expect(
+          OVM_L1CrossDomainMessenger.relayMessage(
+            target,
+            sender,
+            message,
+            0,
+            proof
+          )
+        ).to.not.be.reverted
+      })
+    })
+
+    describe('onlyRelayer', () => {
+      it('when the OVM_L2MessageRelayer address is set, should revert if called by a different account', async () => {
+        // set to a random NON-ZERO address
+        await AddressManager.setAddress(
+          'OVM_L2MessageRelayer',
+          '0x1234123412341234123412341234123412341234'
+        )
+
+        await expect(
+          OVM_L1CrossDomainMessenger.relayMessage(
+            target,
+            sender,
+            message,
+            0,
+            proof
+          )
+        ).to.be.revertedWith(
+          'Only OVM_L2MessageRelayer can relay L2-to-L1 messages.'
+        )
+      })
     })
   })
 })

yarn.lock

@@ -1830,6 +1830,11 @@
   dependencies:
     "@octokit/openapi-types" "^6.0.0"
 
+"@openzeppelin/contracts-upgradeable@^3.3.0":
+  version "3.4.1"
+  resolved "https://registry.yarnpkg.com/@openzeppelin/contracts-upgradeable/-/contracts-upgradeable-3.4.1.tgz#38dfdfa86fda0a088c6fcdebe6870cfaf897b471"
+  integrity sha512-wBGlUzEkOxcj/ghtcF2yKc8ZYh+PTUtm1mK38zoENulJ6aplij7eH8quo3lMugfzPJy+V6V5qI8QhdQmCn7hkQ==
+
 "@openzeppelin/contracts@^3.3.0":
   version "3.4.1"
   resolved "https://registry.yarnpkg.com/@openzeppelin/contracts/-/contracts-3.4.1.tgz#03c891fec7f93be0ae44ed74e57a122a38732ce7"