Make short-circuiting old version deployments more robust to snapshotting

packages/package-utils/RetryProvider.js

@@ -8,10 +8,18 @@ class RetryProvider extends ethers.providers.StaticJsonRpcProvider {
   }
 
   static attemptCheck(err, attemptNumber) {
-    if (err.code === "CALL_EXCEPTION" || err.code === "UNPREDICTABLE_GAS_LIMIT") {
+    const allowedErrorCodes = ["CALL_EXCEPTION", "UNPREDICTABLE_GAS_LIMIT"];
+    if (allowedErrorCodes.includes(err.code)) {
       console.log(`Got a ${err.code}, no retrying`);
       return false;
     }
+
+    // I _think_ this means we're using solidity-coverage vs stock hardhat, but haven't dug in to it
+    if (err.error && err.error.data && err.error.data.length > 10 && err.error.data.substring(0, 10) === "0x08c379a0") {
+      console.log("Got a revert with reason, no retrying");
+      return false;
+    }
+
     console.log("Retrying RPC request #", attemptNumber);
     if (attemptNumber === 5) {
       return false;

scripts/deployOldUpgradeableVersion.js

@@ -8,9 +8,9 @@ const contract = require("@truffle/contract");
 const { web3GetCode, setStorageSlot } = require("../helpers/test-helper");
 const { ROOT_ROLE, RECOVERY_ROLE, ADMINISTRATION_ROLE, ARCHITECTURE_ROLE, ADDRESS_ZERO } = require("../helpers/constants");
 
-const colonyDeployed = {};
-const colonyNetworkDeployed = {};
-const deployedResolverAddresses = {};
+let colonyDeployed = {};
+let colonyNetworkDeployed = {};
+let deployedResolverAddresses = {};
 
 module.exports.deployOldExtensionVersion = async (contractName, interfaceName, implementationNames, versionTag, colonyNetwork) => {
   if (versionTag.indexOf(" ") !== -1) {
@@ -108,6 +108,12 @@ module.exports.registerOldColonyVersion = async (colonyVersionResolverAddress, c
   }
 };
 
+module.exports.resetAlreadyDeployedVersionTracking = async () => {
+  colonyDeployed = {};
+  colonyNetworkDeployed = {};
+  deployedResolverAddresses = {};
+};
+
 module.exports.deployOldColonyVersion = async (contractName, interfaceName, implementationNames, versionTag, colonyNetwork) => {
   if (versionTag.indexOf(" ") !== -1) {
     throw new Error("Version tag cannot contain spaces");
@@ -116,14 +122,12 @@ module.exports.deployOldColonyVersion = async (contractName, interfaceName, impl
     colonyDeployed[interfaceName] = {};
   }
   if (colonyDeployed[interfaceName][versionTag]) {
-    // Already deployed... if truffle's not snapshotted it away. See if there's any code there.
+    // Already deployed... note that if a snapshot revert is made without calling `resetAlreadyDeployedVersionTracking`,
+    // then this will break.
     const { resolverAddress } = colonyDeployed[interfaceName][versionTag];
-    const code = await web3GetCode(resolverAddress);
-    if (code !== "0x") {
-      // Must also check it's registered
-      await module.exports.registerOldColonyVersion(resolverAddress, colonyNetwork);
-      return colonyDeployed[interfaceName][versionTag];
-    }
+    // Must also check it's registered
+    await module.exports.registerOldColonyVersion(resolverAddress, colonyNetwork);
+    return colonyDeployed[interfaceName][versionTag];
   }
 
   try {

test/contracts-network/colony.js

@@ -413,6 +413,30 @@ contract("Colony", (accounts) => {
 
       await checkErrorRevert(colony.executeMetaTransaction(USER1, txData, r, s, v, { from: USER1 }), "metatransaction-signer-signature-mismatch");
     });
+
+    it("not vulnerable to metatransactions / multicall vulnerability", async () => {
+      // https://blog.solidityscan.com/unveiling-the-erc-2771context-and-multicall-vulnerability-f96ffa5b499f
+      // Create an expenditure as a user
+      await colony.makeExpenditure(1, UINT256_MAX, 1);
+
+      // Should not be able to multicall and cancel it as another user, pretending to be the first user
+      const expenditureId = await colony.getExpenditureCount();
+      let txData1 = await colony.contract.methods.cancelExpenditure(expenditureId).encodeABI();
+
+      const METATRANSACTION_FLAG = ethers.utils.id("METATRANSACTION");
+
+      txData1 += METATRANSACTION_FLAG.slice(2) + USER0.slice(2);
+
+      const txData2 = await colony.contract.methods.multicall([txData1]).encodeABI();
+
+      const { r, s, v } = await getMetaTransactionParameters(txData2, USER1, colony.address);
+
+      // User 1 can't cancel the expenditure directly
+      await checkErrorRevert(colony.cancelExpenditure(expenditureId, { from: USER1 }), "colony-expenditure-not-owner");
+
+      // And can't via metatransaction using specially constructed malicious txdata
+      await checkErrorRevert(colony.executeMetaTransaction(USER1, txData2, r, s, v, { from: USER1 }), "colony-metatx-function-call-unsuccessful");
+    });
   });
 
   describe("when executing a multicall transaction", () => {

test/truffle-fixture.js

@@ -66,6 +66,7 @@ const ethers = require("ethers");
 const { soliditySha3 } = require("web3-utils");
 const truffleContract = require("@truffle/contract");
 const createXABI = require("../lib/createx/artifacts/src/ICreateX.sol/ICreateX.json");
+const { resetAlreadyDeployedVersionTracking } = require("../scripts/deployOldUpgradeableVersion");
 
 let postFixtureSnapshotId;
 
@@ -83,6 +84,7 @@ const { getChainId, hardhatRevert, hardhatSnapshot, deployCreateXIfNeeded, isXda
 module.exports = async () => {
   if (postFixtureSnapshotId) {
     await hardhatRevert(hre.network.provider, postFixtureSnapshotId);
+    await resetAlreadyDeployedVersionTracking();
     postFixtureSnapshotId = await hardhatSnapshot(hre.network.provider);
     return;
   }