Fix setExpenditureValues with metatransactions

contracts/colony/ColonyExpenditure.sol

@@ -384,14 +384,24 @@ contract ColonyExpenditure is ColonyStorage {
     uint256[][] memory _slots,
     uint256[][] memory _values
   ) internal {
+    bytes memory metatransactionAffix;
+    if (isMetatransaction()) {
+      metatransactionAffix = abi.encodePacked(METATRANSACTION_FLAG, msgSender());
+    }
     for (uint256 i; i < _tokens.length; i++) {
+      // Slither is technically right, but as with multicall (see comment there),
+      // I don't think this is exploitable here.
+      // slither-disable-next-line encode-packed-collision
       (bool success, bytes memory returndata) = address(this).delegatecall(
-        abi.encodeWithSignature(
-          "setExpenditurePayouts(uint256,uint256[],address,uint256[])",
-          _id,
-          _slots[i],
-          _tokens[i],
-          _values[i]
+        abi.encodePacked(
+          abi.encodeWithSignature(
+            "setExpenditurePayouts(uint256,uint256[],address,uint256[])",
+            _id,
+            _slots[i],
+            _tokens[i],
+            _values[i]
+          ),
+          metatransactionAffix
         )
       );
       if (!success) {

contracts/common/MetaTransactionMsgSender.sol

@@ -5,18 +5,14 @@ import { DSMath } from "../../lib/dappsys/math.sol";
 
 abstract contract MetaTransactionMsgSender is DSMath {
   bytes32 constant METATRANSACTION_FLAG = keccak256("METATRANSACTION");
+  uint256 constant METATRANSACTION_DATA_MIN_LENGTH = 32 + 20;
+  // Where 32 is the length of METATRANSACTION_FLAG in bytes
+  // Where 20 is the length of an address in bytes
 
   function msgSender() internal view returns (address payable sender) {
-    uint256 index = msg.data.length;
-    if (msg.sender == address(this) && index >= 52) {
+    if (isMetatransaction()) {
       bytes memory array = msg.data;
-      bytes32 flag;
-      assembly {
-        flag := mload(add(array, sub(index, 20)))
-      }
-      if (flag != METATRANSACTION_FLAG) {
-        return payable(msg.sender);
-      }
+      uint256 index = msg.data.length;
       assembly {
         // Load the 32 bytes word from memory with the address on the lower 20 bytes, and mask those.
         sender := and(mload(add(array, index)), 0xffffffffffffffffffffffffffffffffffffffff)
@@ -25,4 +21,17 @@ abstract contract MetaTransactionMsgSender is DSMath {
       return payable(msg.sender);
     }
   }
+
+  function isMetatransaction() internal view returns (bool) {
+    uint256 index = msg.data.length;
+    if (msg.sender == address(this) && index >= METATRANSACTION_DATA_MIN_LENGTH) {
+      bytes memory array = msg.data;
+      bytes32 flag;
+      assembly {
+        flag := mload(add(array, sub(index, 20)))
+      }
+      return flag == METATRANSACTION_FLAG;
+    }
+    return false;
+  }
 }

test/contracts-network/colony-expenditure.js

@@ -17,7 +17,7 @@ const {
   CURR_VERSION,
 } = require("../../helpers/constants");
 const { checkErrorRevert, expectEvent, getTokenArgs, forwardTime, getBlockTime, bn2bytes32, upgradeColonyTo } = require("../../helpers/test-helper");
-const { fundColonyWithTokens, setupRandomColony } = require("../../helpers/test-data-generator");
+const { fundColonyWithTokens, setupRandomColony, getMetaTransactionParameters } = require("../../helpers/test-data-generator");
 const { setupEtherRouter } = require("../../helpers/upgradable-contracts");
 const {
   deployColonyVersionGLWSS4,
@@ -559,6 +559,71 @@ contract("Colony Expenditure", (accounts) => {
       expect(payout).to.eq.BN(WAD.muln(40));
     });
 
+    it("should allow setExpenditureValues to be called via multicall", async () => {
+      const txData = await colony.contract.methods
+        .setExpenditureValues(
+          expenditureId,
+          [SLOT0, SLOT1, SLOT2],
+          [RECIPIENT, USER, ADMIN],
+          [SLOT1, SLOT2],
+          [localSkillId, localSkillId],
+          [SLOT0, SLOT1],
+          [10, 20],
+          [SLOT0, SLOT2],
+          [WAD.divn(3), WAD.divn(2)],
+          [token.address, otherToken.address],
+          [
+            [SLOT0, SLOT1],
+            [SLOT1, SLOT2],
+          ],
+          [
+            [WAD.muln(10).toString(), WAD.muln(20).toString()],
+            [WAD.muln(30).toString(), WAD.muln(40).toString()],
+          ],
+        )
+        .encodeABI();
+
+      const { r, s, v } = await getMetaTransactionParameters(txData, ADMIN, colony.address);
+
+      const tx = await colony.executeMetaTransaction(ADMIN, txData, r, s, v, { from: accounts[1] });
+
+      expectEvent(tx, "MetaTransactionExecuted", [ADMIN, accounts[1], txData]);
+
+      let slot;
+      slot = await colony.getExpenditureSlot(expenditureId, SLOT0);
+      expect(slot.recipient).to.equal(RECIPIENT);
+      expect(slot.skills[0]).to.be.zero;
+      expect(slot.claimDelay).to.eq.BN(10);
+      expect(slot.payoutModifier).to.eq.BN(WAD.divn(3));
+
+      slot = await colony.getExpenditureSlot(expenditureId, SLOT1);
+      expect(slot.recipient).to.equal(USER);
+      expect(slot.skills[0]).to.eq.BN(localSkillId);
+      expect(slot.claimDelay).to.eq.BN(20);
+      expect(slot.payoutModifier).to.be.zero;
+
+      slot = await colony.getExpenditureSlot(expenditureId, SLOT2);
+      expect(slot.recipient).to.equal(ADMIN);
+      expect(slot.skills[0]).to.eq.BN(localSkillId);
+      expect(slot.claimDelay).to.be.zero;
+      expect(slot.payoutModifier).to.eq.BN(WAD.divn(2));
+
+      let payout;
+      payout = await colony.getExpenditureSlotPayout(expenditureId, SLOT0, token.address);
+      expect(payout).to.eq.BN(WAD.muln(10));
+      payout = await colony.getExpenditureSlotPayout(expenditureId, SLOT1, token.address);
+      expect(payout).to.eq.BN(WAD.muln(20));
+      payout = await colony.getExpenditureSlotPayout(expenditureId, SLOT2, token.address);
+      expect(payout).to.be.zero;
+
+      payout = await colony.getExpenditureSlotPayout(expenditureId, SLOT0, otherToken.address);
+      expect(payout).to.be.zero;
+      payout = await colony.getExpenditureSlotPayout(expenditureId, SLOT1, otherToken.address);
+      expect(payout).to.eq.BN(WAD.muln(30));
+      payout = await colony.getExpenditureSlotPayout(expenditureId, SLOT2, otherToken.address);
+      expect(payout).to.eq.BN(WAD.muln(40));
+    });
+
     it("should revert with an error even if the delegatecall in setExpenditurePayouts is used and fails", async () => {
       await checkErrorRevert(
         colony.setExpenditureValues(