Merge pull request #1120 from JoinColony/develop

Merge develop in to master for GLWSS3

contracts/Migrations.sol

@@ -2,7 +2,7 @@ pragma solidity 0.7.3;
 
 
 contract Migrations {
-  address public owner;
+  address public immutable owner;
 
   uint public last_completed_migration;
 

contracts/colony/Colony.sol

@@ -29,7 +29,7 @@ contract Colony is BasicMetaTransaction, Multicall, ColonyStorage, PatriciaTreeP
 
   // This function, exactly as defined, is used in build scripts. Take care when updating.
   // Version number should be upped with every change in Colony or its dependency contracts or libraries.
-  function version() public pure returns (uint256 colonyVersion) { return 11; }
+  function version() public pure returns (uint256 colonyVersion) { return 12; }
 
   function getColonyNetwork() public view returns (address) {
     return colonyNetworkAddress;
@@ -104,13 +104,16 @@ contract Colony is BasicMetaTransaction, Multicall, ColonyStorage, PatriciaTreeP
   {
     require(_users.length == _amounts.length, "colony-bootstrap-bad-inputs");
 
-    for (uint i = 0; i < _users.length; i++) {
+    for (uint256 i = 0; i < _users.length; i++) {
       require(_amounts[i] >= 0, "colony-bootstrap-bad-amount-input");
       require(uint256(_amounts[i]) <= fundingPots[1].balance[token], "colony-bootstrap-not-enough-tokens");
       fundingPots[1].balance[token] = sub(fundingPots[1].balance[token], uint256(_amounts[i]));
       nonRewardPotsTotal[token] = sub(nonRewardPotsTotal[token], uint256(_amounts[i]));
+    }
 
-      assert(ERC20Extended(token).transfer(_users[i], uint256(_amounts[i])));
+    // After doing all the local storage changes, then do all the external calls
+    for (uint256 i = 0; i < _users.length; i++) {
+      require(ERC20Extended(token).transfer(_users[i], uint256(_amounts[i])), "colony-bootstrap-token-transfer-failed");
       IColonyNetwork(colonyNetworkAddress).appendReputationUpdateLog(_users[i], _amounts[i], domains[1].skillId);
     }
 
@@ -292,7 +295,7 @@ contract Colony is BasicMetaTransaction, Multicall, ColonyStorage, PatriciaTreeP
 
   function upgrade(uint256 _newVersion) public always auth {
     // Upgrades can only go up in version, one at a time
-    uint256 currentVersion = this.version();
+    uint256 currentVersion = version();
     require(_newVersion == currentVersion + 1, "colony-version-must-be-one-newer");
     // Requested version has to be registered
     address newResolver = IColonyNetwork(colonyNetworkAddress).getColonyVersionResolver(_newVersion);
@@ -307,12 +310,18 @@ contract Colony is BasicMetaTransaction, Multicall, ColonyStorage, PatriciaTreeP
     emit ColonyUpgraded(msgSender(), currentVersion, _newVersion);
   }
 
-  // v9 to v10
+  // v11 to v12
   function finishUpgrade() public always {
     ColonyAuthority colonyAuthority = ColonyAuthority(address(authority));
     bytes4 sig;
 
-    sig = bytes4(keccak256("setExpenditurePayout(uint256,uint256,uint256,uint256,address,uint256)"));
+    sig = bytes4(keccak256("makeArbitraryTransactions(address[],bytes[],bool)"));
+    colonyAuthority.setRoleCapability(uint8(ColonyRole.Root), address(this), sig, true);
+
+    sig = bytes4(keccak256("setDefaultGlobalClaimDelay(uint256)"));
+    colonyAuthority.setRoleCapability(uint8(ColonyRole.Root), address(this), sig, true);
+
+    sig = bytes4(keccak256("setExpenditureMetadata(uint256,uint256,uint256,string)"));
     colonyAuthority.setRoleCapability(uint8(ColonyRole.Arbitration), address(this), sig, true);
   }
 

contracts/colony/ColonyAuthority.sol

@@ -28,6 +28,8 @@ contract ColonyAuthority is CommonAuthority {
   uint8 constant ARCHITECTURE_ROLE = uint8(ColonyDataTypes.ColonyRole.Architecture);
   uint8 constant ROOT_ROLE = uint8(ColonyDataTypes.ColonyRole.Root);
 
+  // colony is used in the constructor by calls to addRoleCapability, despite what slither thinks
+  // slither-disable-next-line immutable-states
   address internal colony;
 
   constructor(address _colony) public CommonAuthority(_colony) {

contracts/colony/ColonyRoles.sol

@@ -98,20 +98,23 @@ contract ColonyRoles is ColonyStorage, ContractRecoveryDataTypes {
     bytes32 rolesChanged = _roles ^ existingRoles;
     bytes32 roles = _roles;
 
+    // Update the storage slot tracking number of recovery roles before all the external calls are complete
+    // This takes advantage of the fact that the recovery role is the LSB in the roles bytemaps
+    if (uint256(rolesChanged) % 2 == 1) {
+      setTo = uint256(roles) % 2 == 1;
+      if (setTo){
+        recoveryRolesCount += 1;
+      } else {
+        recoveryRolesCount -= 1;
+      }
+    }
+
     for (uint8 roleId; roleId < uint8(ColonyRole.NUMBER_OF_ROLES); roleId += 1) {
       bool changed = uint256(rolesChanged) % 2 == 1;
       if (changed) {
         setTo = uint256(roles) % 2 == 1;
 
         ColonyAuthority(address(authority)).setUserRole(_user, _domainId, roleId, setTo);
-        if (roleId == uint8(ColonyRole.Recovery)) {
-          if (setTo){
-            recoveryRolesCount++;
-          } else {
-            recoveryRolesCount--;
-          }
-          emit RecoveryRoleSet(_user, setTo);
-        }
         emit ColonyRoleSet(msgSender(), _user, _domainId, roleId, setTo);
 
       }

contracts/colony/ColonyTask.sol

@@ -517,7 +517,7 @@ contract ColonyTask is ColonyStorage {
         reputation = getReputation(payout, role.rating, false);
       }
       int256 nSkills = 0;
-      for (uint i = 0; i < task.skills.length; i += 1) {
+      for (uint256 i = 0; i < task.skills.length; i += 1) {
         if (task.skills[i] > 0 ) {
           nSkills += 1;
         }
@@ -527,7 +527,7 @@ contract ColonyTask is ColonyStorage {
 
       int256 reputationPerSkill = reputation / nSkills;
 
-      for (uint i = 0; i < task.skills.length; i += 1) {
+      for (uint256 i = 0; i < task.skills.length; i += 1) {
         if (task.skills[i] > 0) {
           colonyNetworkContract.appendReputationUpdateLog(role.user, reputationPerSkill, task.skills[i]);
         }
@@ -558,7 +558,7 @@ contract ColonyTask is ColonyStorage {
   ) internal pure returns (address[] memory)
   {
     address[] memory reviewerAddresses = new address[](_sigR.length);
-    for (uint i = 0; i < _sigR.length; i++) {
+    for (uint256 i = 0; i < _sigR.length; i++) {
       // 0 'Normal' mode - geth, etc.
       // >0 'Trezor' mode
       // Correct incantation helpfully cribbed from https://github.com/trezor/trezor-mcu/issues/163#issuecomment-368435292

contracts/colonyNetwork/ColonyNetwork.sol

@@ -285,7 +285,7 @@ contract ColonyNetwork is BasicMetaTransaction, ColonyNetworkStorage, Multicall
     }
 
     Skill storage skill = skills[_skillId];
-    for (uint i; i < skill.parents.length; i++) {
+    for (uint256 i; i < skill.parents.length; i++) {
       if (2**(i+1) > _parentSkillNumber) {
         uint _newSkillId = skill.parents[i];
         uint _newParentSkillNumber = _parentSkillNumber - 2**i;

contracts/colonyNetwork/ColonyNetworkAuction.sol

@@ -61,10 +61,10 @@ contract ColonyNetworkAuction is ColonyNetworkStorage, MultiChain {
 
 
 contract DutchAuction is DSMath, MultiChain, BasicMetaTransaction {
-  address payable public colonyNetwork;
-  address public metaColonyAddress;
-  ERC20Extended public clnyToken;
-  ERC20Extended public token;
+  address payable public immutable colonyNetwork;
+  address public immutable metaColonyAddress;
+  ERC20Extended public immutable clnyToken;
+  ERC20Extended public immutable token;
 
   // Total number of auctioned tokens
   uint public quantity;

contracts/colonyNetwork/ColonyNetworkMining.sol

@@ -211,11 +211,14 @@ contract ColonyNetworkMining is ColonyNetworkStorage, MultiChain {
     uint256 lostStake;
     // Passing an array so that we don't incur the EtherRouter overhead for each staker if we looped over
     // it in ReputationMiningCycle.invalidateHash;
-
-    ITokenLocking(tokenLocking).deposit(clnyToken, 0, true); // Faux deposit to clear any locks
     for (uint256 i; i < _stakers.length; i++) {
       lostStake = min(miningStakes[_stakers[i]].amount, _amount);
       miningStakes[_stakers[i]].amount = sub(miningStakes[_stakers[i]].amount, lostStake);
+    }
+
+    ITokenLocking(tokenLocking).deposit(clnyToken, 0, true); // Faux deposit to clear any locks
+    // Do all the external calls after all the storage changes
+    for (uint256 i; i < _stakers.length; i++) {
       ITokenLocking(tokenLocking).transferStake(_stakers[i], lostStake, clnyToken, address(this));
       // TODO: Lose rep?
       emit ReputationMinerPenalised(_stakers[i], lostStake);

contracts/common/TokenAuthority.sol

@@ -21,7 +21,7 @@ import "../../lib/dappsys/auth.sol";
 
 
 contract TokenAuthority is DSAuthority {
-  address public token;
+  address public immutable token;
   mapping(address => mapping(bytes4 => bool)) authorizations;
 
   bytes4 constant BURN_FUNC_SIG = bytes4(keccak256("burn(uint256)"));
@@ -38,7 +38,7 @@ contract TokenAuthority is DSAuthority {
     authorizations[_colony][mintSig] = true;
     authorizations[_colony][mintSigOverload] = true;
 
-    for (uint i = 0; i < allowedToTransfer.length; i++) {
+    for (uint256 i = 0; i < allowedToTransfer.length; i++) {
       authorizations[allowedToTransfer[i]][transferSig] = true;
       authorizations[allowedToTransfer[i]][transferFromSig] = true;
     }

contracts/metaTxToken/MetaTxToken.sol

@@ -21,12 +21,16 @@ import "./DSTokenBaseMeta.sol";
 import "./DSAuthMeta.sol";
 
 contract MetaTxToken is DSTokenBaseMeta(0), DSAuthMeta {
-  uint8 public decimals;
+  uint8 public immutable decimals;
+  // Immutable-states incorrectly flags strings as being able to be immutable.
+  // Can be removed when https://github.com/crytic/slither/issues/1595 is fixed
+  // slither-disable-next-line immutable-states
   string public symbol;
+  // slither-disable-next-line immutable-states
   string public name;
 
   bool public locked;
-  bytes32 public DOMAIN_SEPARATOR;
+  bytes32 public immutable DOMAIN_SEPARATOR;
 
   mapping(address => uint256) metatransactionNonces;
 

contracts/patriciaTree/PatriciaTreeBase.sol

@@ -56,7 +56,7 @@ contract PatriciaTreeBase is PatriciaTreeProofs {
     }
     if (numSiblings > 0) {
       _siblings = new bytes32[](numSiblings);
-      for (uint i = 0; i < numSiblings; i++) {
+      for (uint256 i = 0; i < numSiblings; i++) {
         _siblings[i] = siblings[i];
       }
     }

contracts/patriciaTree/PatriciaTreeProofs.sol

@@ -42,7 +42,7 @@ contract PatriciaTreeProofs {
     e.node = valueHash;
     bytes32[2] memory edgeHashes;
 
-    for (uint i = 0; i < siblings.length; i++) {
+    for (uint256 i = 0; i < siblings.length; i++) {
       uint bitSet = branchMask.lowestBitSet();
       branchMask &= ~(uint(1) << bitSet);
       (k, e.label) = k.splitAt(255 - bitSet);

contracts/reputationMiningCycle/ReputationMiningCycle.sol

@@ -149,7 +149,7 @@ contract ReputationMiningCycle is ReputationMiningCycleCommon {
     if (!submissionWindowClosed()) {
       return false;
     }
-    for (uint i = firstIncompleteRound; i <= _round; i += 1) {
+    for (uint256 i = firstIncompleteRound; i <= _round; i += 1) {
       if (nHashesCompletedChallengeRound[i] != disputeRounds[i].length) {
         return false;
       }

helpers/constants.js

@@ -13,7 +13,7 @@ const INT256_MIN = new BN(2).pow(new BN(255)).mul(new BN(-1));
 const INT128_MAX = new BN(2).pow(new BN(127)).sub(new BN(1));
 const INT128_MIN = new BN(2).pow(new BN(127)).mul(new BN(-1));
 
-const CURR_VERSION = 11;
+const CURR_VERSION = 12;
 
 const RECOVERY_ROLE = 0;
 const ROOT_ROLE = 1;

packages/metatransaction-broadcaster/Dockerfile

@@ -3,8 +3,8 @@ COPY ./packages ./packages
 COPY ./package.json ./
 COPY ./package-lock.json ./
 COPY ./build ./build
-RUN npm ci
+RUN yarn
 RUN cd ./packages/metatransaction-broadcaster/ && npm i
 RUN cd ./packages/package-utils/ && npm i
 EXPOSE 3000
-CMD node $NODE_ARGS packages/metatransaction-broadcaster/bin/index.js --colonyNetworkAddress $COLONYNETWORK_ADDRESS --privateKey $PRIVATE_KEY --gasLimit $GASLIMIT $ARGS
+CMD node $NODE_ARGS packages/metatransaction-broadcaster/bin/index.js --colonyNetworkAddress $COLONYNETWORK_ADDRESS --privateKey $PRIVATE_KEY $ARGS

packages/metatransaction-broadcaster/MetatransactionBroadcaster.js

@@ -6,6 +6,10 @@ const queue = require("express-queue");
 const NonceManager = require("./ExtendedNonceManager");
 const { colonyIOCors, ConsoleAdapter, updateGasEstimate } = require("../package-utils");
 
+const ETHEREUM_BRIDGE_ADDRESS = "0x75Df5AF045d91108662D8080fD1FEFAd6aA0bb59";
+const BINANCE_BRIDGE_ADDRESS = "0x162E898bD0aacB578C8D5F8d6ca588c13d2A383F";
+const REQUIRE_TO_PASS_MESSAGE_SIG = ethers.utils.keccak256(ethers.utils.toUtf8Bytes("requireToPassMessage(address,bytes,uint256)")).slice(0, 10);
+
 class MetatransactionBroadcaster {
   /**
    * Constructor for MetatransactionBroadcaster
@@ -194,14 +198,35 @@ class MetatransactionBroadcaster {
     const possibleColony = new ethers.Contract(target, colonyDef.abi, this.wallet);
     try {
       const tx = possibleColony.interface.parseTransaction({ data: txData });
-      if (tx.signature === "makeArbitraryTransaction(address,bytes)") {
-        return false;
-      }
-      if (tx.signature === "makeArbitraryTransactions(address[],bytes[],bool)") {
-        return false;
-      }
-      if (tx.signature === "makeSingleArbitraryTransaction(address,bytes)") {
-        return false;
+      // If it's an arbitrary transaction...
+      if (
+        tx.signature === "makeArbitraryTransaction(address,bytes)" ||
+        tx.signature === "makeSingleArbitraryTransaction(address,bytes)" ||
+        tx.signature === "makeArbitraryTransactions(address[],bytes[],bool)"
+      ) {
+        // We allow it if these transactions are going only to known bridges.
+        let addresses = tx.args[0];
+        let calls = tx.args[1];
+
+        if (!Array.isArray(addresses)) {
+          addresses = [addresses];
+        }
+        if (!Array.isArray(calls)) {
+          calls = [calls];
+        }
+        if (addresses.length !== calls.length) {
+          // If the arrays don't line up, return false.
+          return false;
+        }
+
+        for (let i = 0; i < addresses.length; i += 1) {
+          if (
+            !MetatransactionBroadcaster.isBridgeAddress(addresses[i]) || // It's not going to a bridge address
+            calls[i].slice(0, 10) !== REQUIRE_TO_PASS_MESSAGE_SIG // Or it is, but not calling the function we allow
+          ) {
+            return false;
+          }
+        }
       }
     } catch (err) {
       // Not a colony related transaction (we recognise)
@@ -256,6 +281,19 @@ class MetatransactionBroadcaster {
     return true;
   }
 
+  static isBridgeAddress(targetOrTargets) {
+    if (targetOrTargets === ETHEREUM_BRIDGE_ADDRESS) {
+      return true;
+    }
+    if (targetOrTargets === BINANCE_BRIDGE_ADDRESS) {
+      return true;
+    }
+    if (Array.isArray(targetOrTargets)) {
+      return targetOrTargets.map((x) => MetatransactionBroadcaster.isBridgeAddress(x)).every((x) => x === true);
+    }
+    return false;
+  }
+
   async isValidSetAuthorityTransaction(tx, userAddress) {
     // Get the most recent metatx this user sent on colonyNetwork
     let logs = await this.provider.getLogs({

packages/package-utils/updateGasEstimate.js

@@ -40,7 +40,7 @@ const updateGasEstimate = async function (_type, chainId, adapter) {
     const gasEstimates = await axios.request(options);
     let gasPrice;
     if (gasEstimates[type]) {
-      gasPrice = ethers.utils.hexlify((gasEstimates[type] / factor) * 1e9);
+      gasPrice = ethers.utils.hexlify(ethers.BigNumber.from(gasEstimates[type] * 1e9).div(factor));
     } else {
       gasPrice = defaultGasPrice;
     }