More updates post-review

Author: area

contracts/bridging/WormholeBridgeForColony.sol

@@ -21,9 +21,10 @@ pragma solidity 0.8.23;
 import { IWormhole } from "../../lib/wormhole/ethereum/contracts/interfaces/IWormhole.sol";
 import { IColonyNetwork } from "../colonyNetwork/IColonyNetwork.sol";
 import { IColonyBridge } from "./IColonyBridge.sol";
+import { CallWithGuards } from "../common/CallWithGuards.sol";
 import { DSAuth } from "../../lib/dappsys/auth.sol";
 
-contract WormholeBridgeForColony is DSAuth, IColonyBridge {
+contract WormholeBridgeForColony is DSAuth, IColonyBridge, CallWithGuards {
   address colonyNetwork;
   // ChainId => colonyBridge
   mapping(uint256 => address) colonyBridges;
@@ -100,22 +101,15 @@ contract WormholeBridgeForColony is DSAuth, IColonyBridge {
 
     // Do the thing
 
-    (bool success, bytes memory returndata) = address(colonyNetwork).call(wormholeMessage.payload);
+    (bool success, bytes memory returndata) = callWithGuards(
+      colonyNetwork,
+      wormholeMessage.payload
+    );
+    // Note that this is not a require because returndata might not be a string, and if we try
+    // to decode it we'll get a revert.
     if (!success) {
-      // Stolen shamelessly from
-      // https://ethereum.stackexchange.com/questions/83528/how-can-i-get-the-revert-reason-of-a-call-in-solidity-so-that-i-can-use-it-in-th
-      // If the _res length is less than 68, then the transaction failed silently (without a revert message)
-      if (returndata.length >= 68) {
-        assembly {
-          // Slice the sighash.
-          returndata := add(returndata, 0x04)
-        }
-        require(false, abi.decode(returndata, (string))); // All that remains is the revert string
-      }
-      require(false, "require-execute-call-reverted-with-no-error");
+      revert(abi.decode(returndata, (string)));
     }
-
-    require(success, "wormhole-bridge-receive-message-failed");
   }
 
   function sendMessage(

contracts/common/CallWithGuards.sol

@@ -0,0 +1,35 @@
+pragma solidity 0.8.23;
+
+contract CallWithGuards {
+  function isContract(address addr) internal view returns (bool) {
+    uint256 size;
+    assembly {
+      size := extcodesize(addr)
+    }
+    return size > 0;
+  }
+
+  function callWithGuards(
+    address _target,
+    bytes memory _payload
+  ) internal returns (bool, bytes memory) {
+    if (!isContract(_target)) {
+      return (false, abi.encode("require-execute-call-target-not-contract"));
+    }
+    (bool success, bytes memory returndata) = address(_target).call(_payload);
+    if (!success) {
+      // Stolen shamelessly from
+      // https://ethereum.stackexchange.com/questions/83528/how-can-i-get-the-revert-reason-of-a-call-in-solidity-so-that-i-can-use-it-in-th
+      // If the _res length is less than 68, then the transaction failed silently (without a revert message)
+      if (returndata.length >= 68) {
+        assembly {
+          // Slice the sighash.
+          returndata := add(returndata, 0x04)
+        }
+        return (false, returndata); // All that remains is the revert string
+      }
+      return (false, abi.encode("require-execute-call-reverted-with-no-error"));
+    }
+    return (success, returndata);
+  }
+}

contracts/testHelpers/RequireExecuteCall.sol

@@ -19,23 +19,11 @@
 pragma solidity 0.8.23;
 pragma experimental ABIEncoderV2;
 
-contract RequireExecuteCall {
+import { CallWithGuards } from "./../common/CallWithGuards.sol";
+
+contract RequireExecuteCall is CallWithGuards {
   function executeCall(address target, bytes memory action) public {
-    bool success;
-    bytes memory returndata;
-    (success, returndata) = target.call(action);
-    if (!success) {
-      // Stolen shamelessly from
-      // https://ethereum.stackexchange.com/questions/83528/how-can-i-get-the-revert-reason-of-a-call-in-solidity-so-that-i-can-use-it-in-th
-      // If the _res length is less than 68, then the transaction failed silently (without a revert message)
-      if (returndata.length >= 68) {
-        assembly {
-          // Slice the sighash.
-          returndata := add(returndata, 0x04)
-        }
-        require(false, abi.decode(returndata, (string))); // All that remains is the revert string
-      }
-      require(false, "require-execute-call-reverted-with-no-error");
-    }
+    (bool success, bytes memory returndata) = callWithGuards(target, action);
+    require(success, abi.decode(returndata, (string)));
   }
 }

hardhat.config.js

@@ -1,3 +1,5 @@
+const { FORKED_XDAI_CHAINID } = require("./helpers/constants");
+
 module.exports = {
   defaultNetwork: "hardhat",
   solidity: {
@@ -15,7 +17,7 @@ module.exports = {
   },
   networks: {
     hardhat: {
-      chainId: 265669100,
+      chainId: FORKED_XDAI_CHAINID,
       throwOnCallFailures: false,
       throwOnTransactionFailures: false,
       blockGasLimit: 6721975,

helpers/test-helper.js

@@ -431,12 +431,7 @@ exports.expectAllEvents = async function expectAllEvents(tx, eventNames) {
 };
 
 exports.forwardTime = async function forwardTime(seconds, test, _web3provider) {
-  let web3provider;
-  if (!_web3provider) {
-    web3provider = web3.currentProvider;
-  } else {
-    web3provider = _web3provider;
-  }
+  const web3provider = _web3provider || web3.currentProvider;
   if (typeof seconds !== "number") {
     throw new Error("typeof seconds is not a number");
   }
@@ -1247,14 +1242,7 @@ exports.sleep = function sleep(ms) {
 };
 
 exports.upgradeColonyTo = async function (colony, _version) {
-  let version = _version;
-  if (!BN.isBN(_version)) {
-    try {
-      version = new BN(_version);
-    } catch (err) {
-      console.log("Unable to convert passed version to BN");
-    }
-  }
+  const version = new BN(_version);
   let currentVersion = await colony.version();
   while (currentVersion.ltn(version)) {
     await colony.upgrade(currentVersion.addn(1));

scripts/mockBridgeMonitor.js

@@ -45,6 +45,10 @@ class MockBridgeMonitor {
     });
   }
 
+  // VAAs are the primitives used on wormhole (Verified Action Approvals)
+  // See https://docs.wormhole.com/wormhole/explore-wormhole/vaa for more details
+  // Note that the documentation sometimes also calls them VMs (as does IWormhole)
+  // I believe VM stands for 'Verified Message'
   async encodeMockVAA(sender, sequence, nonce, payload, consistencyLevel, chainId) {
     const version = 1;
     const timestamp = Math.floor(Date.now() / 1000);

test/contracts-network/colony-network.js

@@ -507,11 +507,7 @@ contract("Colony Network", (accounts) => {
     let suffix;
 
     before(async () => {
-      if (await isXdai()) {
-        suffix = "colonyxdai";
-      } else {
-        suffix = "eth";
-      }
+      suffix = (await isXdai()) ? "colonyxdai" : "eth";
     });
 
     beforeEach(async () => {