From fe011fe67013c003eb989963936a785c2a0e14d8 Mon Sep 17 00:00:00 2001 From: Alex Rea Date: Mon, 13 May 2024 17:18:18 +0100 Subject: [PATCH 1/4] Fix setExpendtiureValues with metatransactions --- contracts/colony/ColonyExpenditure.sol | 19 ++++-- contracts/common/MetaTransactionMsgSender.sol | 13 ++++ test/contracts-network/colony-expenditure.js | 67 ++++++++++++++++++- 3 files changed, 92 insertions(+), 7 deletions(-) diff --git a/contracts/colony/ColonyExpenditure.sol b/contracts/colony/ColonyExpenditure.sol index 0c86949293..d99b9c8371 100644 --- a/contracts/colony/ColonyExpenditure.sol +++ b/contracts/colony/ColonyExpenditure.sol @@ -384,14 +384,21 @@ 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++) { (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) { diff --git a/contracts/common/MetaTransactionMsgSender.sol b/contracts/common/MetaTransactionMsgSender.sol index adcdf5aeca..9be6bcbca5 100644 --- a/contracts/common/MetaTransactionMsgSender.sol +++ b/contracts/common/MetaTransactionMsgSender.sol @@ -25,4 +25,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 >= 52) { + bytes memory array = msg.data; + bytes32 flag; + assembly { + flag := mload(add(array, sub(index, 20))) + } + return flag == METATRANSACTION_FLAG; + } + return false; + } } diff --git a/test/contracts-network/colony-expenditure.js b/test/contracts-network/colony-expenditure.js index ba4ee52b0b..5a33e1af32 100644 --- a/test/contracts-network/colony-expenditure.js +++ b/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( From bccd58c1236d1943333bf94ec8540c8eea7690b7 Mon Sep 17 00:00:00 2001 From: Alex Rea Date: Tue, 14 May 2024 06:59:15 +0100 Subject: [PATCH 2/4] BasicMetaTransaction DRY; slither exception --- contracts/colony/ColonyExpenditure.sol | 3 +++ contracts/common/MetaTransactionMsgSender.sol | 11 ++--------- 2 files changed, 5 insertions(+), 9 deletions(-) diff --git a/contracts/colony/ColonyExpenditure.sol b/contracts/colony/ColonyExpenditure.sol index d99b9c8371..ea472a0cd9 100644 --- a/contracts/colony/ColonyExpenditure.sol +++ b/contracts/colony/ColonyExpenditure.sol @@ -389,6 +389,9 @@ contract ColonyExpenditure is ColonyStorage { 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.encodePacked( abi.encodeWithSignature( diff --git a/contracts/common/MetaTransactionMsgSender.sol b/contracts/common/MetaTransactionMsgSender.sol index 9be6bcbca5..909b980a0e 100644 --- a/contracts/common/MetaTransactionMsgSender.sol +++ b/contracts/common/MetaTransactionMsgSender.sol @@ -7,16 +7,9 @@ abstract contract MetaTransactionMsgSender is DSMath { bytes32 constant METATRANSACTION_FLAG = keccak256("METATRANSACTION"); 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) From 2618e178b6ebf6b8842f213cf02f1de60d5d23ce Mon Sep 17 00:00:00 2001 From: Alex Rea Date: Wed, 15 May 2024 17:34:27 +0100 Subject: [PATCH 3/4] Make source of magic 52 clearer --- contracts/common/MetaTransactionMsgSender.sol | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/contracts/common/MetaTransactionMsgSender.sol b/contracts/common/MetaTransactionMsgSender.sol index 909b980a0e..859fbac48b 100644 --- a/contracts/common/MetaTransactionMsgSender.sol +++ b/contracts/common/MetaTransactionMsgSender.sol @@ -5,6 +5,7 @@ import { DSMath } from "../../lib/dappsys/math.sol"; abstract contract MetaTransactionMsgSender is DSMath { bytes32 constant METATRANSACTION_FLAG = keccak256("METATRANSACTION"); + uint256 METATRANSACTION_DATA_MIN_LENGTH = METATRANSACTION_FLAG.length + 20; // Where 20 is the length of an address in bytes function msgSender() internal view returns (address payable sender) { if (isMetatransaction()) { @@ -21,7 +22,7 @@ abstract contract MetaTransactionMsgSender is DSMath { function isMetatransaction() internal view returns (bool) { uint256 index = msg.data.length; - if (msg.sender == address(this) && index >= 52) { + if (msg.sender == address(this) && index >= METATRANSACTION_DATA_MIN_LENGTH) { bytes memory array = msg.data; bytes32 flag; assembly { From 357f7f0643fd995d038d86bdd56b8ba882218e9d Mon Sep 17 00:00:00 2001 From: Alex Rea Date: Thu, 16 May 2024 09:42:37 +0100 Subject: [PATCH 4/4] Remove unintentional storage variable --- contracts/common/MetaTransactionMsgSender.sol | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/contracts/common/MetaTransactionMsgSender.sol b/contracts/common/MetaTransactionMsgSender.sol index 859fbac48b..37bab1327f 100644 --- a/contracts/common/MetaTransactionMsgSender.sol +++ b/contracts/common/MetaTransactionMsgSender.sol @@ -5,7 +5,9 @@ import { DSMath } from "../../lib/dappsys/math.sol"; abstract contract MetaTransactionMsgSender is DSMath { bytes32 constant METATRANSACTION_FLAG = keccak256("METATRANSACTION"); - uint256 METATRANSACTION_DATA_MIN_LENGTH = METATRANSACTION_FLAG.length + 20; // Where 20 is the length of an address in bytes + 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) { if (isMetatransaction()) {