test: reentrancy modifier on OVM_L1CrossDomainMessenger.relayMessage() (#555)

* stub out test

* fix: getXDomainCalldata argument order to match usage

* feat: completed test

* fix: working test

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

@@ -11,7 +11,7 @@ import { Lib_ReentrancyGuard } from "../../../libraries/utils/Lib_ReentrancyGuar
 /**
  * @title Abs_BaseCrossDomainMessenger
  * @dev The Base Cross Domain Messenger is an abstract contract providing the interface and common functionality used in the
- * L1 and L2 Cross Domain Messengers. It can also serve as a template for developers wishing to implement a custom bridge 
+ * L1 and L2 Cross Domain Messengers. It can also serve as a template for developers wishing to implement a custom bridge
  * contract to suit their needs.
  *
  * Compiler used: defined by child contract

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

@@ -14,6 +14,7 @@ import { iOVM_L2ToL1MessagePasser } from "../../../iOVM/predeploys/iOVM_L2ToL1Me
 /* Contract Imports */
 import { Abs_BaseCrossDomainMessenger } from "./Abs_BaseCrossDomainMessenger.sol";
 
+
 /**
  * @title OVM_L2CrossDomainMessenger
  * @dev The L2 Cross Domain Messenger contract sends messages from L2 to L1, and is the entry point
@@ -104,6 +105,7 @@ contract OVM_L2CrossDomainMessenger is iOVM_L2CrossDomainMessenger, Abs_BaseCros
                 block.number
             )
         );
+
         relayedMessages[relayId] = true;
     }
 

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

@@ -113,7 +113,7 @@ describe('OVM_L1CrossDomainMessenger', () => {
       ).to.deep.equal([
         Mock__OVM_L2CrossDomainMessenger.address,
         BigNumber.from(gasLimit),
-        getXDomainCalldata(await signer.getAddress(), target, message, 0),
+        getXDomainCalldata(target, await signer.getAddress(), message, 0),
       ])
     })
 
@@ -171,7 +171,7 @@ describe('OVM_L1CrossDomainMessenger', () => {
       ])
       sender = await signer.getAddress()
 
-      calldata = getXDomainCalldata(sender, target, message, 0)
+      calldata = getXDomainCalldata(target, sender, message, 0)
 
       const precompile = '0x4200000000000000000000000000000000000000'
 

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

@@ -12,6 +12,7 @@ import {
   NON_NULL_BYTES32,
   NON_ZERO_ADDRESS,
   getXDomainCalldata,
+  getNextBlockNumber,
 } from '../../../../helpers'
 import { solidityKeccak256 } from 'ethers/lib/utils'
 
@@ -88,7 +89,7 @@ describe('OVM_L2CrossDomainMessenger', () => {
       expect(
         Mock__OVM_L2ToL1MessagePasser.smocked.passMessageToL1.calls[0]
       ).to.deep.equal([
-        getXDomainCalldata(await signer.getAddress(), target, message, 0),
+        getXDomainCalldata(target, await signer.getAddress(), message, 0),
       ])
     })
 
@@ -192,10 +193,67 @@ describe('OVM_L2CrossDomainMessenger', () => {
         await OVM_L2CrossDomainMessenger.successfulMessages(
           solidityKeccak256(
             ['bytes'],
-            [getXDomainCalldata(await signer.getAddress(), target, message, 0)]
+            [getXDomainCalldata(target, sender, message, 0)]
           )
         )
       ).to.be.true
     })
+
+    it('should revert if trying to reenter `relayMessage`', async () => {
+      Mock__OVM_L1MessageSender.smocked.getL1MessageSender.will.return.with(
+        Mock__OVM_L1CrossDomainMessenger.address
+      )
+
+      const reentrantMessage = OVM_L2CrossDomainMessenger.interface.encodeFunctionData(
+        'relayMessage',
+        [target, sender, message, 1]
+      )
+
+      // Calculate xDomainCallData used for indexing
+      // (within the first call to the L2 Messenger).
+      const xDomainCallData = getXDomainCalldata(
+        OVM_L2CrossDomainMessenger.address,
+        sender,
+        reentrantMessage,
+        0
+      )
+
+      // Make the call.
+      await OVM_L2CrossDomainMessenger.relayMessage(
+        OVM_L2CrossDomainMessenger.address,
+        sender,
+        reentrantMessage,
+        0
+      )
+
+      // We can't test for the nonReentrant revert string because it occurs in the second call frame,
+      // and target.call() won't "bubble up" the revert. So we need to use other criteria to ensure the
+      // right things are happening.
+      // Criteria 1: the reentrant message is NOT listed in successful messages.
+      expect(
+        await OVM_L2CrossDomainMessenger.successfulMessages(
+          solidityKeccak256(['bytes'], [xDomainCallData])
+        )
+      ).to.be.false
+
+      // Criteria 2: the relayID of the reentrant message is recorded.
+      // Get blockNumber at time of the call.
+      const blockNumber = (await getNextBlockNumber(ethers.provider)) - 1
+      const relayId = solidityKeccak256(
+        ['bytes'],
+        [
+          ethers.utils.solidityPack(
+            ['bytes', 'address', 'uint256'],
+            [xDomainCallData, sender, blockNumber]
+          ),
+        ]
+      )
+
+      expect(await OVM_L2CrossDomainMessenger.relayedMessages(relayId)).to.be
+        .true
+
+      // Criteria 3: the target contract did not receive a call.
+      expect(Mock__TargetContract.smocked.setTarget.calls[0]).to.be.undefined
+    })
   })
 })

packages/contracts/test/helpers/codec/bridge.ts

 import { getContractInterface } from '../../../src/contract-defs'
 
 export const getXDomainCalldata = (
-  sender: string,
   target: string,
+  sender: string,
   message: string,
   messageNonce: number
 ): string => {