Delegatecall to preserve msgSender()

JoinColony · Aug 4, 2022 · 3d45e9d · 3d45e9d
1 parent c9ac604
commit 3d45e9d
Show file tree

Hide file tree

Showing 4 changed files with 35 additions and 13 deletions.
diff --git a/contracts/colony/ColonyExpenditure.sol b/contracts/colony/ColonyExpenditure.sol
@@ -361,7 +361,13 @@ contract ColonyExpenditure is ColonyStorage {
     internal
   {
     for (uint256 i; i < _tokens.length; i++) {
-      IColony(address(this)).setExpenditurePayouts(_id, _slots[i], _tokens[i], _values[i]);
+      (bool success, bytes memory returndata) = address(this).delegatecall(abi.encodeWithSignature("setExpenditurePayouts(uint256,uint256[],address,uint256[])", _id, _slots[i], _tokens[i], _values[i]));
+      if (!success) {
+        if (returndata.length == 0) revert();
+        assembly {
+          revert(add(32, returndata), mload(returndata))
+        }
+      }
     }
   }
 

diff --git a/contracts/colony/ColonyFunding.sol b/contracts/colony/ColonyFunding.sol
@@ -148,7 +148,7 @@ contract ColonyFunding is ColonyStorage { // ignore-swc-123
   public
   stoppable
   expenditureDraft(_id)
-  expenditureOwnerOrSelf(_id)
+  expenditureOnlyOwner(_id)
   {
     setExpenditurePayoutsInternal(_id, _slots, _token, _amounts);
   }
@@ -179,7 +179,7 @@ contract ColonyFunding is ColonyStorage { // ignore-swc-123
   public
   stoppable
   expenditureDraft(_id)
-  expenditureOwnerOrSelf(_id)
+  expenditureOnlyOwner(_id)
   {
     uint256[] memory slots = new uint256[](1);
     slots[0] = _slot;

diff --git a/contracts/colony/ColonyStorage.sol b/contracts/colony/ColonyStorage.sol
@@ -196,11 +196,6 @@ contract ColonyStorage is ColonyDataTypes, ColonyNetworkDataTypes, DSMath, Commo
     _;
   }
 
-  modifier expenditureOwnerOrSelf(uint256 _id) {
-    require(expenditures[_id].owner == msgSender() || address(this) == msgSender(), "colony-expenditure-not-owner-or-self");
-    _;
-  }
-
   modifier expenditureOnlyOwner(uint256 _id) {
     require(expenditures[_id].owner == msgSender(), "colony-expenditure-not-owner");
     _;

diff --git a/test/contracts-network/colony-expenditure.js b/test/contracts-network/colony-expenditure.js
@@ -388,7 +388,7 @@ contract("Colony Expenditure", (accounts) => {
 
     it("should not allow non-owners to update skills or payouts", async () => {
       await checkErrorRevert(colony.setExpenditureSkill(expenditureId, SLOT0, GLOBAL_SKILL_ID), "colony-expenditure-not-owner");
-      await checkErrorRevert(colony.setExpenditurePayout(expenditureId, SLOT0, token.address, WAD), "colony-expenditure-not-owner-or-self");
+      await checkErrorRevert(colony.setExpenditurePayout(expenditureId, SLOT0, token.address, WAD), "colony-expenditure-not-owner");
     });
 
     it("should allow owners to add a slot payout", async () => {
@@ -450,10 +450,7 @@ contract("Colony Expenditure", (accounts) => {
     });
 
     it("should not allow non-owners to set a payout", async () => {
-      await checkErrorRevert(
-        colony.setExpenditurePayout(expenditureId, SLOT0, token.address, WAD, { from: USER }),
-        "colony-expenditure-not-owner-or-self"
-      );
+      await checkErrorRevert(colony.setExpenditurePayout(expenditureId, SLOT0, token.address, WAD, { from: USER }), "colony-expenditure-not-owner");
     });
 
     it("should allow owners to update many values simultaneously", async () => {
@@ -514,6 +511,30 @@ contract("Colony Expenditure", (accounts) => {
       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(
+          expenditureId,
+          [SLOT0, SLOT1, SLOT2],
+          [RECIPIENT, USER, ADMIN],
+          [SLOT1, SLOT2],
+          [GLOBAL_SKILL_ID, GLOBAL_SKILL_ID],
+          [SLOT0, SLOT1],
+          [10, 20],
+          [SLOT0, SLOT2],
+          [WAD.divn(3), WAD.divn(2)],
+          [token.address, otherToken.address],
+          [[SLOT0], [SLOT1, SLOT2]],
+          [
+            [WAD.muln(10), WAD.muln(20)],
+            [WAD.muln(30), WAD.muln(40)],
+          ],
+          { from: ADMIN }
+        ),
+        "colony-expenditure-bad-slots"
+      );
+    });
+
     it("should not allow owners to update many values simultaneously if not owner", async () => {
       await checkErrorRevert(
         colony.setExpenditureValues(expenditureId, [], [], [], [], [], [], [], [], [], [[], []], [[], []], { from: USER }),