diff --git a/README.md b/README.md index 23bec8d8..ad495563 100644 --- 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 new file mode 100644 index 00000000..061415ea Binary files /dev/null and b/audits/ZodiacModifierUpdateFeb2023.pdf differ diff --git a/contracts/core/Modifier.sol b/contracts/core/Modifier.sol index 41a3fac9..3139a58c 100644 --- 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 index 43419c36..2bad41ec 100644 --- 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 index c7e3c563..dac3c745 100644 --- 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 index 31eae01c..23d09d13 100644 --- 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 index 8eee7139..5615f035 100644 --- 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 ", "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 index 9c06a48e..37907f48 100644 --- 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 index a4ddc174..3bede968 100644 --- 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"