diff --git a/contracts/core/MiniAvatar.sol b/contracts/core/MiniAvatar.sol new file mode 100644 index 00000000..b6c99e28 --- /dev/null +++ b/contracts/core/MiniAvatar.sol @@ -0,0 +1,143 @@ +// SPDX-License-Identifier: LGPL-3.0-only +pragma solidity >=0.7.0 <0.9.0; + +import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol"; +import "../signature/SignatureChecker.sol"; + +abstract contract MiniAvatar is SignatureChecker, OwnableUpgradeable { + event EnabledModule(address module); + event DisabledModule(address module); + event ExecutionFromModuleSuccess(address indexed module); + event ExecutionFromModuleFailure(address indexed module); + + address internal constant SENTINEL_MODULES = address(0x1); + /// Mapping of modules. + mapping(address => address) internal modules; + + /// `sender` is not an authorized module. + /// @param sender The address of the sender. + error NotAuthorized(address sender); + + /// `module` is invalid. + error InvalidModule(address module); + + /// `pageSize` is invalid. + error InvalidPageSize(); + + /// `module` is already disabled. + error AlreadyDisabledModule(address module); + + /// `module` is already enabled. + error AlreadyEnabledModule(address module); + + /// @dev `setModules()` was already called. + error SetupModulesAlreadyCalled(); + + /* + -------------------------------------------------- + */ + + modifier moduleOnly() { + if (modules[msg.sender] == address(0)) { + if (modules[moduleTxSignedBy()] == address(0)) { + revert NotAuthorized(msg.sender); + } + moduleTxNonceBump(); + } + + _; + } + + /// @dev Disables a module. + /// @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 onlyOwner { + if (module == address(0) || module == SENTINEL_MODULES) + revert InvalidModule(module); + if (modules[prevModule] != module) revert AlreadyDisabledModule(module); + modules[prevModule] = modules[module]; + modules[module] = address(0); + emit DisabledModule(module); + } + + /// @dev Enables a module that can add transactions to the queue + /// @param module Address of the module to be enabled + /// @notice This can only be called by the owner + function enableModule(address module) public onlyOwner { + if (module == address(0) || module == SENTINEL_MODULES) + revert InvalidModule(module); + if (modules[module] != address(0)) revert AlreadyEnabledModule(module); + modules[module] = modules[SENTINEL_MODULES]; + modules[SENTINEL_MODULES] = module; + emit EnabledModule(module); + } + + /// @dev Returns if an module is enabled + /// @return True if the module is enabled + function isModuleEnabled(address _module) public view returns (bool) { + return SENTINEL_MODULES != _module && modules[_module] != address(0); + } + + /// @dev Returns array of modules. + /// If all entries fit into a single page, the next pointer will be 0x1. + /// If another page is present, next will be the last element of the returned array. + /// @param start Start of the page. Has to be a module or start pointer (0x1 address) + /// @param pageSize Maximum number of modules that should be returned. Has to be > 0 + /// @return array Array of modules. + /// @return next Start of the next page. + function getModulesPaginated( + address start, + uint256 pageSize + ) external view returns (address[] memory array, address next) { + if (start != SENTINEL_MODULES && !isModuleEnabled(start)) { + revert InvalidModule(start); + } + if (pageSize == 0) { + revert InvalidPageSize(); + } + + // Init array with max page size + array = new address[](pageSize); + + // Populate return array + uint256 moduleCount = 0; + next = modules[start]; + while ( + next != address(0) && + next != SENTINEL_MODULES && + moduleCount < pageSize + ) { + array[moduleCount] = next; + next = modules[next]; + moduleCount++; + } + + // Because of the argument validation we can assume that + // the `currentModule` will always be either a module address + // or sentinel address (aka the end). If we haven't reached the end + // inside the loop, we need to set the next pointer to the last element + // because it skipped over to the next module which is neither included + // in the current page nor won't be included in the next one + // if you pass it as a start. + if (next != SENTINEL_MODULES) { + next = array[moduleCount - 1]; + } + // Set correct size of returned array + // solhint-disable-next-line no-inline-assembly + assembly { + 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/Modifier.sol b/contracts/core/Modifier.sol index 160898c1..b1153de4 100644 --- a/contracts/core/Modifier.sol +++ b/contracts/core/Modifier.sol @@ -1,36 +1,13 @@ // SPDX-License-Identifier: LGPL-3.0-only -/// @title Modifier Interface - A contract that sits between a Module and an Avatar and enforce some additional logic. pragma solidity >=0.7.0 <0.9.0; -import "./Module.sol"; import "../interfaces/IAvatar.sol"; -import "../signature/SignatureChecker.sol"; - -abstract contract Modifier is Module, IAvatar, SignatureChecker { - address internal constant SENTINEL_MODULES = address(0x1); - /// Mapping of modules. - mapping(address => address) internal modules; - - /// `sender` is not an authorized module. - /// @param sender The address of the sender. - error NotAuthorized(address sender); - - /// `module` is invalid. - error InvalidModule(address module); - - /// `pageSize` is invalid. - error InvalidPageSize(); - - /// `module` is already disabled. - error AlreadyDisabledModule(address module); - - /// `module` is already enabled. - error AlreadyEnabledModule(address module); - - /// @dev `setModules()` was already called. - error SetupModulesAlreadyCalled(); +import "./MiniAvatar.sol"; +import "./Module.sol"; +/// @title Modifier Interface - A contract that sits between a Module and an Avatar and enforces some additional logic. +abstract contract Modifier is Module, MiniAvatar, IAvatar { /* -------------------------------------------------- You must override at least one of following two virtual functions, @@ -48,7 +25,7 @@ abstract contract Modifier is Module, IAvatar, SignatureChecker { uint256 value, bytes calldata data, Enum.Operation operation - ) public virtual override moduleOnly returns (bool success) {} + ) public virtual override onlyOwner returns (bool success) {} /// @dev Passes a transaction to the modifier, expects return data. /// @notice Can only be called by enabled modules. @@ -65,117 +42,7 @@ abstract contract Modifier is Module, IAvatar, SignatureChecker { public virtual override - moduleOnly + onlyOwner returns (bool success, bytes memory returnData) {} - - /* - -------------------------------------------------- - */ - - modifier moduleOnly() { - if (modules[msg.sender] == address(0)) { - if (modules[moduleTxSignedBy()] == address(0)) { - revert NotAuthorized(msg.sender); - } - moduleTxNonceBump(); - } - - _; - } - - /// @dev Disables a module on the modifier. - /// @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 { - if (module == address(0) || module == SENTINEL_MODULES) - revert InvalidModule(module); - if (modules[prevModule] != module) revert AlreadyDisabledModule(module); - modules[prevModule] = modules[module]; - modules[module] = address(0); - emit DisabledModule(module); - } - - /// @dev Enables a module that can add transactions to the queue - /// @param module Address of the module to be enabled - /// @notice This can only be called by the owner - function enableModule(address module) public override onlyOwner { - if (module == address(0) || module == SENTINEL_MODULES) - revert InvalidModule(module); - if (modules[module] != address(0)) revert AlreadyEnabledModule(module); - modules[module] = modules[SENTINEL_MODULES]; - modules[SENTINEL_MODULES] = module; - emit EnabledModule(module); - } - - /// @dev Returns if an module is enabled - /// @return True if the module is enabled - function isModuleEnabled( - address _module - ) public view override returns (bool) { - return SENTINEL_MODULES != _module && modules[_module] != address(0); - } - - /// @dev Returns array of modules. - /// If all entries fit into a single page, the next pointer will be 0x1. - /// If another page is present, next will be the last element of the returned array. - /// @param start Start of the page. Has to be a module or start pointer (0x1 address) - /// @param pageSize Maximum number of modules that should be returned. Has to be > 0 - /// @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) { - if (start != SENTINEL_MODULES && !isModuleEnabled(start)) { - revert InvalidModule(start); - } - if (pageSize == 0) { - revert InvalidPageSize(); - } - - // Init array with max page size - array = new address[](pageSize); - - // Populate return array - uint256 moduleCount = 0; - next = modules[start]; - while ( - next != address(0) && - next != SENTINEL_MODULES && - moduleCount < pageSize - ) { - array[moduleCount] = next; - next = modules[next]; - moduleCount++; - } - - // Because of the argument validation we can assume that - // the `currentModule` will always be either a module address - // or sentinel address (aka the end). If we haven't reached the end - // inside the loop, we need to set the next pointer to the last element - // because it skipped over to the next module which is neither included - // in the current page nor won't be included in the next one - // if you pass it as a start. - if (next != SENTINEL_MODULES) { - next = array[moduleCount - 1]; - } - // Set correct size of returned array - // solhint-disable-next-line no-inline-assembly - assembly { - 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/ModifierGuardable.sol b/contracts/core/ModifierGuardable.sol new file mode 100644 index 00000000..f2aa7261 --- /dev/null +++ b/contracts/core/ModifierGuardable.sol @@ -0,0 +1,47 @@ +// SPDX-License-Identifier: LGPL-3.0-only +pragma solidity >=0.7.0 <0.9.0; + +import "../interfaces/IAvatar.sol"; +import "./MiniAvatar.sol"; +import "./ModuleGuardable.sol"; + +/// @title Modifier Interface - A contract that sits between a Module and an Avatar and enforce some additional logic. +abstract contract ModifierGuardable is ModuleGuardable, MiniAvatar, IAvatar { + /* + -------------------------------------------------- + You must override at least one of following two virtual functions, + execTransactionFromModule() and execTransactionFromModuleReturnData(). + */ + + /// @dev Passes a transaction to the modifier. + /// @notice Can only be called by enabled modules. + /// @param to Destination address of module transaction. + /// @param value Ether value of module transaction. + /// @param data Data payload of module transaction. + /// @param operation Operation type of module transaction. + function execTransactionFromModule( + address to, + uint256 value, + bytes calldata data, + Enum.Operation operation + ) public virtual override onlyOwner returns (bool success) {} + + /// @dev Passes a transaction to the modifier, expects return data. + /// @notice Can only be called by enabled modules. + /// @param to Destination address of module transaction. + /// @param value Ether value of module transaction. + /// @param data Data payload of module transaction. + /// @param operation Operation type of module transaction. + function execTransactionFromModuleReturnData( + address to, + uint256 value, + bytes calldata data, + Enum.Operation operation + ) + public + virtual + override + onlyOwner + returns (bool success, bytes memory returnData) + {} +} diff --git a/contracts/core/Module.sol b/contracts/core/Module.sol index 04b292a8..dbf37f95 100644 --- a/contracts/core/Module.sol +++ b/contracts/core/Module.sol @@ -1,13 +1,11 @@ // SPDX-License-Identifier: LGPL-3.0-only - -/// @title Module Interface - A contract that can pass messages to a Module Manager contract if enabled by that contract. pragma solidity >=0.7.0 <0.9.0; -import "../interfaces/IAvatar.sol"; import "../factory/FactoryFriendly.sol"; -import "../guard/Guardable.sol"; +import "../interfaces/IAvatar.sol"; -abstract contract Module is FactoryFriendly, Guardable { +/// @title Module Interface - A contract that can pass messages to a Module Manager contract if enabled by that contract. +abstract contract Module is FactoryFriendly { /// @dev Address that will ultimately execute function calls. address public avatar; /// @dev Address that this module will pass transactions to. @@ -45,8 +43,14 @@ abstract contract Module is FactoryFriendly, Guardable { uint256 value, bytes memory data, Enum.Operation operation - ) internal returns (bool success) { - (success, ) = _exec(to, value, data, operation); + ) internal virtual returns (bool success) { + return + IAvatar(target).execTransactionFromModule( + to, + value, + data, + operation + ); } /// @dev Passes a transaction to be executed by the target and returns data. @@ -60,49 +64,13 @@ abstract contract Module is FactoryFriendly, Guardable { uint256 value, bytes memory data, Enum.Operation operation - ) internal returns (bool success, bytes memory returnData) { - (success, returnData) = _exec(to, value, data, operation); - } - - function _exec( - address to, - uint256 value, - bytes memory data, - Enum.Operation operation - ) private returns (bool success, bytes memory returnData) { - address currentGuard = guard; - if (currentGuard != address(0)) { - IGuard(currentGuard).checkTransaction( - /// Transaction info used by module transactions. + ) internal virtual returns (bool success, bytes memory returnData) { + return + IAvatar(target).execTransactionFromModuleReturnData( to, value, data, - operation, - /// Zero out the redundant transaction information only used for Safe multisig transctions. - 0, - 0, - 0, - address(0), - payable(0), - "", - msg.sender + operation ); - (success, returnData) = IAvatar(target) - .execTransactionFromModuleReturnData( - to, - value, - data, - operation - ); - IGuard(currentGuard).checkAfterExecution("", success); - } else { - (success, returnData) = IAvatar(target) - .execTransactionFromModuleReturnData( - to, - value, - data, - operation - ); - } } } diff --git a/contracts/core/ModuleGuardable.sol b/contracts/core/ModuleGuardable.sol new file mode 100644 index 00000000..48a1d9ea --- /dev/null +++ b/contracts/core/ModuleGuardable.sol @@ -0,0 +1,89 @@ +// SPDX-License-Identifier: LGPL-3.0-only + +/// @title Module Interface - A contract that can pass messages to a Module Manager contract if enabled by that contract. +pragma solidity >=0.7.0 <0.9.0; + +import "../guard/Guardable.sol"; +import "./Module.sol"; + +abstract contract ModuleGuardable is Module, Guardable { + /// @dev Passes a transaction to be executed by the avatar. + /// @notice Can only be called by this contract. + /// @param to Destination address of module transaction. + /// @param value Ether value of module transaction. + /// @param data Data payload of module transaction. + /// @param operation Operation type of module transaction: 0 == call, 1 == delegate call. + function exec( + address to, + uint256 value, + bytes memory data, + Enum.Operation operation + ) internal override returns (bool success) { + address currentGuard = guard; + if (currentGuard != address(0)) { + IGuard(currentGuard).checkTransaction( + /// Transaction info used by module transactions. + to, + value, + data, + operation, + /// Zero out the redundant transaction information only used for Safe multisig transctions. + 0, + 0, + 0, + address(0), + payable(0), + "", + msg.sender + ); + } + success = IAvatar(target).execTransactionFromModule( + to, + value, + data, + operation + ); + if (currentGuard != address(0)) { + IGuard(currentGuard).checkAfterExecution("", success); + } + } + + /// @dev Passes a transaction to be executed by the target and returns data. + /// @notice Can only be called by this contract. + /// @param to Destination address of module transaction. + /// @param value Ether value of module transaction. + /// @param data Data payload of module transaction. + /// @param operation Operation type of module transaction: 0 == call, 1 == delegate call. + function execAndReturnData( + address to, + uint256 value, + bytes memory data, + Enum.Operation operation + ) internal override returns (bool success, bytes memory returnData) { + address currentGuard = guard; + if (currentGuard != address(0)) { + IGuard(currentGuard).checkTransaction( + /// Transaction info used by module transactions. + to, + value, + data, + operation, + /// Zero out the redundant transaction information only used for Safe multisig transctions. + 0, + 0, + 0, + address(0), + payable(0), + "", + msg.sender + ); + } + + (success, returnData) = IAvatar(target) + .execTransactionFromModuleReturnData(to, value, data, operation); + + if (currentGuard != address(0)) { + IGuard(currentGuard).checkAfterExecution("", success); + } + } +} diff --git a/contracts/interfaces/IAvatar.sol b/contracts/interfaces/IAvatar.sol index c757611d..d270af46 100644 --- a/contracts/interfaces/IAvatar.sol +++ b/contracts/interfaces/IAvatar.sol @@ -6,25 +6,6 @@ pragma solidity >=0.7.0 <0.9.0; import "@gnosis.pm/safe-contracts/contracts/common/Enum.sol"; interface IAvatar { - event EnabledModule(address module); - event DisabledModule(address module); - event ExecutionFromModuleSuccess(address indexed module); - event ExecutionFromModuleFailure(address indexed module); - - /// @dev Enables a module on the avatar. - /// @notice Can only be called by the avatar. - /// @notice Modules should be stored as a linked list. - /// @notice Must emit EnabledModule(address module) if successful. - /// @param module Module to be enabled. - function enableModule(address module) external; - - /// @dev Disables a module on the avatar. - /// @notice Can only be called by the avatar. - /// @notice Must emit DisabledModule(address module) if successful. - /// @param prevModule Address that pointed to the module to be removed in the linked list - /// @param module Module to be removed. - function disableModule(address prevModule, address module) external; - /// @dev Allows a Module to execute a transaction. /// @notice Can only be called by an enabled module. /// @notice Must emit ExecutionFromModuleSuccess(address module) if successful. @@ -54,18 +35,4 @@ interface IAvatar { bytes memory data, Enum.Operation operation ) external returns (bool success, bytes memory returnData); - - /// @dev Returns if an module is enabled - /// @return True if the module is enabled - function isModuleEnabled(address module) external view returns (bool); - - /// @dev Returns array of modules. - /// @param start Start of the page. - /// @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 - returns (address[] memory array, address next); } diff --git a/contracts/test/TestGuard.sol b/contracts/test/TestGuard.sol index de63f73b..a50e1fe4 100644 --- a/contracts/test/TestGuard.sol +++ b/contracts/test/TestGuard.sol @@ -1,13 +1,7 @@ // SPDX-License-Identifier: LGPL-3.0-only - -/// @title Modifier Interface - A contract that sits between a Module and an Avatar and enforce some additional logic. pragma solidity >=0.7.0 <0.9.0; -import "../guard/BaseGuard.sol"; -import "../factory/FactoryFriendly.sol"; -import "@gnosis.pm/safe-contracts/contracts/GnosisSafe.sol"; -import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol"; -import "../core/Module.sol"; +import "../core/ModuleGuardable.sol"; contract TestGuard is FactoryFriendly, BaseGuard { event PreChecked(bool checked); @@ -46,7 +40,7 @@ contract TestGuard is FactoryFriendly, BaseGuard { function checkAfterExecution(bytes32, bool) public override { require( - Module(module).guard() == address(this), + ModuleGuardable(module).guard() == address(this), "Module cannot remove its own guard." ); emit PostChecked(true); diff --git a/contracts/test/TestModule.sol b/contracts/test/TestModule.sol index 0e8e7e2c..36fa06db 100644 --- a/contracts/test/TestModule.sol +++ b/contracts/test/TestModule.sol @@ -3,9 +3,9 @@ /// @title Modifier Interface - A contract that sits between a Module and an Avatar and enforce some additional logic. pragma solidity >=0.7.0 <0.9.0; -import "../core/Module.sol"; +import "../core/ModuleGuardable.sol"; -contract TestModule is Module { +contract TestModule is ModuleGuardable { constructor(address _avatar, address _target) { bytes memory initParams = abi.encode(_avatar, _target); setUp(initParams); diff --git a/test/01_IAvatar.spec.ts b/test/01_IAvatar.spec.ts index 60efd22c..8fae82e6 100644 --- a/test/01_IAvatar.spec.ts +++ b/test/01_IAvatar.spec.ts @@ -3,15 +3,16 @@ import { loadFixture } from "@nomicfoundation/hardhat-network-helpers"; import { expect } from "chai"; import hre from "hardhat"; +import { TestAvatar__factory } from "../typechain-types"; describe("IAvatar", async () => { async function setupTests() { + const [signer] = await hre.ethers.getSigners(); const Avatar = await hre.ethers.getContractFactory("TestAvatar"); - const avatar = await Avatar.deploy(); - const iAvatar = await hre.ethers.getContractAt("IAvatar", avatar.address); + const avatar = await Avatar.connect(signer).deploy(); + const iAvatar = TestAvatar__factory.connect(avatar.address, signer); const tx = { to: avatar.address, - value: 0, data: "0x", operation: 0, diff --git a/test/03_Modifier.spec.ts b/test/03_Modifier.spec.ts index 7bcb91df..e040338c 100644 --- a/test/03_Modifier.spec.ts +++ b/test/03_Modifier.spec.ts @@ -6,17 +6,22 @@ import { PopulatedTransaction } from "ethers"; import hre from "hardhat"; import typedDataForTransaction from "./typesDataForTransaction"; import { SignerWithAddress } from "@nomiclabs/hardhat-ethers/signers"; -import { TestModifier__factory } from "../typechain-types"; +import { TestAvatar__factory, TestModifier__factory } from "../typechain-types"; describe("Modifier", async () => { const SENTINEL_MODULES = "0x0000000000000000000000000000000000000001"; async function setupTests() { + const [signer] = await hre.ethers.getSigners(); const Avatar = await hre.ethers.getContractFactory("TestAvatar"); - const avatar = await Avatar.deploy(); - const iAvatar = await hre.ethers.getContractAt("IAvatar", avatar.address); + const avatar = await Avatar.connect(signer).deploy(); + const iAvatar = TestAvatar__factory.connect(avatar.address, signer); const Modifier = await hre.ethers.getContractFactory("TestModifier"); - const modifier = await Modifier.deploy(iAvatar.address, iAvatar.address); + const modifier = await Modifier.connect(signer).deploy( + iAvatar.address, + iAvatar.address + ); + await iAvatar.enableModule(modifier.address); const tx = { to: avatar.address, @@ -32,7 +37,7 @@ describe("Modifier", async () => { }; return { iAvatar, - modifier, + modifier: TestModifier__factory.connect(modifier.address, signer), tx, }; } @@ -244,9 +249,9 @@ describe("Modifier", async () => { const { modifier } = await loadFixture(setupTests); const [user1, user2, user3] = await hre.ethers.getSigners(); - await expect(modifier.enableModule(user1.address)); - await expect(modifier.enableModule(user2.address)); - await expect(modifier.enableModule(user3.address)); + await modifier.enableModule(user1.address); + await modifier.enableModule(user2.address); + await modifier.enableModule(user3.address); await expect(await modifier.isModuleEnabled(user1.address)).to.be.true; await expect(await modifier.isModuleEnabled(user2.address)).to.be.true;