diff --git a/.changeset/wild-cycles-care.md b/.changeset/wild-cycles-care.md new file mode 100644 index 0000000000000..a7927a9ecdc08 --- /dev/null +++ b/.changeset/wild-cycles-care.md @@ -0,0 +1,5 @@ +--- +"@eth-optimism/contracts": patch +--- + +Add pause(), blockMessage() and allowMessage() to L1 messenger diff --git a/packages/contracts/contracts/optimistic-ethereum/OVM/bridge/messaging/Abs_BaseCrossDomainMessenger.sol b/packages/contracts/contracts/optimistic-ethereum/OVM/bridge/messaging/Abs_BaseCrossDomainMessenger.sol index fae425de27aa8..b4b468b5d9d1c 100644 --- a/packages/contracts/contracts/optimistic-ethereum/OVM/bridge/messaging/Abs_BaseCrossDomainMessenger.sol +++ b/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() {} /******************** diff --git a/packages/contracts/contracts/optimistic-ethereum/OVM/bridge/messaging/OVM_L1CrossDomainMessenger.sol b/packages/contracts/contracts/optimistic-ethereum/OVM/bridge/messaging/OVM_L1CrossDomainMessenger.sol index 9d2a69c336ccf..445afd808509b 100644 --- a/packages/contracts/contracts/optimistic-ethereum/OVM/bridge/messaging/OVM_L1CrossDomainMessenger.sol +++ b/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; diff --git a/packages/contracts/contracts/optimistic-ethereum/OVM/bridge/messaging/OVM_L2CrossDomainMessenger.sol b/packages/contracts/contracts/optimistic-ethereum/OVM/bridge/messaging/OVM_L2CrossDomainMessenger.sol index 292ae6eff89c3..dfd3d56d8be6a 100644 --- a/packages/contracts/contracts/optimistic-ethereum/OVM/bridge/messaging/OVM_L2CrossDomainMessenger.sol +++ b/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() {} diff --git a/packages/contracts/package.json b/packages/contracts/package.json index 9284c1ff94960..18d155a11567b 100644 --- a/packages/contracts/package.json +++ b/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" diff --git a/packages/contracts/test/contracts/OVM/bridge/base/OVM_L1CrossDomainMessenger.spec.ts b/packages/contracts/test/contracts/OVM/bridge/base/OVM_L1CrossDomainMessenger.spec.ts index 613ccfb2d6444..7ed33d551d4e9 100644 --- a/packages/contracts/test/contracts/OVM/bridge/base/OVM_L1CrossDomainMessenger.spec.ts +++ b/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.' + ) + }) }) }) }) diff --git a/yarn.lock b/yarn.lock index 4451ecc8e3ef0..3f13f957a4b48 100644 --- a/yarn.lock +++ b/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"