Audit/tob 6 message passing reentrancy risk 201 (#105)

* reentrancy guard

* check-effects-interactions pattern

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

@@ -4,11 +4,12 @@ pragma experimental ABIEncoderV2;
 
 /* Interface Imports */
 import { iOVM_BaseCrossDomainMessenger } from "../../iOVM/bridge/iOVM_BaseCrossDomainMessenger.sol";
+import '@openzeppelin/contracts/utils/ReentrancyGuard.sol';
 
 /**
  * @title OVM_BaseCrossDomainMessenger
  */
-contract OVM_BaseCrossDomainMessenger is iOVM_BaseCrossDomainMessenger {
+contract OVM_BaseCrossDomainMessenger is iOVM_BaseCrossDomainMessenger, ReentrancyGuard {
 
     /**********************
      * Contract Variables *
@@ -20,11 +21,12 @@ contract OVM_BaseCrossDomainMessenger is iOVM_BaseCrossDomainMessenger {
     uint256 public messageNonce;
     address override public xDomainMessageSender;
 
-
     /********************
      * Public Functions *
      ********************/
 
+    constructor() ReentrancyGuard() public {}
+
     /**
      * Sends a cross domain message to the target messenger.
      * @param _target Target contract address.
@@ -35,7 +37,7 @@ contract OVM_BaseCrossDomainMessenger is iOVM_BaseCrossDomainMessenger {
         address _target,
         bytes memory _message,
         uint32 _gasLimit
-    )
+    ) nonReentrant
         override
         public
     {
@@ -46,11 +48,10 @@ contract OVM_BaseCrossDomainMessenger is iOVM_BaseCrossDomainMessenger {
             messageNonce
         );
 
-        _sendXDomainMessage(xDomainCalldata, _gasLimit);
-
         messageNonce += 1;
         sentMessages[keccak256(xDomainCalldata)] = true;
 
+        _sendXDomainMessage(xDomainCalldata, _gasLimit);
         emit SentMessage(xDomainCalldata);
     }
 

packages/contracts/package.json

@@ -29,6 +29,7 @@
   "dependencies": {
     "@ethersproject/contracts": "^5.0.5",
     "@ethersproject/hardware-wallets": "^5.0.8",
+    "@openzeppelin/contracts": "^3.3.0",
     "ethers": "5.0.0"
   },
   "devDependencies": {

packages/contracts/test/contracts/libraries/rlp/Lib_RLPWriter.spec.ts

@@ -42,7 +42,7 @@ describe('Lib_RLPWriter', () => {
     }
   })
 
-  describe.only('Use of library with other memory-modifying operations', () => {
+  describe('Use of library with other memory-modifying operations', () => {
     it('should allow creation of a contract beforehand and still work', async () => {
       const randomAddress = '0x1234123412341234123412341234123412341234'
       const rlpEncodedRandomAddress = '0x941234123412341234123412341234123412341234'

packages/contracts/yarn.lock

@@ -692,6 +692,11 @@
     safe-buffer "^5.1.1"
     util.promisify "^1.0.0"
 
+"@openzeppelin/contracts@^3.3.0":
+  version "3.3.0"
+  resolved "https://registry.yarnpkg.com/@openzeppelin/contracts/-/contracts-3.3.0.tgz#ffdb693c5c349fc33bba420248dd3ac0a2d7c408"
+  integrity sha512-AemZEsQYtUp1WRkcmZm1div5ORfTpLquLaziCIrSagjxyKdmObxuaY1yjQ5SHFMctR8rLwp706NXTbiIRJg7pw==
+
 "@resolver-engine/core@^0.3.3":
   version "0.3.3"
   resolved "https://registry.yarnpkg.com/@resolver-engine/core/-/core-0.3.3.tgz#590f77d85d45bc7ecc4e06c654f41345db6ca967"