Merge pull request #108 from gnosis/add-setupModules()-function-to-Mo…

…difier.sol [A2] Adds a setupModules function to modifier.sol
gnosisguild · Mar 6, 2023 · 4098d62 · 4098d62
2 parents d7b59e5 + 8a30ace
commit 4098d62
Show file tree

Hide file tree

Showing 9 changed files with 99 additions and 73 deletions.
diff --git a/README.md b/README.md
@@ -61,6 +61,7 @@ contract MyModule is Module {
 
 - **[Bridge](https://github.com/gnosis/zodiac-module-bridge)** (developed by [Gnosis Guild](https://twitter.com/gnosisguild)): This module allows an address on one chain to control an avatar on another chain using an Arbitrary Message Bridge (AMB). This enables a DAO on one chain to control assets and interact with systems like a Gnosis Safe on a different chain.
 - **[Exit](https://github.com/gnosis/zodiac-module-exit)** (developed by [Gnosis Guild](https://twitter.com/gnosisguild)): This module allows users to redeem a designated token for a relative share of an avatar's assets, similar to MolochDAO's infamous rageQuit() function.
+- **[Governor](https://github.com/gnosis/zodiac-module-oz-governor/)** (Developed by [Gnosis Guild](https://twitter.com/gnosisguild)): An opinionated implementation of [OpenZeppelin's Governor contracts](https://docs.openzeppelin.com/contracts/4.x/api/governance) designed to be used in a Zodiac-style setup, allowing a Avatar (like a Gnosis Safe) to controlled by on-chain governance similar to [Compound's Governor Alpha and Bravo](https://compound.finance/docs/governance).
 - **[Optimistic Governor](https://docs.outcome.finance/optimistic-governance/what-is-the-optimistic-governor)** (developed by [Outcome Finance](https://www.outcome.finance/): This module allows on-chain executions based on Snapshot proposal results. The module utilizes UMA's optimistic oracle to govern a Gnosis Safe based on a set of rules defined off-chain.
 - **[Reality](https://github.com/gnosis/zodiac-module-reality)** (developed by [Gnosis Guild](https://twitter.com/gnosisguild)): This module allows on-chain execution based on the outcome of events reported by Reality.eth. While built initially to execute Gnosis Safe transactions according to Snapshot proposals, this module is framework agnostic. It can enable proposal execution from just about anywhere. For example, it can bring Discord polls on-chain.
 - **[Safe Minion](https://github.com/HausDAO/MinionSummonerV2/blob/main/contracts/SafeMinion.sol)** (developed by [DAOHaus](https://daohaus.club)): This module allows Moloch DAOs to manage the assets in a Gnosis Safe based on the outcome of v2 Moloch DAO proposals. Safe Minion enables Moloch DAOs to manage collections of NFTs, manage LP positions with AMMs, and initiate any other arbitrary interactions. It enables DAOs that start as a Gnosis Safe to later delegate governance to a Moloch DAO.

diff --git a/audits/ZodiacModifierUpdateFeb2023.pdf b/audits/ZodiacModifierUpdateFeb2023.pdf
diff --git a/contracts/core/Modifier.sol b/contracts/core/Modifier.sol
@@ -24,6 +24,9 @@ abstract contract Modifier is Module, IAvatar {
     /// `module` is already enabled.
     error AlreadyEnabledModule(address module);
 
+    /// @dev `setModules()` was already called.
+    error SetupModulesAlreadyCalled();
+
     /*
     --------------------------------------------------
     You must override at least one of following two virtual functions,
@@ -75,11 +78,10 @@ abstract contract Modifier is Module, IAvatar {
     /// @notice This can only be called by the owner.
     /// @param prevModule Module that pointed to the module to be removed in the linked list.
     /// @param module Module to be removed.
-    function disableModule(address prevModule, address module)
-        public
-        override
-        onlyOwner
-    {
+    function disableModule(
+        address prevModule,
+        address module
+    ) public override onlyOwner {
         if (module == address(0) || module == SENTINEL_MODULES)
             revert InvalidModule(module);
         if (modules[prevModule] != module) revert AlreadyDisabledModule(module);
@@ -102,12 +104,9 @@ abstract contract Modifier is Module, IAvatar {
 
     /// @dev Returns if an module is enabled
     /// @return True if the module is enabled
-    function isModuleEnabled(address _module)
-        public
-        view
-        override
-        returns (bool)
-    {
+    function isModuleEnabled(
+        address _module
+    ) public view override returns (bool) {
         return SENTINEL_MODULES != _module && modules[_module] != address(0);
     }
 
@@ -116,12 +115,10 @@ abstract contract Modifier is Module, IAvatar {
     /// @param pageSize Maximum number of modules that should be returned.
     /// @return array Array of modules.
     /// @return next Start of the next page.
-    function getModulesPaginated(address start, uint256 pageSize)
-        external
-        view
-        override
-        returns (address[] memory array, address next)
-    {
+    function getModulesPaginated(
+        address start,
+        uint256 pageSize
+    ) external view override returns (address[] memory array, address next) {
         /// Init array with max page size.
         array = new address[](pageSize);
 
@@ -144,4 +141,12 @@ abstract contract Modifier is Module, IAvatar {
             mstore(array, moduleCount)
         }
     }
+
+    /// @dev Initializes the modules linked list.
+    /// @notice Should be called as part of the `setUp` / initializing function and can only be called once.
+    function setupModules() internal {
+        if (modules[SENTINEL_MODULES] != address(0))
+            revert SetupModulesAlreadyCalled();
+        modules[SENTINEL_MODULES] = SENTINEL_MODULES;
+    }
 }
diff --git a/contracts/core/Module.sol b/contracts/core/Module.sol
@@ -46,9 +46,9 @@ abstract contract Module is FactoryFriendly, Guardable {
         bytes memory data,
         Enum.Operation operation
     ) internal returns (bool success) {
-        /// Check if a transactioon guard is enabled.
-        if (guard != address(0)) {
-            IGuard(guard).checkTransaction(
+        address currentGuard = guard;
+        if (currentGuard != address(0)) {
+            IGuard(currentGuard).checkTransaction(
                 /// Transaction info used by module transactions.
                 to,
                 value,
@@ -63,15 +63,20 @@ abstract contract Module is FactoryFriendly, Guardable {
                 bytes("0x"),
                 msg.sender
             );
-        }
-        success = IAvatar(target).execTransactionFromModule(
-            to,
-            value,
-            data,
-            operation
-        );
-        if (guard != address(0)) {
-            IGuard(guard).checkAfterExecution(bytes32("0x"), success);
+            success = IAvatar(target).execTransactionFromModule(
+                to,
+                value,
+                data,
+                operation
+            );
+            IGuard(currentGuard).checkAfterExecution(bytes32("0x"), success);
+        } else {
+            success = IAvatar(target).execTransactionFromModule(
+                to,
+                value,
+                data,
+                operation
+            );
         }
         return success;
     }
@@ -88,9 +93,9 @@ abstract contract Module is FactoryFriendly, Guardable {
         bytes memory data,
         Enum.Operation operation
     ) internal returns (bool success, bytes memory returnData) {
-        /// Check if a transactioon guard is enabled.
-        if (guard != address(0)) {
-            IGuard(guard).checkTransaction(
+        address currentGuard = guard;
+        if (currentGuard != address(0)) {
+            IGuard(currentGuard).checkTransaction(
                 /// Transaction info used by module transactions.
                 to,
                 value,
@@ -105,11 +110,22 @@ abstract contract Module is FactoryFriendly, Guardable {
                 bytes("0x"),
                 msg.sender
             );
-        }
-        (success, returnData) = IAvatar(target)
-            .execTransactionFromModuleReturnData(to, value, data, operation);
-        if (guard != address(0)) {
-            IGuard(guard).checkAfterExecution(bytes32("0x"), success);
+            (success, returnData) = IAvatar(target)
+                .execTransactionFromModuleReturnData(
+                    to,
+                    value,
+                    data,
+                    operation
+                );
+            IGuard(currentGuard).checkAfterExecution(bytes32("0x"), success);
+        } else {
+            (success, returnData) = IAvatar(target)
+                .execTransactionFromModuleReturnData(
+                    to,
+                    value,
+                    data,
+                    operation
+                );
         }
         return (success, returnData);
     }

diff --git a/contracts/test/TestModifier.sol b/contracts/test/TestModifier.sol
@@ -73,6 +73,7 @@ contract TestModifier is Modifier {
     }
 
     function setUp(bytes memory initializeParams) public override initializer {
+        setupModules();
         __Ownable_init();
         (address _avatar, address _target) = abi.decode(
             initializeParams,
@@ -81,4 +82,8 @@ contract TestModifier is Modifier {
         avatar = _avatar;
         target = _target;
     }
+
+    function attemptToSetupModules() public {
+        setupModules();
+    }
 }
diff --git a/hardhat.config.ts b/hardhat.config.ts
@@ -55,7 +55,18 @@ export default {
     sources: "contracts",
   },
   solidity: {
-    compilers: [{ version: "0.8.6" }, { version: "0.6.12" }],
+    compilers: [
+      {
+        version: "0.8.6",
+        settings: {
+          optimizer: {
+            enabled: true,
+            runs: 1000,
+          },
+        },
+      },
+      { version: "0.6.12" },
+    ],
   },
   networks: {
     mainnet: {

diff --git a/package.json b/package.json
@@ -1,6 +1,6 @@
 {
   "name": "@gnosis.pm/zodiac",
-  "version": "3.1.0",
+  "version": "3.2.0",
   "description": "Zodiac is a composable design philosophy and collection of standards for building DAO ecosystem tooling.",
   "author": "Auryn Macmillan <[email protected]>",
   "license": "LGPL-3.0+",
@@ -86,8 +86,8 @@
   "dependencies": {
     "@gnosis.pm/mock-contract": "^4.0.0",
     "@gnosis.pm/safe-contracts": "1.3.0",
-    "@openzeppelin/contracts": "^4.3.2",
-    "@openzeppelin/contracts-upgradeable": "^4.2.0",
+    "@openzeppelin/contracts": "^4.8.1",
+    "@openzeppelin/contracts-upgradeable": "^4.8.1",
     "ethers": "^5.7.1"
   },
   "peerDependencies": {

diff --git a/test/03_Modifier.spec.ts b/test/03_Modifier.spec.ts
@@ -35,6 +35,14 @@ describe("Modifier", async () => {
     };
   });
 
+  describe("setupModules", async () => {
+    it("reverts if called more than once", async () => {
+      const { modifier } = await setupTests();
+      await expect(modifier.attemptToSetupModules()).to.be.revertedWith(
+        "SetupModulesAlreadyCalled()"
+      );
+    });
+  });
   describe("enableModule", async () => {
     it("reverts if caller is not the owner", async () => {
       const { modifier } = await setupTests();
@@ -59,9 +67,6 @@ describe("Modifier", async () => {
 
     it("reverts if module is already enabled", async () => {
       const { modifier } = await setupTests();
-      await expect(modifier.enableModule(user1.address))
-        .to.emit(modifier, "EnabledModule")
-        .withArgs(user1.address);
       await expect(modifier.enableModule(user1.address))
         .to.emit(modifier, "EnabledModule")
         .withArgs(user1.address);
@@ -173,9 +178,7 @@ describe("Modifier", async () => {
       const { modifier } = await setupTests();
       let tx = await modifier.getModulesPaginated(SENTINEL_MODULES, 3);
       tx = tx.toString();
-      await expect(tx).to.be.equals(
-        [[], "0x0000000000000000000000000000000000000000"].toString()
-      );
+      await expect(tx).to.be.equals([[], SENTINEL_MODULES].toString());
     });
 
     it("returns one module if one module is enabled", async () => {
@@ -184,10 +187,7 @@ describe("Modifier", async () => {
       let tx = await modifier.getModulesPaginated(SENTINEL_MODULES, 3);
       tx = tx.toString();
       await expect(tx).to.be.equals(
-        [
-          [user1.address],
-          "0x0000000000000000000000000000000000000000",
-        ].toString()
+        [[user1.address], SENTINEL_MODULES].toString()
       );
     });
 
@@ -203,11 +203,7 @@ describe("Modifier", async () => {
       let tx = await modifier.getModulesPaginated(SENTINEL_MODULES, 3);
       tx = tx.toString();
       await expect(tx).to.be.equals(
-        [
-          user2.address,
-          user1.address,
-          "0x0000000000000000000000000000000000000000",
-        ].toString()
+        [user2.address, user1.address, SENTINEL_MODULES].toString()
       );
     });
   });
@@ -229,10 +225,6 @@ describe("Modifier", async () => {
 
     it("execute a transaction.", async () => {
       const { modifier, tx } = await setupTests();
-      await expect(await modifier.enableModule(user1.address))
-        .to.emit(modifier, "EnabledModule")
-        .withArgs(user1.address);
-      // delete once you figure out why you need to do this twice
       await expect(await modifier.enableModule(user1.address))
         .to.emit(modifier, "EnabledModule")
         .withArgs(user1.address);
@@ -265,10 +257,6 @@ describe("Modifier", async () => {
 
     it("execute a transaction.", async () => {
       const { modifier, tx } = await setupTests();
-      await expect(await modifier.enableModule(user1.address))
-        .to.emit(modifier, "EnabledModule")
-        .withArgs(user1.address);
-      // delete once you figure out why you need to do this twice
       await expect(await modifier.enableModule(user1.address))
         .to.emit(modifier, "EnabledModule")
         .withArgs(user1.address);

diff --git a/yarn.lock b/yarn.lock
@@ -792,15 +792,15 @@
     "@types/sinon-chai" "^3.2.3"
     "@types/web3" "1.0.19"
 
-"@openzeppelin/contracts-upgradeable@^4.2.0":
-  version "4.8.0"
-  resolved "https://registry.yarnpkg.com/@openzeppelin/contracts-upgradeable/-/contracts-upgradeable-4.8.0.tgz#26688982f46969018e3ed3199e72a07c8d114275"
-  integrity sha512-5GeFgqMiDlqGT8EdORadp1ntGF0qzWZLmEY7Wbp/yVhN7/B3NNzCxujuI77ktlyG81N3CUZP8cZe3ZAQ/cW10w==
-
-"@openzeppelin/contracts@^4.3.2":
-  version "4.8.0"
-  resolved "https://registry.yarnpkg.com/@openzeppelin/contracts/-/contracts-4.8.0.tgz#6854c37df205dd2c056bdfa1b853f5d732109109"
-  integrity sha512-AGuwhRRL+NaKx73WKRNzeCxOCOCxpaqF+kp8TJ89QzAipSwZy/NoflkWaL9bywXFRhIzXt8j38sfF7KBKCPWLw==
+"@openzeppelin/contracts-upgradeable@^4.8.1":
+  version "4.8.1"
+  resolved "https://registry.yarnpkg.com/@openzeppelin/contracts-upgradeable/-/contracts-upgradeable-4.8.1.tgz#363f7dd08f25f8f77e16d374350c3d6b43340a7a"
+  integrity sha512-1wTv+20lNiC0R07jyIAbHU7TNHKRwGiTGRfiNnA8jOWjKT98g5OgLpYWOi40Vgpk8SPLA9EvfJAbAeIyVn+7Bw==
+
+"@openzeppelin/contracts@^4.8.1":
+  version "4.8.1"
+  resolved "https://registry.yarnpkg.com/@openzeppelin/contracts/-/contracts-4.8.1.tgz#709cfc4bbb3ca9f4460d60101f15dac6b7a2d5e4"
+  integrity sha512-xQ6eUZl+RDyb/FiZe1h+U7qr/f4p/SrTSQcTPH2bjur3C5DbuW/zFgCU/b1P/xcIaEqJep+9ju4xDRi3rmChdQ==
 
 "@resolver-engine/core@^0.3.3":
   version "0.3.3"