diff --git a/packages/contracts/deploy/update/to_v1.3.0/90_ManagingDAO.ts b/packages/contracts/deploy/update/to_v1.3.0/90_ManagingDAO.ts index 297644a0c..0ce0cb468 100644 --- a/packages/contracts/deploy/update/to_v1.3.0/90_ManagingDAO.ts +++ b/packages/contracts/deploy/update/to_v1.3.0/90_ManagingDAO.ts @@ -1,6 +1,9 @@ import {DAOFactory__factory, DAO__factory} from '../../../typechain'; import {getContractAddress} from '../../helpers'; -import {IMPLICIT_INITIAL_PROTOCOL_VERSION} from '@aragon/osx-commons-sdk'; +import { + getProtocolVersion, + IMPLICIT_INITIAL_PROTOCOL_VERSION, +} from '@aragon/osx-commons-sdk'; import {DeployFunction} from 'hardhat-deploy/types'; import {HardhatRuntimeEnvironment} from 'hardhat/types'; @@ -24,7 +27,8 @@ const func: DeployFunction = async function (hre: HardhatRuntimeEnvironment) { const upgradeTX = await managementDAO.populateTransaction.upgradeToAndCall( newDaoImplementation, managementDAO.interface.encodeFunctionData('initializeFrom', [ - IMPLICIT_INITIAL_PROTOCOL_VERSION, + // Get the managing dao's protocol version from which upgrade will happen. + await getProtocolVersion(managementDAO), [], ]) ); @@ -38,6 +42,8 @@ const func: DeployFunction = async function (hre: HardhatRuntimeEnvironment) { value: 0, description: `Upgrade the management DAO (${managementDAOAddress}) to the new implementation (${newDaoImplementation}).`, }); + + console.log(hre.managementDAOActions); }; export default func; func.tags = ['ManagementDAO', 'v1.3.0']; diff --git a/packages/contracts/hardhat.config.ts b/packages/contracts/hardhat.config.ts index 27ca02c0e..23cc26336 100644 --- a/packages/contracts/hardhat.config.ts +++ b/packages/contracts/hardhat.config.ts @@ -12,13 +12,16 @@ import '@openzeppelin/hardhat-upgrades'; import * as dotenv from 'dotenv'; import 'hardhat-deploy'; import 'hardhat-gas-reporter'; -import {extendEnvironment, HardhatUserConfig} from 'hardhat/config'; +import {extendEnvironment, HardhatUserConfig, task} from 'hardhat/config'; import type {NetworkUserConfig} from 'hardhat/types'; import 'solidity-coverage'; import 'solidity-docgen'; +import util from 'util'; dotenv.config(); +const exec = util.promisify(require('child_process').exec); + const ETH_KEY = process.env.ETH_KEY; const accounts = ETH_KEY ? ETH_KEY.split(',') : []; @@ -53,6 +56,15 @@ extendEnvironment(hre => { hre.testingFork = testingFork; }); +// Needed to override and extend in order to make coverage work with typechain +// Without this, it doesn't generate typechains the way we want with custom scripts. +task('coverage').setAction(async (args, hre, runSuper) => { + await exec('yarn typechain:osx'); + await exec('yarn typechain:osx-versions'); + + await runSuper(); +}); + const ENABLE_DEPLOY_TEST = process.env.TEST_UPDATE_DEPLOY_SCRIPT !== undefined; console.log('Is deploy test is enabled: ', ENABLE_DEPLOY_TEST); diff --git a/packages/contracts/scripts/generate-typechain-osx.ts b/packages/contracts/scripts/generate-typechain-osx.ts index 40bb38b2b..1678fce80 100644 --- a/packages/contracts/scripts/generate-typechain-osx.ts +++ b/packages/contracts/scripts/generate-typechain-osx.ts @@ -28,7 +28,12 @@ async function generateTypechain(): Promise { } const dirPath = path.dirname(filePath); - if (!excludedDirs.has(dirPath)) { + + // Only arrange the contracts' paths in the current + // directory without previous versions. + const containsPrefix = Array.from(excludedDirs).some(prefix => dirPath.startsWith(prefix)); + + if (!containsPrefix) { jsonFiles.push(filePath); } }); diff --git a/packages/contracts/src/core/dao/DAO.sol b/packages/contracts/src/core/dao/DAO.sol index 345bb6d70..fbf5106c5 100644 --- a/packages/contracts/src/core/dao/DAO.sol +++ b/packages/contracts/src/core/dao/DAO.sol @@ -18,6 +18,7 @@ import {ProtocolVersion} from "@aragon/osx-commons-contracts/src/utils/versionin import {VersionComparisonLib} from "@aragon/osx-commons-contracts/src/utils/versioning/VersionComparisonLib.sol"; import {hasBit, flipBit} from "@aragon/osx-commons-contracts/src/utils/math/BitMap.sol"; import {IDAO} from "@aragon/osx-commons-contracts/src/dao/IDAO.sol"; +import {IExecutor, Action} from "@aragon/osx-commons-contracts/src/executors/IExecutor.sol"; import {PermissionManager} from "../permission/PermissionManager.sol"; import {CallbackHandler} from "../utils/CallbackHandler.sol"; @@ -28,12 +29,14 @@ import {IEIP4824} from "./IEIP4824.sol"; /// @notice This contract is the entry point to the Aragon DAO framework and provides our users a simple and easy to use public interface. /// @dev Public API of the Aragon DAO framework. /// @custom:security-contact sirt@aragon.org +/// @custom:oz-upgrades-unsafe-allow constructor constructor delegatecall contract DAO is IEIP4824, Initializable, IERC1271, ERC165StorageUpgradeable, IDAO, + IExecutor, UUPSUpgradeable, ProtocolVersion, PermissionManager, @@ -60,6 +63,9 @@ contract DAO is bytes32 public constant REGISTER_STANDARD_CALLBACK_PERMISSION_ID = keccak256("REGISTER_STANDARD_CALLBACK_PERMISSION"); + /// @notice The ID of the permission that allows to withdraw native eth by allowed entities. + bytes32 private constant ETH_TRANSFER_PERMISSION_ID = keccak256(""); + /// @notice The ID of the permission required to validate [ERC-1271](https://eips.ethereum.org/EIPS/eip-1271) signatures. bytes32 public constant VALIDATE_SIGNATURE_PERMISSION_ID = keccak256("VALIDATE_SIGNATURE_PERMISSION"); @@ -117,6 +123,9 @@ contract DAO is /// @notice Thrown when a function is removed but left to not corrupt the interface ID. error FunctionRemoved(); + /// @notice Thrown when initialize is called after it has already been executed. + error AlreadyInitialized(); + /// @notice Emitted when a new DAO URI is set. /// @param daoURI The new URI. event NewURI(string daoURI); @@ -134,8 +143,16 @@ contract DAO is _reentrancyStatus = _NOT_ENTERED; } + /// @notice This ensures that the initialize function cannot be called during the upgrade process. + modifier onlyCallAtInitialization() { + if (_getInitializedVersion() != 0) { + revert AlreadyInitialized(); + } + + _; + } + /// @notice Disables the initializers on the implementation contract to prevent it from being left uninitialized. - /// @custom:oz-upgrades-unsafe-allow constructor constructor() { _disableInitializers(); } @@ -155,10 +172,14 @@ contract DAO is address _initialOwner, address _trustedForwarder, string calldata daoURI_ - ) external reinitializer(3) { + ) external onlyCallAtInitialization reinitializer(3) { _reentrancyStatus = _NOT_ENTERED; // added in v1.3.0 + // In addition to the current interfaceId, also support previous version of the interfaceId. + _registerInterface(type(IDAO).interfaceId ^ IExecutor.execute.selector); + _registerInterface(type(IDAO).interfaceId); + _registerInterface(type(IExecutor).interfaceId); _registerInterface(type(IERC1271).interfaceId); _registerInterface(type(IEIP4824).interfaceId); _registerInterface(type(IProtocolVersion).interfaceId); // added in v1.3.0 @@ -198,6 +219,9 @@ contract DAO is _who: address(this), _permissionId: keccak256("SET_SIGNATURE_VALIDATOR_PERMISSION") }); + + _registerInterface(type(IDAO).interfaceId); + _registerInterface(type(IExecutor).interfaceId); } } @@ -246,18 +270,12 @@ contract DAO is _setMetadata(_metadata); } - /// @inheritdoc IDAO + /// @inheritdoc IExecutor function execute( bytes32 _callId, Action[] calldata _actions, uint256 _allowFailureMap - ) - external - override - nonReentrant - auth(EXECUTE_PERMISSION_ID) - returns (bytes[] memory execResults, uint256 failureMap) - { + ) external override nonReentrant returns (bytes[] memory execResults, uint256 failureMap) { // Check that the action array length is within bounds. if (_actions.length > MAX_ACTIONS) { revert TooManyActions(); @@ -268,14 +286,65 @@ contract DAO is uint256 gasBefore; uint256 gasAfter; + bool hasExecutePermission = isGranted( + address(this), + msg.sender, + EXECUTE_PERMISSION_ID, + msg.data + ); + for (uint256 i = 0; i < _actions.length; ) { + Action calldata action = _actions[i]; + + bool isAllowed = hasExecutePermission; + bytes32 permissionId = EXECUTE_PERMISSION_ID; + + bytes32 id; + + // If action.data is 0 length, it's native eth transfer + // which is checked the same way, though `id` is keccak256('0x'). + if (action.data.length >= 4) { + id = keccak256(action.data[:4]); + } else if (action.data.length == 0) { + id = ETH_TRANSFER_PERMISSION_ID; + } + + (bool created, , ) = getPermissionStatus(action.to, id); + + if (created) { + isAllowed = isGranted(action.to, msg.sender, id, action.data); + permissionId = id; + } + + if (!isAllowed) { + revert Unauthorized(action.to, msg.sender, permissionId); + } + + bool success; + bytes memory data; + gasBefore = gasleft(); - (bool success, bytes memory result) = _actions[i].to.call{value: _actions[i].value}( - _actions[i].data - ); + (success, data) = action.to.call{value: action.value}(action.data); + gasAfter = gasleft(); + if (action.to == address(this)) { + if (!success) { + bytes4 result; + + assembly { + result := mload(add(data, 32)) + } + + if (result == Unauthorized.selector || result == UnauthorizedOwner.selector) { + gasBefore = gasleft(); + (success, data) = action.to.delegatecall(action.data); + gasAfter = gasleft(); + } + } + } + // Check if failure is allowed if (!hasBit(_allowFailureMap, uint8(i))) { // Check if the call failed. @@ -297,7 +366,7 @@ contract DAO is } } - execResults[i] = result; + execResults[i] = data; unchecked { ++i; diff --git a/packages/contracts/src/core/permission/PermissionManager.sol b/packages/contracts/src/core/permission/PermissionManager.sol index 22f36a8a1..19100f206 100644 --- a/packages/contracts/src/core/permission/PermissionManager.sol +++ b/packages/contracts/src/core/permission/PermissionManager.sol @@ -19,6 +19,9 @@ abstract contract PermissionManager is Initializable { /// @notice The ID of the permission required to call the `grant`, `grantWithCondition`, `revoke`, and `bulk` function. bytes32 public constant ROOT_PERMISSION_ID = keccak256("ROOT_PERMISSION"); + /// @notice The ID of the permission required to call the `applyMultiTargetPermissions` function. + bytes32 public constant APPLY_TARGET_PERMISSION_ID = keccak256("APPLY_TARGET_PERMISSION"); + /// @notice A special address encoding permissions that are valid for any address `who` or `where`. address internal constant ANY_ADDR = address(type(uint160).max); @@ -28,9 +31,41 @@ abstract contract PermissionManager is Initializable { /// @notice A special address encoding if a permission is allowed. address internal constant ALLOW_FLAG = address(2); + /// @notice Grant owner flag to check or assign grant ownership right for a permission + uint256 internal constant GRANT_OWNER_FLAG = uint256(1); + + /// @notice Revoke owner flag to check or assign revoke ownership right for a permission + uint256 internal constant REVOKE_OWNER_FLAG = uint256(2); + + /// @notice Full owner flag to check or assign full ownership rights for a permission + uint256 internal constant FULL_OWNER_FLAG = uint256(3); + + /// @notice To freeze the permission, `FREEZE_OWNER` must be added as an owner first. + address internal constant FREEZE_OWNER = address(1); + + /// @notice A struct containing the information for a permission. + /// @param delegations Owners can delegate the permission so delegatees can only grant it one time only. + /// @param owners The current owners of the permission with their own specific flags capabilities. + /// @param created Whether the permission has been created or not - used in consumer contracts(such as DAO.sol) to make decisions whether permission exists or not. + /// @param grantOwnerCounter How many owners(that have grant capabilities) are currently set for a permission. + /// @param revokeOwnerCounter How many owners(that have revoke capabilities) are currently set for a permission. + struct Permission { + mapping(address => uint256) delegations; + mapping(address => uint256) owners; + bool created; + uint64 grantOwnerCounter; + uint64 revokeOwnerCounter; + } + /// @notice A mapping storing permissions as hashes (i.e., `permissionHash(where, who, permissionId)`) and their status encoded by an address (unset, allowed, or redirecting to a `PermissionCondition`). mapping(bytes32 => address) internal permissionsHashed; + /// @notice A mapping storing owners and delegations of each permission. + mapping(bytes32 => Permission) private permissions; + + /// @notice The address that can be granted APPLY_TARGET_PERMISSION_ID. + address public applyTargetMethodGrantee; + /// @notice Thrown if a call is unauthorized. /// @param where The context in which the authorization reverted. /// @param who The address (EOA or contract) missing the permission. @@ -69,6 +104,27 @@ abstract contract PermissionManager is Initializable { /// @notice Thrown if `Operation.GrantWithCondition` is requested as an operation but the method does not support it. error GrantWithConditionNotSupported(); + /// @notice Thrown if the permission is already created. + error PermissionAlreadyCreated(); + + /// @notice Thrown if the calling account doesn't have the correct permission flags set. + error UnauthorizedOwner(address caller, uint256 callerFlags, uint256 flags); + + /// @notice Thrown if an argument passed is a zero address. + error ZeroAddress(); + + /// @notice Thrown if the passed removal flags are invalid. The caller can only pass flags the user already has. + error InvalidMissingFlags(uint256 currentFlags, uint256 removalFlags); + + /// @notice Thrown if the passed flag is set to zero. + error InvalidEmptyFlag(); + + /// @notice Thrown if the permission is frozen. + error PermissionFrozen(address where, bytes32 permissionId); + + /// @notice Thrown if the APPLY_TARGET_PERMISSION_ID is granted to the incorrect address. + error UnrecognizedApplyPermissionsGrantee(address applyTargetMethodGrantee); + /// @notice Emitted when a permission `permission` is granted in the context `here` to the address `_who` for the contract `_where`. /// @param permissionId The permission identifier. /// @param here The address of the context in which the permission is granted. @@ -95,6 +151,91 @@ abstract contract PermissionManager is Initializable { address indexed who ); + /// @notice Emitted when a permission does get delegated. + /// @param where The address of the target contract for which the delegatee does get permissions. + /// @param permissionIdOrSelector The permission identifier. + /// @param delegatee The address of the delegatee. + /// @param flags The flags delegated to the delegatee. + event PermissionDelegated( + address indexed where, + bytes32 indexed permissionIdOrSelector, + address indexed delegatee, + uint256 flags + ); + + /// @notice Emitted when a permission does get undelegated. + /// @param where The address of the target contract for which the delegatee loses permissions. + /// @param permissionIdOrSelector The permission identifier. + /// @param delegatee The address of the delegatee. + /// @param flags The current/updated flags left on the delegatee. + event PermissionUndelegated( + address indexed where, + bytes32 indexed permissionIdOrSelector, + address indexed delegatee, + uint256 flags + ); + + /// @notice Emitted when an owner does get added. + /// @param where The address of the target contract for which the owner does get permissions. + /// @param permissionIdOrSelector The permission identifier. + /// @param owner The address of the new owner. + /// @param flags The new flags of the owner. + event OwnerAdded( + address indexed where, + bytes32 indexed permissionIdOrSelector, + address indexed owner, + uint256 flags + ); + + /// @notice Emitted when an owner gets renounced. + /// @param where The address of the target contract for which the owner loses permissions. + /// @param permissionIdOrSelector The permission identifier. + /// @param owner The address of the owner. + /// @param updatedOwnerFlags The updated/current flags left on the owner. + /// @param updatedDelegateeFlags The updated/current flags left on the owner. + event OwnerRenounced( + address indexed where, + bytes32 indexed permissionIdOrSelector, + address indexed owner, + uint256 updatedOwnerFlags, + uint256 updatedDelegateeFlags + ); + + /// @notice Emitted when a permission does get created. + /// @param where The address of the target contract. + /// @param permissionIdOrSelector The permission identifier. + /// @param owner The initial owner of that permission. + /// @param whos The addresses passed that to grant the permission to them. + event PermissionCreated( + address indexed where, + bytes32 indexed permissionIdOrSelector, + address indexed owner, + address[] whos + ); + + /// @notice Emitted when a ROOT sets applyTargetMethodGrantee. + event ApplyTargetMethodGranteeSet(address indexed applyTargetMethodGrantee); + + /// @notice Initialization method to set the initial owner of the permission manager. + /// @dev The initial owner is granted the `ROOT_PERMISSION_ID` permission. + /// @param _initialOwner The initial owner of the permission manager. + function __PermissionManager_init(address _initialOwner) internal onlyInitializing { + _initializePermissionManager({_initialOwner: _initialOwner}); + } + + /// @notice Modifier used to protect PM methods from only being called by allowed owners. + /// @param _where The target contract to revoke or give permissions on. + /// @param _permissionId The permission to check the permissions for. + /// @param _operation The operation used to check ownership. + modifier ownerAuth( + address _where, + bytes32 _permissionId, + PermissionLib.Operation _operation + ) { + _ownerAuth(_where, _permissionId, _operation); + _; + } + /// @notice A modifier to make functions on inheriting contracts authorized. Permissions to call the function are checked through this permission manager. /// @param _permissionId The permission identifier required to call the method this modifier is applied to. modifier auth(bytes32 _permissionId) { @@ -102,11 +243,248 @@ abstract contract PermissionManager is Initializable { _; } - /// @notice Initialization method to set the initial owner of the permission manager. - /// @dev The initial owner is granted the `ROOT_PERMISSION_ID` permission. - /// @param _initialOwner The initial owner of the permission manager. - function __PermissionManager_init(address _initialOwner) internal onlyInitializing { - _initializePermissionManager({_initialOwner: _initialOwner}); + /// @dev Requires the `ROOT_PERMISSION_ID` permission. + /// @param _where The address of the target contract for which `_who` receives permission. + /// @param _permissionIdOrSelector The permission hash or function selector used for this permission. + /// @param _owner The initial owner of this newly created permission. + /// @param _whos The addresses of the target contracts for which `_who` receives permission. + function createPermission( + address _where, + bytes32 _permissionIdOrSelector, + address _owner, + address[] calldata _whos + ) external auth(ROOT_PERMISSION_ID) { + _createPermission(_where, _permissionIdOrSelector, _owner, _whos); + } + + /// @notice Function to delegate specific flags of a permission. + /// @param _where The address of the target contract for which `_who` receives permission. + /// @param _permissionIdOrSelector The permission hash or function selector used for this permission. + /// @param _delegatee The addresses `who` gets the permission delegated. + /// @param _flags The flags as uint256 the permission owner wants to give this specific delegatee. + function delegatePermission( + address _where, + bytes32 _permissionIdOrSelector, + address _delegatee, + uint256 _flags + ) public virtual { + if (_flags == 0) { + revert InvalidEmptyFlag(); + } + + bytes32 permHash = permissionHash(_where, _permissionIdOrSelector); + Permission storage permission = permissions[permHash]; + + if (_isPermissionFrozen(permission)) { + revert PermissionFrozen(_where, _permissionIdOrSelector); + } + + if (!_checkFlags(permission.owners[msg.sender], _flags)) { + revert UnauthorizedOwner(msg.sender, permission.owners[msg.sender], _flags); + } + + uint256 currentFlags = permission.delegations[_delegatee]; + + // If the same flags that a `delegatee` already holds is added, return early. + if (_checkFlags(currentFlags, _flags)) { + return; + } + + uint256 newFlags = currentFlags | _flags; + permission.delegations[_delegatee] = newFlags; + + emit PermissionDelegated(_where, _permissionIdOrSelector, _delegatee, newFlags); + } + + /// @notice Function to remove specific flags from the delegatee + /// @param _where The address of the target contract for which `_who` receives permission. + /// @param _permissionIdOrSelector The permission hash or function selector used for this permission. + /// @param _delegatee The addresses we want to undelegate specific flags. + /// @param _flags The flags as uint256 the permission owner wants to remove from this specific delegatee. + function undelegatePermission( + address _where, + bytes32 _permissionIdOrSelector, + address _delegatee, + uint256 _flags + ) public virtual { + if (_flags == 0) { + revert InvalidEmptyFlag(); + } + + bytes32 permHash = permissionHash(_where, _permissionIdOrSelector); + Permission storage permission = permissions[permHash]; + + if (!_checkFlags(permission.owners[msg.sender], _flags)) { + revert UnauthorizedOwner(msg.sender, permission.owners[msg.sender], _flags); + } + + uint256 currentFlags = permission.delegations[_delegatee]; + if (!_checkFlags(currentFlags, _flags)) { + revert InvalidMissingFlags(currentFlags, _flags); + } + + uint256 newFlags = currentFlags ^ _flags; + permission.delegations[_delegatee] = newFlags; + + emit PermissionUndelegated(_where, _permissionIdOrSelector, _delegatee, newFlags); + } + + /// @notice Function to add a new owner to a permission. + /// @param _where The address of the target contract for which `_who` receives permission. + /// @param _permissionIdOrSelector The permission hash or function selector used for this permission. + /// @param _owner The new manager for this permission. + /// @param _flags The flags as uint256 to restrict what this specifc owner actually can do. (only revoke? only grant? both?) + function addOwner( + address _where, + bytes32 _permissionIdOrSelector, + address _owner, + uint256 _flags + ) public virtual { + if (_flags == 0) { + revert InvalidEmptyFlag(); + } + + if (_owner == address(0) || _where == address(0)) { + revert ZeroAddress(); + } + + Permission storage permission = permissions[ + permissionHash(_where, _permissionIdOrSelector) + ]; + + if (_isPermissionFrozen(permission)) { + revert PermissionFrozen(_where, _permissionIdOrSelector); + } + + if (!_checkFlags(permission.owners[msg.sender], _flags)) { + revert UnauthorizedOwner(msg.sender, permission.owners[msg.sender], _flags); + } + + uint256 currentFlags = permission.owners[_owner]; + + // If the same flags that an `owner` already holds is added, return early. + if (_checkFlags(currentFlags, _flags)) { + return; + } + + if (_owner != FREEZE_OWNER) { + if ( + _checkFlags(_flags, GRANT_OWNER_FLAG) && + !_checkFlags(currentFlags, GRANT_OWNER_FLAG) + ) { + permission.grantOwnerCounter++; + } + + if ( + _checkFlags(_flags, REVOKE_OWNER_FLAG) && + !_checkFlags(currentFlags, REVOKE_OWNER_FLAG) + ) { + permission.revokeOwnerCounter++; + } + } + + permission.owners[_owner] = currentFlags | _flags; // Update owner permission + + emit OwnerAdded(_where, _permissionIdOrSelector, _owner, _flags); + } + + /// @notice Function that a owner can remove itself as owner. + /// @param _where The address of the target contract for which `_who` receives permission. + /// @param _permissionIdOrSelector The permission hash or function selector used for this permission. + /// @param _flags The flags as uint256 to remove specific rights the calling owner does have. (only revoke? only grant? both?) + function removeOwner(address _where, bytes32 _permissionIdOrSelector, uint256 _flags) public { + if (_flags == 0) { + revert InvalidEmptyFlag(); + } + + Permission storage permission = permissions[ + permissionHash(_where, _permissionIdOrSelector) + ]; + + uint256 ownerFlags = permission.owners[msg.sender]; + + // Check if the removal flags have more bit set as the owner currently has. + if (!_checkFlags(ownerFlags, _flags)) { + revert InvalidMissingFlags(ownerFlags, _flags); + } + + if (_checkFlags(_flags, GRANT_OWNER_FLAG)) { + permission.grantOwnerCounter--; + } + + if (_checkFlags(_flags, REVOKE_OWNER_FLAG)) { + permission.revokeOwnerCounter--; + } + + uint256 newOwnerFlags = ownerFlags ^ _flags; // remove permissions + permission.owners[msg.sender] = newOwnerFlags; + + // Renouncing the ownership should also mean to renounce delegation. + uint256 delegateeFlags = permission.delegations[msg.sender]; + uint256 newDelegateeFlags = 0; + + if (_checkFlags(delegateeFlags, _flags)) { + newDelegateeFlags = delegateeFlags ^ _flags; + permission.delegations[msg.sender] = newDelegateeFlags; + } + + emit OwnerRenounced( + _where, + _permissionIdOrSelector, + msg.sender, + newOwnerFlags, + newDelegateeFlags + ); + } + + /// @notice Function to check if this specific permission is frozen. + /// @param _where The address of the target contract for which `_who` receives permission. + /// @param _permissionIdOrSelector The permission hash or function selector used for this permission. + /// @return True if the permission is frozen and otherwise false. + function isPermissionFrozen( + address _where, + bytes32 _permissionIdOrSelector + ) public view returns (bool) { + Permission storage permission = permissions[ + permissionHash(_where, _permissionIdOrSelector) + ]; + + return _isPermissionFrozen(permission); + } + + /// @notice Function to retrieve if permission is created and how many owners it has. + /// @param _where The address of the target contract for which `_who` receives permission. + /// @param _permissionIdOrSelector The permission hash or function selector used for this permission. + /// @return Whether the permission has been created or not. + /// @return How many grant owners this permission has currently. + /// @return How many revoke owners this permission has currently. + function getPermissionStatus( + address _where, + bytes32 _permissionIdOrSelector + ) public view returns (bool, uint64, uint64) { + Permission storage permission = permissions[ + permissionHash(_where, _permissionIdOrSelector) + ]; + + return (permission.created, permission.grantOwnerCounter, permission.revokeOwnerCounter); + } + + /// @notice Function to retrieve the owner and delegate flags of an `_account` on a permission. + /// @param _where The address of the target contract for which `_who` receives permission. + /// @param _permissionIdOrSelector The permission hash or function selector used for this permission. + /// @param _account The address for which to return the current flags. + /// @return uint256 Returns owner flags. 0 if an `account` is not an owner. + /// @return uint256 Returns delegatee flags. 0 if an `account` is not a delegatee. + function getPermissionFlags( + address _where, + bytes32 _permissionIdOrSelector, + address _account + ) public view returns (uint256, uint256) { + Permission storage permission = permissions[ + permissionHash(_where, _permissionIdOrSelector) + ]; + + return (permission.owners[_account], permission.delegations[_account]); } /// @notice Grants permission to an address to call methods in a contract guarded by an auth modifier with the specified permission identifier. @@ -119,7 +497,7 @@ abstract contract PermissionManager is Initializable { address _where, address _who, bytes32 _permissionId - ) external virtual auth(ROOT_PERMISSION_ID) { + ) external virtual ownerAuth(_where, _permissionId, PermissionLib.Operation.Grant) { _grant({_where: _where, _who: _who, _permissionId: _permissionId}); } @@ -135,7 +513,11 @@ abstract contract PermissionManager is Initializable { address _who, bytes32 _permissionId, IPermissionCondition _condition - ) external virtual auth(ROOT_PERMISSION_ID) { + ) + external + virtual + ownerAuth(_where, _permissionId, PermissionLib.Operation.GrantWithCondition) + { _grantWithCondition({ _where: _where, _who: _who, @@ -154,19 +536,42 @@ abstract contract PermissionManager is Initializable { address _where, address _who, bytes32 _permissionId - ) external virtual auth(ROOT_PERMISSION_ID) { + ) external virtual ownerAuth(_where, _permissionId, PermissionLib.Operation.Revoke) { _revoke({_where: _where, _who: _who, _permissionId: _permissionId}); } + /// @notice Only this contract is allowed to call the apply target methods below. + /// @param _applyTargetMethodGrantee The address that can be granted APPLY_TARGET_PERMISSION_ID. + function setApplyTargetMethodGrantee( + address _applyTargetMethodGrantee + ) public virtual auth(ROOT_PERMISSION_ID) { + applyTargetMethodGrantee = _applyTargetMethodGrantee; + + emit ApplyTargetMethodGranteeSet(_applyTargetMethodGrantee); + } + /// @notice Applies an array of permission operations on a single target contracts `_where`. /// @param _where The address of the single target contract. - /// @param items The array of single-targeted permission operations to apply. + /// @param _items The array of single-targeted permission operations to apply. function applySingleTargetPermissions( address _where, - PermissionLib.SingleTargetPermission[] calldata items - ) external virtual auth(ROOT_PERMISSION_ID) { - for (uint256 i; i < items.length; ) { - PermissionLib.SingleTargetPermission memory item = items[i]; + PermissionLib.SingleTargetPermission[] calldata _items + ) external virtual { + if (!_canApplyTarget()) { + revert Unauthorized(address(this), msg.sender, APPLY_TARGET_PERMISSION_ID); + } + + for (uint256 i; i < _items.length; ) { + PermissionLib.SingleTargetPermission memory item = _items[i]; + Permission storage permission = permissions[permissionHash(_where, item.permissionId)]; + + if (_isPermissionFrozen(permission)) { + revert PermissionFrozen(_where, item.permissionId); + } + + if (!_canManage(permission, msg.sender, item.operation, true)) { + revert Unauthorized(_where, item.who, item.permissionId); + } if (item.operation == PermissionLib.Operation.Grant) { _grant({_where: _where, _who: item.who, _permissionId: item.permissionId}); @@ -186,9 +591,24 @@ abstract contract PermissionManager is Initializable { /// @param _items The array of multi-targeted permission operations to apply. function applyMultiTargetPermissions( PermissionLib.MultiTargetPermission[] calldata _items - ) external virtual auth(ROOT_PERMISSION_ID) { + ) external virtual { + if (!_canApplyTarget()) { + revert Unauthorized(address(this), msg.sender, APPLY_TARGET_PERMISSION_ID); + } + for (uint256 i; i < _items.length; ) { PermissionLib.MultiTargetPermission memory item = _items[i]; + Permission storage permission = permissions[ + permissionHash(item.where, item.permissionId) + ]; + + if (_isPermissionFrozen(permission)) { + revert PermissionFrozen(item.where, item.permissionId); + } + + if (!_canManage(permission, msg.sender, item.operation, true)) { + revert Unauthorized(item.where, item.who, item.permissionId); + } if (item.operation == PermissionLib.Operation.Grant) { _grant({_where: item.where, _who: item.who, _permissionId: item.permissionId}); @@ -229,7 +649,9 @@ abstract contract PermissionManager is Initializable { ]; // If the permission was granted directly, return `true`. - if (specificCallerTargetPermission == ALLOW_FLAG) return true; + if (specificCallerTargetPermission == ALLOW_FLAG) { + return true; + } // If the permission was granted with a condition, check the condition and return the result. if (specificCallerTargetPermission != UNSET_FLAG) { @@ -340,6 +762,14 @@ abstract contract PermissionManager is Initializable { revert PermissionsForAnyAddressDisallowed(); } + // Make sure that this special permission is only granted + // to the address allowed by ROOT. + if (_permissionId == APPLY_TARGET_PERMISSION_ID) { + if (applyTargetMethodGrantee == address(0) || applyTargetMethodGrantee != _who) { + revert UnrecognizedApplyPermissionsGrantee(applyTargetMethodGrantee); + } + } + bytes32 permHash = permissionHash({ _where: _where, _who: _who, @@ -401,6 +831,14 @@ abstract contract PermissionManager is Initializable { } } + // Make sure that this special permission is only granted + // to the address allowed by ROOT. + if (_permissionId == APPLY_TARGET_PERMISSION_ID) { + if (applyTargetMethodGrantee == address(0) || applyTargetMethodGrantee != _who) { + revert UnrecognizedApplyPermissionsGrantee(applyTargetMethodGrantee); + } + } + bytes32 permHash = permissionHash({ _where: _where, _who: _who, @@ -454,7 +892,59 @@ abstract contract PermissionManager is Initializable { } } - /// @notice A private function to be used to check permissions on the permission manager contract (`address(this)`) itself. + /// @notice Function to check if the given address is ROOT. + /// @param _who The address to check for. + /// @return True if the given address is ROOT and otherwise false + function _isRoot(address _who) private view returns (bool) { + return isGranted(address(this), _who, ROOT_PERMISSION_ID, msg.data); + } + + /// @notice Internal function to create a new permission. + /// @dev Requires the `ROOT_PERMISSION_ID` permission. + /// @param _where The address of the target contract for which `_who` receives permission. + /// @param _permissionIdOrSelector The permission hash or function selector used for this permission. + /// @param _owner The initial owner of this newly created permission. + /// @param _whos The addresses of the target contracts for which `_who` receives permission. + function _createPermission( + address _where, + bytes32 _permissionIdOrSelector, + address _owner, + address[] calldata _whos + ) internal virtual { + Permission storage permission = permissions[ + permissionHash(_where, _permissionIdOrSelector) + ]; + + if (permission.created) { + revert PermissionAlreadyCreated(); + } + + permission.created = true; + permission.owners[_owner] = FULL_OWNER_FLAG; + + if (_whos.length > 0) { + for (uint256 i = 0; i < _whos.length; i++) { + _grant(_where, _whos[i], _permissionIdOrSelector); + } + } + + permission.grantOwnerCounter = 1; + permission.revokeOwnerCounter = 1; + + emit PermissionCreated(_where, _permissionIdOrSelector, _owner, _whos); + } + + /// @notice Internal function to check if this specific permission is frozen. + /// @param _permission Permission struct to check. + /// @return True if the permission is frozen and otherwise false + function _isPermissionFrozen(Permission storage _permission) private view returns (bool) { + return + _permission.grantOwnerCounter == 0 && + _permission.revokeOwnerCounter == 0 && + _permission.owners[FREEZE_OWNER] != 0; + } + + /// @notice An internal function to be used to check permissions on the permission manager contract (`address(this)`) itself. /// @param _permissionId The permission identifier required to call the method this modifier is applied to. function _auth(bytes32 _permissionId) internal view virtual { if (!isGranted(address(this), msg.sender, _permissionId, msg.data)) { @@ -466,6 +956,37 @@ abstract contract PermissionManager is Initializable { } } + /// @notice An internal function to check if the caller has either root or apply target permission. + /// @dev Reverts in case the caller has none of these permissions. + /// @return isAllowed True if the caller has either ROOT or `APPLY_TARGET_PERMISSION_ID` on `address(this)`, otherwise false. + function _canApplyTarget() internal view virtual returns (bool isAllowed) { + isAllowed = _isRoot(msg.sender); + + if (!isAllowed) { + isAllowed = isGranted(address(this), msg.sender, APPLY_TARGET_PERMISSION_ID, msg.data); + } + } + + /// @notice An internal function used to protect PM methods from only being called by allowed owners or ROOT in case no owner is set. + /// @param _where The target contract to revoke or give permissions on. + /// @param _permissionId The permission to check the permissions for. + /// @param _operation The operation used to check ownership. + function _ownerAuth( + address _where, + bytes32 _permissionId, + PermissionLib.Operation _operation + ) internal virtual { + Permission storage permission = permissions[permissionHash(_where, _permissionId)]; + + if (_isPermissionFrozen(permission)) { + revert PermissionFrozen(_where, _permissionId); + } + + if (!_canManage(permission, msg.sender, _operation, _isRoot(msg.sender))) { + revert Unauthorized(_where, msg.sender, _permissionId); + } + } + /// @notice Generates the hash for the `permissionsHashed` mapping obtained from the word "PERMISSION", the contract address, the address owning the permission, and the permission identifier. /// @param _where The address of the target contract for which `_who` receives permission. /// @param _who The address (EOA or contract) owning the permission. @@ -479,6 +1000,77 @@ abstract contract PermissionManager is Initializable { return keccak256(abi.encodePacked("PERMISSION", _who, _where, _permissionId)); } + /// @notice Generates the hash for the `permissionsHashed` mapping obtained from the word "PERMISSION", the contract address, the address owning the permission, and the permission identifier. + /// @param _where The address of the target contract for which `_who` receives permission. + /// @param _permissionId The permission identifier. + /// @return The role hash. + function permissionHash( + address _where, + bytes32 _permissionId + ) internal pure virtual returns (bytes32) { + return keccak256(abi.encodePacked("ROLE_PERMISSION_ID", _where, _permissionId)); + } + + /// @notice Checks the permission bitmap against the passed flags. + /// @param _permission uint256 bitmap to check against. + /// @param _flags uint256 bitmap to check. + /// @return True if the bit's are flipped as expected and false otherwise. + function _checkFlags(uint256 _permission, uint256 _flags) private pure returns (bool) { + return (_permission & _flags) == _flags; + } + + /// @notice Checks if `_who` is allowed for the operation. + /// @dev If the permission is created, either the `_who` must be an owner/delegatee or ROOT only if permission has no owners. + /// @param _permission The Permission struct. + /// @param _who The address to check + /// @param _operation The operation to check the permission against. + /// @return True if the permission checks succeded otherwise false. + function _canManage( + Permission storage _permission, + address _who, + PermissionLib.Operation _operation, + bool isRoot + ) private returns (bool) { + if (!_permission.created) { + return isRoot; + } + + if ( + _operation == PermissionLib.Operation.Grant || + _operation == PermissionLib.Operation.GrantWithCondition + ) { + if (_checkFlags(_permission.owners[_who], GRANT_OWNER_FLAG)) { + return true; + } else { + uint256 delegationFlags = _permission.delegations[_who]; + + if (_checkFlags(delegationFlags, GRANT_OWNER_FLAG)) { + _permission.delegations[_who] = delegationFlags ^ GRANT_OWNER_FLAG; + return true; + } + + return isRoot && _permission.grantOwnerCounter == 0; + } + } + + if (_operation == PermissionLib.Operation.Revoke) { + if (_checkFlags(_permission.owners[_who], REVOKE_OWNER_FLAG)) { + return true; + } else { + uint256 delegationFlags = _permission.delegations[_who]; + + if (_checkFlags(delegationFlags, REVOKE_OWNER_FLAG)) { + _permission.delegations[_who] = delegationFlags ^ REVOKE_OWNER_FLAG; + return true; + } + + return isRoot && _permission.revokeOwnerCounter == 0; + } + } + + return false; + } + /// @notice Decides if the granting permissionId is restricted when `_who == ANY_ADDR` or `_where == ANY_ADDR`. /// @param _permissionId The permission identifier. /// @return Whether or not the permission is restricted. @@ -491,5 +1083,5 @@ abstract contract PermissionManager is Initializable { } /// @notice This empty reserved space is put in place to allow future versions to add new variables without shifting down storage in the inheritance chain (see [OpenZeppelin's guide about storage gaps](https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps)). - uint256[49] private __gap; + uint256[47] private __gap; } diff --git a/packages/contracts/src/framework/dao/DAOFactory.sol b/packages/contracts/src/framework/dao/DAOFactory.sol index 0e964a470..7711b4b5b 100644 --- a/packages/contracts/src/framework/dao/DAOFactory.sol +++ b/packages/contracts/src/framework/dao/DAOFactory.sol @@ -12,11 +12,23 @@ import {PermissionLib} from "@aragon/osx-commons-contracts/src/permission/Permis import {ProxyLib} from "@aragon/osx-commons-contracts/src/utils/deployment/ProxyLib.sol"; import {DAO} from "../../core/dao/DAO.sol"; -import {PluginRepo} from "../plugin/repo/PluginRepo.sol"; import {PluginSetupProcessor} from "../plugin/setup/PluginSetupProcessor.sol"; import {hashHelpers, PluginSetupRef} from "../plugin/setup/PluginSetupProcessorHelpers.sol"; import {DAORegistry} from "./DAORegistry.sol"; +// Permission Identifiers necessary to create and set up the dao. +bytes32 constant ROOT_PERMISSION_ID = keccak256("ROOT_PERMISSION"); +bytes32 constant APPLY_INSTALLATION_PERMISSION_ID = keccak256("APPLY_INSTALLATION_PERMISSION"); +bytes32 constant APPLY_TARGET_PERMISSION_ID = keccak256("APPLY_TARGET_PERMISSION"); +bytes32 constant UPGRADE_DAO_PERMISSION_ID = keccak256("UPGRADE_DAO_PERMISSION"); +bytes32 constant SET_METADATA_PERMISSION_ID = keccak256("SET_METADATA_PERMISSION"); +bytes32 constant REGISTER_STANDARD_CALLBACK_PERMISSION_ID = keccak256( + "REGISTER_STANDARD_CALLBACK_PERMISSION" +); +bytes32 constant SET_TRUSTED_FORWARDER_PERMISSION_ID = keccak256( + "SET_TRUSTED_FORWARDER_PERMISSION" +); + /// @title DAOFactory /// @author Aragon X - 2022-2023 /// @notice This contract is used to create a DAO. @@ -93,20 +105,21 @@ contract DAOFactory is ERC165, ProtocolVersion { // Register DAO. daoRegistry.register(createdDao, msg.sender, _daoSettings.subdomain); - // Get Permission IDs - bytes32 rootPermissionID = createdDao.ROOT_PERMISSION_ID(); - bytes32 applyInstallationPermissionID = pluginSetupProcessor - .APPLY_INSTALLATION_PERMISSION_ID(); + // Set the psp to be the allowed grantee for `APPLY_TARGET_PERMISSION_ID`. + createdDao.setApplyTargetMethodGrantee(address(pluginSetupProcessor)); - // Grant the temporary permissions. - // Grant Temporarily `ROOT_PERMISSION` to `pluginSetupProcessor`. - createdDao.grant(address(createdDao), address(pluginSetupProcessor), rootPermissionID); + // Grant Temporarily `APPLY_TARGET_PERMISSION_ID` to `pluginSetupProcessor`. + createdDao.grant( + address(createdDao), + address(pluginSetupProcessor), + APPLY_TARGET_PERMISSION_ID + ); // Grant Temporarily `APPLY_INSTALLATION_PERMISSION` on `pluginSetupProcessor` to this `DAOFactory`. createdDao.grant( address(pluginSetupProcessor), address(this), - applyInstallationPermissionID + APPLY_INSTALLATION_PERMISSION_ID ); // Install plugins on the newly created DAO. @@ -139,19 +152,23 @@ contract DAOFactory is ERC165, ProtocolVersion { _setDAOPermissions(createdDao); // Revoke the temporarily granted permissions. - // Revoke Temporarily `ROOT_PERMISSION` from `pluginSetupProcessor`. - createdDao.revoke(address(createdDao), address(pluginSetupProcessor), rootPermissionID); + // Revoke Temporarily `APPLY_TARGET_PERMISSION_ID` from `pluginSetupProcessor`. + createdDao.revoke( + address(createdDao), + address(pluginSetupProcessor), + APPLY_TARGET_PERMISSION_ID + ); // Revoke `APPLY_INSTALLATION_PERMISSION` on `pluginSetupProcessor` from this `DAOFactory` . createdDao.revoke( address(pluginSetupProcessor), address(this), - applyInstallationPermissionID + APPLY_INSTALLATION_PERMISSION_ID ); // Revoke Temporarily `ROOT_PERMISSION_ID` from `pluginSetupProcessor` that implicitly granted to this `DaoFactory` // at the create dao step `address(this)` being the initial owner of the new created DAO. - createdDao.revoke(address(createdDao), address(this), rootPermissionID); + createdDao.revoke(address(createdDao), address(this), ROOT_PERMISSION_ID); } /// @notice Deploys a new DAO `ERC1967` proxy, and initialize it with this contract as the initial owner. @@ -187,27 +204,27 @@ contract DAOFactory is ERC165, ProtocolVersion { items[0] = PermissionLib.SingleTargetPermission( PermissionLib.Operation.Grant, address(_dao), - _dao.ROOT_PERMISSION_ID() + ROOT_PERMISSION_ID ); items[1] = PermissionLib.SingleTargetPermission( PermissionLib.Operation.Grant, address(_dao), - _dao.UPGRADE_DAO_PERMISSION_ID() + UPGRADE_DAO_PERMISSION_ID ); items[2] = PermissionLib.SingleTargetPermission( PermissionLib.Operation.Grant, address(_dao), - _dao.SET_TRUSTED_FORWARDER_PERMISSION_ID() + SET_TRUSTED_FORWARDER_PERMISSION_ID ); items[3] = PermissionLib.SingleTargetPermission( PermissionLib.Operation.Grant, address(_dao), - _dao.SET_METADATA_PERMISSION_ID() + SET_METADATA_PERMISSION_ID ); items[4] = PermissionLib.SingleTargetPermission( PermissionLib.Operation.Grant, address(_dao), - _dao.REGISTER_STANDARD_CALLBACK_PERMISSION_ID() + REGISTER_STANDARD_CALLBACK_PERMISSION_ID ); _dao.applySingleTargetPermissions(address(_dao), items); diff --git a/packages/contracts/test/core/dao/dao.ts b/packages/contracts/test/core/dao/dao.ts index cd5106361..e63ce6243 100644 --- a/packages/contracts/test/core/dao/dao.ts +++ b/packages/contracts/test/core/dao/dao.ts @@ -17,9 +17,14 @@ import { IProtocolVersion__factory, PermissionConditionMock__factory, PermissionConditionMock, + IExecutor__factory, + IDAO, + ERC165, } from '../../../typechain'; import {DAO__factory as DAO_V1_0_0__factory} from '../../../typechain/@aragon/osx-v1.0.1/core/dao/DAO.sol'; +import {IDAO__factory as IDAO_V1_0_0_factory} from '../../../typechain/@aragon/osx-v1.0.1/core/dao/IDAO.sol'; import {DAO__factory as DAO_V1_3_0__factory} from '../../../typechain/@aragon/osx-v1.3.0/core/dao/DAO.sol'; +import {IDAO__factory as IDAO_V3_0_0_factory} from '../../../typechain/@aragon/osx-v1.3.0/core/dao/IDAO.sol'; import {ExecutedEvent} from '../../../typechain/DAO'; import { getActions, @@ -142,7 +147,7 @@ describe('DAO', function () { dummyAddress1, daoExampleURI ) - ).to.be.revertedWith('Initializable: contract is already initialized'); + ).to.be.revertedWithCustomError(dao, 'AlreadyInitialized'); }); it('initializes with the correct trusted forwarder', async () => { @@ -301,6 +306,25 @@ describe('DAO', function () { ).toNumber() ).to.equal(0); }); + + it('registers IExecutor interface for versions < 1.4.0', async () => { + // Create an uninitialized DAO. + const uninitializedDao = await deployWithProxy(DAO); + + expect( + await uninitializedDao.supportsInterface( + getInterfaceId(IExecutor__factory.createInterface()) + ) + ).to.be.false; + + await uninitializedDao.initializeFrom([1, 3, 0], EMPTY_DATA); + + expect( + await uninitializedDao.supportsInterface( + getInterfaceId(IExecutor__factory.createInterface()) + ) + ).to.be.true; + }); }); describe('Upgrades', async () => { @@ -308,6 +332,10 @@ describe('DAO', function () { let currentContractFactory: ContractFactory; let initArgs: any; + const IExecutorInterfaceId = getInterfaceId( + IExecutor__factory.createInterface() + ); + before(() => { currentContractFactory = new DAO__factory(signers[0]); @@ -333,7 +361,7 @@ describe('DAO', function () { it('upgrades from v1.0.0', async () => { legacyContractFactory = new DAO_V1_0_0__factory(signers[0]); - const {fromImplementation, toImplementation} = + const {proxy, fromImplementation, toImplementation} = await deployAndUpgradeFromToCheck( signers[0], signers[1], @@ -357,12 +385,23 @@ describe('DAO', function () { IMPLICIT_INITIAL_PROTOCOL_VERSION ); expect(toProtocolVersion).to.deep.equal(osxContractsVersion()); + + await proxy.initializeFrom([1, 0, 0], EMPTY_DATA); + + // Check that it still supports old interfaceId for backwards compatibility. + expect( + await proxy.supportsInterface( + getInterfaceId(IDAO_V1_0_0_factory.createInterface()) + ) + ).to.be.true; + + expect(await proxy.supportsInterface(IExecutorInterfaceId)).to.be.true; }); it('from v1.3.0', async () => { legacyContractFactory = new DAO_V1_3_0__factory(signers[0]); - const {fromImplementation, toImplementation} = + const {proxy, fromImplementation, toImplementation} = await deployAndUpgradeFromToCheck( signers[0], signers[1], @@ -384,6 +423,17 @@ describe('DAO', function () { expect(fromProtocolVersion).to.not.deep.equal(toProtocolVersion); expect(fromProtocolVersion).to.deep.equal([1, 3, 0]); expect(toProtocolVersion).to.deep.equal(osxContractsVersion()); + + await proxy.initializeFrom([1, 3, 0], EMPTY_DATA); + + // Check that it still supports old interfaceId for backwards compatibility. + expect( + await proxy.supportsInterface( + getInterfaceId(IDAO_V3_0_0_factory.createInterface()) + ) + ).to.be.true; + + expect(await proxy.supportsInterface(IExecutorInterfaceId)).to.be.true; }); }); @@ -399,7 +449,6 @@ describe('DAO', function () { it('supports the `IDAO` interface', async () => { const iface = IDAO__factory.createInterface(); - expect(getInterfaceId(iface)).to.equal('0x9385547e'); // the interfaceID from IDAO v1.0.0 expect(await dao.supportsInterface(getInterfaceId(iface))).to.be.true; }); @@ -506,7 +555,7 @@ describe('DAO', function () { await expect(dao.execute(ZERO_BYTES32, [data.succeedAction], 0)) .to.be.revertedWithCustomError(dao, 'Unauthorized') .withArgs( - dao.address, + data.succeedAction.to, ownerAddress, DAO_PERMISSIONS.EXECUTE_PERMISSION_ID ); @@ -562,6 +611,190 @@ describe('DAO', function () { .withArgs(1); }); + it('reverts if a permission is created and the caller does not have the permission to call it', async () => { + const permissionId = ethers.utils.keccak256( + data.succeedAction.data.substr(0, 10) + ); + + await dao.createPermission( + data.succeedAction.to, + permissionId, + ownerAddress, + [signers[1].address] + ); + + await expect(dao.execute(ZERO_BYTES32, [data.succeedAction], 0)) + .to.be.revertedWithCustomError(dao, 'Unauthorized') + .withArgs(data.succeedAction.to, ownerAddress, permissionId); + }); + + it('succeeds if a permission is created and the caller does not have execution rights', async () => { + await dao.revoke( + dao.address, + ownerAddress, + DAO_PERMISSIONS.EXECUTE_PERMISSION_ID + ); + + const permissionId = ethers.utils.keccak256( + data.succeedAction.data.substr(0, 10) + ); + + await dao.createPermission( + data.succeedAction.to, + permissionId, + ownerAddress, + [ownerAddress] + ); + + await expect(dao.execute(ZERO_BYTES32, [data.succeedAction], 0)).to.emit( + dao, + 'Executed' + ); + }); + + it('should fail for the caller with only execution permission if the other caller has the permission to call something', async () => { + await dao.grant( + dao.address, + signers[1].address, + DAO_PERMISSIONS.EXECUTE_PERMISSION_ID + ); + + const permissionId = ethers.utils.keccak256( + data.succeedAction.data.substr(0, 10) + ); + + await dao.createPermission( + data.succeedAction.to, + permissionId, + ownerAddress, + [ownerAddress] + ); + + await dao.revoke( + dao.address, + ownerAddress, + DAO_PERMISSIONS.EXECUTE_PERMISSION_ID + ); + + await expect(dao.execute(ZERO_BYTES32, [data.succeedAction], 0)).to.emit( + dao, + 'Executed' + ); + + await expect( + dao.connect(signers[1]).execute(ZERO_BYTES32, [data.succeedAction], 0) + ) + .to.be.revertedWithCustomError(dao, 'Unauthorized') + .withArgs(data.succeedAction.to, signers[1].address, permissionId); + }); + + it('reverts if neither dao nor caller has permission to call PM function', async () => { + await dao.revoke( + dao.address, + dao.address, + DAO_PERMISSIONS.ROOT_PERMISSION_ID + ); + await dao.revoke( + dao.address, + ownerAddress, + DAO_PERMISSIONS.ROOT_PERMISSION_ID + ); + + const iface = new ethers.utils.Interface(DAO__factory.abi); + + const action = { + to: dao.address, + data: iface.encodeFunctionData('grant', [ + ownerAddress, + ownerAddress, + DAO_PERMISSIONS.EXECUTE_PERMISSION_ID, + ]), + value: 0, + }; + + await expect(dao.execute(ZERO_BYTES32, [action], 0)) + .to.be.revertedWithCustomError(dao, 'ActionFailed') + .withArgs(0); + expect( + await dao.hasPermission( + ownerAddress, + ownerAddress, + DAO_PERMISSIONS.EXECUTE_PERMISSION_ID, + '0x' + ) + ).to.be.false; + }); + + it('succeeds if only dao has permission to call PM function', async () => { + await dao.grant( + dao.address, + dao.address, + DAO_PERMISSIONS.ROOT_PERMISSION_ID + ); + await dao.revoke( + dao.address, + ownerAddress, + DAO_PERMISSIONS.ROOT_PERMISSION_ID + ); + + const iface = new ethers.utils.Interface(DAO__factory.abi); + + const action = { + to: dao.address, + data: iface.encodeFunctionData('grant', [ + ownerAddress, + ownerAddress, + DAO_PERMISSIONS.EXECUTE_PERMISSION_ID, + ]), + value: 0, + }; + + await expect(dao.execute(ZERO_BYTES32, [action], 0)).to.not.be.reverted; + expect( + await dao.hasPermission( + ownerAddress, + ownerAddress, + DAO_PERMISSIONS.EXECUTE_PERMISSION_ID, + '0x' + ) + ).to.be.true; + }); + + it('succeeds if only caller has permission to call PM function', async () => { + await dao.revoke( + dao.address, + dao.address, + DAO_PERMISSIONS.ROOT_PERMISSION_ID + ); + await dao.grant( + dao.address, + ownerAddress, + DAO_PERMISSIONS.ROOT_PERMISSION_ID + ); + + const iface = new ethers.utils.Interface(DAO__factory.abi); + + const action = { + to: dao.address, + data: iface.encodeFunctionData('grant', [ + ownerAddress, + ownerAddress, + DAO_PERMISSIONS.EXECUTE_PERMISSION_ID, + ]), + value: 0, + }; + + await expect(dao.execute(ZERO_BYTES32, [action], 0)).to.not.be.reverted; + expect( + await dao.hasPermission( + ownerAddress, + ownerAddress, + DAO_PERMISSIONS.EXECUTE_PERMISSION_ID, + '0x' + ) + ).to.be.true; + }); + it('succeeds if action is failable but allowFailureMap allows it', async () => { let num = ethers.BigNumber.from(0); num = flipBit(0, num); @@ -651,7 +884,7 @@ describe('DAO', function () { it('reverts if failure is allowed but not enough gas is provided (many actions)', async () => { const GasConsumer = new GasConsumer__factory(signers[0]); - let gasConsumer = await GasConsumer.deploy(); + const gasConsumer = await GasConsumer.deploy(); // Prepare an action array calling `consumeGas` twenty times. const gasConsumingAction = { @@ -672,7 +905,7 @@ describe('DAO', function () { // Provide too little gas so that the last `to.call` fails, but the remaining gas is enough to finish the subsequent operations. await expect( dao.execute(ZERO_BYTES32, [gasConsumingAction], allowFailureMap, { - gasLimit: expectedGas.sub(3000), + gasLimit: expectedGas.sub(3200), }) ).to.be.revertedWithCustomError(dao, 'InsufficientGas'); @@ -686,12 +919,12 @@ describe('DAO', function () { it('reverts if failure is allowed but not enough gas is provided (one action)', async () => { const GasConsumer = new GasConsumer__factory(signers[0]); - let gasConsumer = await GasConsumer.deploy(); + const gasConsumer = await GasConsumer.deploy(); // Prepare an action array calling `consumeGas` one times. const gasConsumingAction = { to: gasConsumer.address, - data: GasConsumer.interface.encodeFunctionData('consumeGas', [1]), + data: GasConsumer.interface.encodeFunctionData('consumeGas', [2]), value: 0, }; @@ -751,6 +984,91 @@ describe('DAO', function () { const newBalance = await ethers.provider.getBalance(recipient); expect(newBalance.sub(currentBalance)).to.equal(amount); }); + + it('reverts if ETH transfer permission is created and caller lacks it, but still holds EXECUTE_PERMISSION_ID', async () => { + // put native tokens into the DAO + await dao.deposit( + ethers.constants.AddressZero, + amount, + 'ref', + options + ); + + const caller = signers[0].address; + + // make sure caller still has EXECUTE_PERMISSION + expect( + await dao.hasPermission( + dao.address, + caller, + DAO_PERMISSIONS.EXECUTE_PERMISSION_ID, + '0x' + ) + ).to.be.true; + + // create transfer action + const action = { + to: signers[1].address, + value: amount, + data: '0x', + }; + + const permissionId = ethers.utils.keccak256(action.data); + + // create a permission where permissionId is the hash of empty data. + await dao.createPermission( + action.to, + permissionId, + signers[1].address, + [] + ); + + // This must fail as caller doesn't have this specific permission + await expect(dao.execute(ZERO_BYTES32, [action], 0)) + .to.be.revertedWithCustomError(dao, 'Unauthorized') + .withArgs(action.to, caller, permissionId); + }); + + it('succeeds if ETH transfer permission is created and caller holds it, but but lacks EXECUTE_PERMISSION_ID', async () => { + // put native tokens into the DAO + await dao.deposit( + ethers.constants.AddressZero, + amount, + 'ref', + options + ); + + // use caller that doesn't hold EXECUTE_PERMISSION in which case + // it still should succeed. + const caller = signers[2]; + + // create transfer action + const action = { + to: signers[1].address, + value: amount, + data: '0x', + }; + + const recipient = action.to; + const currentBalance = await ethers.provider.getBalance(recipient); + + const permissionId = ethers.utils.keccak256(action.data); + + // create a permission where permissionId is the hash of empty data. + await dao.createPermission( + action.to, + permissionId, + signers[1].address, + [caller.address] // grant to caller + ); + + // This must fail as caller doesn't have this specific permission + await expect(dao.connect(caller).execute(ZERO_BYTES32, [action], 0)) + .to.not.be.reverted; + + const newBalance = await ethers.provider.getBalance(recipient); + expect(newBalance.sub(currentBalance)).to.equal(amount); + }); }); describe('ERC20 Transfer', async () => { diff --git a/packages/contracts/test/core/permission/permission-manager.ts b/packages/contracts/test/core/permission/permission-manager.ts index b62456972..f29a2d6ab 100644 --- a/packages/contracts/test/core/permission/permission-manager.ts +++ b/packages/contracts/test/core/permission/permission-manager.ts @@ -12,6 +12,7 @@ import {expect} from 'chai'; import {ethers} from 'hardhat'; const ADMIN_PERMISSION_ID = ethers.utils.id('ADMIN_PERMISSION'); +const APPLY_TARGET_PERMISSION_ID = ethers.utils.id('APPLY_TARGET_PERMISSION'); const RESTRICTED_PERMISSIONS_FOR_ANY_ADDR = [ DAO_PERMISSIONS.ROOT_PERMISSION_ID, ethers.utils.id('TEST_PERMISSION_1'), @@ -36,6 +37,14 @@ interface SingleTargetPermission { permissionId: string; } +const GRANT_OWNER_FLAG = 1; +const REVOKE_OWNER_FLAG = 2; +const FULL_OWNER_FLAG = 3; +const FREEZE_OWNER = '0x0000000000000000000000000000000000000001'; + +const someWhere = '0xb794F5eA0ba39494cE839613fffBA74279579268'; +const somePermissionId = ethers.utils.id('SOME_PERMISSION'); + describe('Core: PermissionManager', function () { let pm: PermissionManagerTest; let signers: SignerWithAddress[]; @@ -77,287 +86,1427 @@ describe('Core: PermissionManager', function () { }); }); - describe('grant', () => { - it('should add permission', async () => { - await pm.grant(pm.address, otherSigner.address, ADMIN_PERMISSION_ID); - const permission = await pm.getAuthPermission( + describe('addOwner', () => { + beforeEach(async () => { + await pm.createPermission( pm.address, - otherSigner.address, - ADMIN_PERMISSION_ID + ADMIN_PERMISSION_ID, + ownerSigner.address, + [] ); - expect(permission).to.be.equal(ALLOW_FLAG); }); - it('reverts if both `_who == ANY_ADDR` and `_where == ANY_ADDR', async () => { + it('should add an owner with all the flags passed', async () => { await expect( - pm.grant(ANY_ADDR, ANY_ADDR, DAO_PERMISSIONS.ROOT_PERMISSION_ID) - ).to.be.revertedWithCustomError(pm, 'PermissionsForAnyAddressDisallowed'); - }); - - it('reverts if permissionId is restricted and `_who == ANY_ADDR` or `_where == ANY_ADDR`', async () => { - for (let i = 0; i < RESTRICTED_PERMISSIONS_FOR_ANY_ADDR.length; i++) { - await expect( - pm.grant(pm.address, ANY_ADDR, RESTRICTED_PERMISSIONS_FOR_ANY_ADDR[i]) - ).to.be.revertedWithCustomError( - pm, - 'PermissionsForAnyAddressDisallowed' - ); - await expect( - pm.grant(ANY_ADDR, pm.address, RESTRICTED_PERMISSIONS_FOR_ANY_ADDR[i]) - ).to.be.revertedWithCustomError( - pm, - 'PermissionsForAnyAddressDisallowed' + pm.addOwner( + pm.address, + ADMIN_PERMISSION_ID, + otherSigner.address, + FULL_OWNER_FLAG + ) + ) + .to.emit(pm, 'OwnerAdded') + .withArgs( + pm.address, + ADMIN_PERMISSION_ID, + otherSigner.address, + FULL_OWNER_FLAG ); - } - }); - - it('reverts if permissionId is not restricted and`_who == ANY_ADDR` or `_where == ANY_ADDR`', async () => { - await expect( - pm.grant(pm.address, ANY_ADDR, ADMIN_PERMISSION_ID) - ).to.be.revertedWithCustomError(pm, 'PermissionsForAnyAddressDisallowed'); - - await expect( - pm.grant(ANY_ADDR, pm.address, ADMIN_PERMISSION_ID) - ).to.be.revertedWithCustomError(pm, 'PermissionsForAnyAddressDisallowed'); - }); - - it('should emit Granted', async () => { - await expect( - pm.grant(pm.address, otherSigner.address, ADMIN_PERMISSION_ID) - ).to.emit(pm, 'Granted'); - }); - it('should not emit granted event if already granted', async () => { - await pm.grant(pm.address, otherSigner.address, ADMIN_PERMISSION_ID); - await expect( - pm.grant(pm.address, otherSigner.address, ADMIN_PERMISSION_ID) - ).to.not.emit(pm, 'Granted'); + expect( + await pm.getPermissionFlags( + pm.address, + ADMIN_PERMISSION_ID, + otherSigner.address + ) + ).to.deep.equal([FULL_OWNER_FLAG, 0]); }); - it('should not allow grant', async () => { + it('should add an owner with only specific flags passed', async () => { await expect( - pm - .connect(otherSigner) - .grant(pm.address, otherSigner.address, ADMIN_PERMISSION_ID) + pm.addOwner( + pm.address, + ADMIN_PERMISSION_ID, + otherSigner.address, + GRANT_OWNER_FLAG + ) ) - .to.be.revertedWithCustomError(pm, 'Unauthorized') + .to.emit(pm, 'OwnerAdded') .withArgs( pm.address, + ADMIN_PERMISSION_ID, otherSigner.address, - DAO_PERMISSIONS.ROOT_PERMISSION_ID + GRANT_OWNER_FLAG ); - }); - it('should not allow for non ROOT', async () => { - await pm.grant(pm.address, ownerSigner.address, ADMIN_PERMISSION_ID); + expect( + await pm.getPermissionFlags( + pm.address, + ADMIN_PERMISSION_ID, + otherSigner.address + ) + ).to.deep.equal([GRANT_OWNER_FLAG, 0]); + await expect( - pm - .connect(otherSigner) - .grant( - pm.address, - otherSigner.address, - DAO_PERMISSIONS.ROOT_PERMISSION_ID - ) + pm.addOwner( + pm.address, + ADMIN_PERMISSION_ID, + otherSigner.address, + REVOKE_OWNER_FLAG + ) ) - .to.be.revertedWithCustomError(pm, 'Unauthorized') + .to.emit(pm, 'OwnerAdded') .withArgs( pm.address, + ADMIN_PERMISSION_ID, otherSigner.address, - DAO_PERMISSIONS.ROOT_PERMISSION_ID + REVOKE_OWNER_FLAG ); - }); - }); - describe('grantWithCondition', () => { - before(async () => { - conditionMock = await new PermissionConditionMock__factory( - signers[0] - ).deploy(); + expect( + await pm.getPermissionFlags( + pm.address, + ADMIN_PERMISSION_ID, + otherSigner.address + ) + ).to.deep.equal([FULL_OWNER_FLAG, 0]); }); - it('reverts if the condition address is not a contract', async () => { + it('should correctly increase owner counters', async () => { + const newOwner = otherSigner.address; + + await pm.addOwner( + pm.address, + ADMIN_PERMISSION_ID, + newOwner, + GRANT_OWNER_FLAG + ); + + expect( + await pm.getPermissionStatus(pm.address, ADMIN_PERMISSION_ID) + ).to.deep.equal([true, 2, 1]); + + // If the same flag that `newOwner` already holds is added, + // it shouldn't increase counts and neither emit the event await expect( - pm.grantWithCondition( + pm.addOwner(pm.address, ADMIN_PERMISSION_ID, newOwner, GRANT_OWNER_FLAG) + ).to.not.emit(pm, 'OwnerAdded'); + + expect( + await pm.getPermissionStatus(pm.address, ADMIN_PERMISSION_ID) + ).to.deep.equal([true, 2, 1]); + + // Add the same owner but with revoke which should increase revoke counter only. + await expect( + pm.addOwner( pm.address, - otherSigner.address, ADMIN_PERMISSION_ID, - ethers.constants.AddressZero + newOwner, + REVOKE_OWNER_FLAG ) - ) - .to.be.revertedWithCustomError(pm, 'ConditionNotAContract') - .withArgs(ethers.constants.AddressZero); + ).to.emit(pm, 'OwnerAdded'); + + expect( + await pm.getPermissionStatus(pm.address, ADMIN_PERMISSION_ID) + ).to.deep.equal([true, 2, 2]); }); - it('reverts if the condition contract does not support `IPermissionConditon`', async () => { - const nonConditionContract = - await new PluginUUPSUpgradeableV1Mock__factory(signers[0]).deploy(); + it('should revert if a zero flag is passed', async () => { + await expect( + pm.addOwner(pm.address, ADMIN_PERMISSION_ID, otherSigner.address, 0) + ).to.be.revertedWithCustomError(pm, 'InvalidEmptyFlag'); + }); + it('should revert if an _owner passed is address(0)', async () => { await expect( - pm.grantWithCondition( + pm.addOwner( pm.address, - otherSigner.address, ADMIN_PERMISSION_ID, - nonConditionContract.address + ethers.constants.AddressZero, + FULL_OWNER_FLAG ) - ) - .to.be.revertedWithCustomError(pm, 'ConditionInterfaceNotSupported') - .withArgs(nonConditionContract.address); + ).to.be.revertedWithCustomError(pm, 'ZeroAddress'); }); - it('should add permission', async () => { - await pm.grantWithCondition( + it('should revert if _where passed is address(0)', async () => { + await expect( + pm.addOwner( + ethers.constants.AddressZero, + ADMIN_PERMISSION_ID, + otherSigner.address, + FULL_OWNER_FLAG + ) + ).to.be.revertedWithCustomError(pm, 'ZeroAddress'); + }); + + it('should revert if the permission is frozen', async () => { + const freezeAddr = FREEZE_OWNER; + + await pm.addOwner( pm.address, - otherSigner.address, ADMIN_PERMISSION_ID, - conditionMock.address - ); - const permission = await pm.getAuthPermission( - pm.address, - otherSigner.address, - ADMIN_PERMISSION_ID + freezeAddr, + FULL_OWNER_FLAG ); - expect(permission).to.be.equal(conditionMock.address); - }); - it('should emit Granted', async () => { + await pm.removeOwner(pm.address, ADMIN_PERMISSION_ID, FULL_OWNER_FLAG); + await expect( - pm.grantWithCondition( + pm.addOwner( pm.address, - otherSigner.address, ADMIN_PERMISSION_ID, - conditionMock.address + otherSigner.address, + FULL_OWNER_FLAG ) - ).to.emit(pm, 'Granted'); + ) + .to.be.revertedWithCustomError(pm, 'PermissionFrozen') + .withArgs(pm.address, ADMIN_PERMISSION_ID); }); - it('should emit Granted when condition is present and `who == ANY_ADDR` or `where == ANY_ADDR`', async () => { + it('should revert if the caller does not have ownership for revoke', async () => { + const someAddress = await ethers.Wallet.createRandom().getAddress(); + + await pm.removeOwner(pm.address, ADMIN_PERMISSION_ID, REVOKE_OWNER_FLAG); + await expect( - pm.grantWithCondition( + pm.addOwner( pm.address, - ANY_ADDR, ADMIN_PERMISSION_ID, - conditionMock.address + someAddress, + REVOKE_OWNER_FLAG ) - ).to.emit(pm, 'Granted'); + ) + .to.be.revertedWithCustomError(pm, 'UnauthorizedOwner') + .withArgs(ownerSigner.address, GRANT_OWNER_FLAG, REVOKE_OWNER_FLAG); + // Note that it should still have grant owner capability. await expect( - pm.grantWithCondition( - ANY_ADDR, + pm.addOwner( pm.address, ADMIN_PERMISSION_ID, - conditionMock.address + someAddress, + GRANT_OWNER_FLAG ) - ).to.emit(pm, 'Granted'); + ).to.not.be.reverted; }); - it('should not emit Granted with already granted with the same condition or ALLOW_FLAG', async () => { - await pm.grantWithCondition( - pm.address, - otherSigner.address, - ADMIN_PERMISSION_ID, - conditionMock.address - ); + it('should revert if the caller does not have ownership for grant', async () => { + const someAddress = await ethers.Wallet.createRandom().getAddress(); + + await pm.removeOwner(pm.address, ADMIN_PERMISSION_ID, GRANT_OWNER_FLAG); + await expect( - pm.grantWithCondition( + pm.addOwner( pm.address, - otherSigner.address, ADMIN_PERMISSION_ID, - conditionMock.address + someAddress, + GRANT_OWNER_FLAG ) - ).to.not.emit(pm, 'Granted'); + ) + .to.be.revertedWithCustomError(pm, 'UnauthorizedOwner') + .withArgs(ownerSigner.address, REVOKE_OWNER_FLAG, GRANT_OWNER_FLAG); + + // Note that it should still have revoke owner capability. + await expect( + pm.addOwner( + pm.address, + ADMIN_PERMISSION_ID, + someAddress, + REVOKE_OWNER_FLAG + ) + ).to.not.be.reverted; }); - it('reverts if tries to grant the same permission, but with different condition', async () => { - await pm.grantWithCondition( + it('should not emit event if owner already holds the flags that is being added', async () => { + const owner = signers[2]; + + await pm.addOwner( pm.address, - otherSigner.address, ADMIN_PERMISSION_ID, - conditionMock.address + owner.address, + FULL_OWNER_FLAG ); - const newConditionMock = await new PermissionConditionMock__factory( - signers[0] - ).deploy(); - await expect( - pm.grantWithCondition( + pm.addOwner( pm.address, - otherSigner.address, ADMIN_PERMISSION_ID, - newConditionMock.address - ) - ) - .to.be.revertedWithCustomError( - pm, - 'PermissionAlreadyGrantedForDifferentCondition' + owner.address, + GRANT_OWNER_FLAG ) - .withArgs( - pm.address, - otherSigner.address, - ADMIN_PERMISSION_ID, - conditionMock.address, - newConditionMock.address - ); + ).to.not.emit(pm, 'OwnerAdded'); }); + }); - it('should set PermissionCondition', async () => { - await pm.grantWithCondition( - pm.address, - otherSigner.address, + describe('removeOwner', () => { + beforeEach(async () => { + await pm.createPermission( + someWhere, ADMIN_PERMISSION_ID, - conditionMock.address + ownerSigner.address, + [] ); + }); + + it('should remove the FULL_OWNER_FLAGS from the owner', async () => { + await expect( + pm.removeOwner(someWhere, ADMIN_PERMISSION_ID, FULL_OWNER_FLAG) + ) + .to.emit(pm, 'OwnerRenounced') + .withArgs(someWhere, ADMIN_PERMISSION_ID, ownerSigner.address, 0, 0); + expect( - await pm.getAuthPermission( - pm.address, - otherSigner.address, - ADMIN_PERMISSION_ID + await pm.getPermissionFlags( + someWhere, + ADMIN_PERMISSION_ID, + ownerSigner.address ) - ).to.be.equal(conditionMock.address); + ).to.deep.equal([0, 0]); }); - it('should not allow grant', async () => { + it('should remove the specific flag from the owner', async () => { await expect( - pm - .connect(otherSigner) - .grantWithCondition( - pm.address, - otherSigner.address, - ADMIN_PERMISSION_ID, - conditionMock.address - ) + pm.removeOwner(someWhere, ADMIN_PERMISSION_ID, GRANT_OWNER_FLAG) ) - .to.be.revertedWithCustomError(pm, 'Unauthorized') + .to.emit(pm, 'OwnerRenounced') .withArgs( - pm.address, + someWhere, + ADMIN_PERMISSION_ID, + ownerSigner.address, + REVOKE_OWNER_FLAG, + 0 + ); + + expect( + await pm.getPermissionFlags( + someWhere, + ADMIN_PERMISSION_ID, + ownerSigner.address + ) + ).to.deep.equal([REVOKE_OWNER_FLAG, 0]); + + await expect( + pm.removeOwner(someWhere, ADMIN_PERMISSION_ID, REVOKE_OWNER_FLAG) + ) + .to.emit(pm, 'OwnerRenounced') + .withArgs(someWhere, ADMIN_PERMISSION_ID, ownerSigner.address, 0, 0); + + expect( + await pm.getPermissionFlags( + someWhere, + ADMIN_PERMISSION_ID, + ownerSigner.address + ) + ).to.deep.equal([0, 0]); + }); + + it('should revert if a zero flag is passed', async () => { + await expect( + pm.removeOwner(someWhere, ADMIN_PERMISSION_ID, 0) + ).to.be.revertedWithCustomError(pm, 'InvalidEmptyFlag'); + }); + + it("should revert if flags that don't exist are removed", async () => { + await pm.removeOwner(someWhere, ADMIN_PERMISSION_ID, GRANT_OWNER_FLAG); + + await expect( + pm.removeOwner(someWhere, ADMIN_PERMISSION_ID, GRANT_OWNER_FLAG) + ) + .to.be.revertedWithCustomError(pm, 'InvalidMissingFlags') + .withArgs(REVOKE_OWNER_FLAG, GRANT_OWNER_FLAG); + }); + + it('should correctly decrease owner counters', async () => { + expect( + await pm.getPermissionStatus(someWhere, ADMIN_PERMISSION_ID) + ).to.deep.equal([true, 1, 1]); + + const newOwner = otherSigner.address; + + await pm.addOwner( + someWhere, + ADMIN_PERMISSION_ID, + newOwner, + FULL_OWNER_FLAG + ); + + expect( + await pm.getPermissionStatus(someWhere, ADMIN_PERMISSION_ID) + ).to.deep.equal([true, 2, 2]); + + await pm + .connect(otherSigner) + .removeOwner(someWhere, ADMIN_PERMISSION_ID, GRANT_OWNER_FLAG); + + expect( + await pm.getPermissionStatus(someWhere, ADMIN_PERMISSION_ID) + ).to.deep.equal([true, 1, 2]); + + await pm + .connect(otherSigner) + .removeOwner(someWhere, ADMIN_PERMISSION_ID, REVOKE_OWNER_FLAG); + + expect( + await pm.getPermissionStatus(someWhere, ADMIN_PERMISSION_ID) + ).to.deep.equal([true, 1, 1]); + }); + + it('should also remove delegatee flags when removing the owner flags', async () => { + const owner = signers[2]; + + await pm.addOwner( + someWhere, + ADMIN_PERMISSION_ID, + owner.address, + FULL_OWNER_FLAG + ); + + await pm.delegatePermission( + someWhere, + ADMIN_PERMISSION_ID, + owner.address, + FULL_OWNER_FLAG + ); + + await expect( + pm + .connect(owner) + .removeOwner(someWhere, ADMIN_PERMISSION_ID, REVOKE_OWNER_FLAG) + ) + .to.emit(pm, 'OwnerRenounced') + .withArgs( + someWhere, + ADMIN_PERMISSION_ID, + owner.address, + GRANT_OWNER_FLAG, + GRANT_OWNER_FLAG + ); + }); + }); + + describe('createPermission', () => { + it('should only be callable by the ROOT user', async () => { + await expect( + pm.createPermission( + pm.address, + ADMIN_PERMISSION_ID, + otherSigner.address, + [] + ) + ) + .to.emit(pm, 'PermissionCreated') + .withArgs(pm.address, ADMIN_PERMISSION_ID, otherSigner.address, []); + + await expect( + pm + .connect(otherSigner) + .createPermission( + pm.address, + ethers.utils.id('TEST_PERMISSION_1'), + otherSigner.address, + [] + ) + ) + .to.be.revertedWithCustomError(pm, 'Unauthorized') + .withArgs( + pm.address, + otherSigner.address, + DAO_PERMISSIONS.ROOT_PERMISSION_ID + ); + }); + + it('should revert in case the permission is already existing', async () => { + await pm.createPermission( + pm.address, + ADMIN_PERMISSION_ID, + otherSigner.address, + [] + ); + + await expect( + pm.createPermission( + pm.address, + ADMIN_PERMISSION_ID, + otherSigner.address, + [] + ) + ).to.be.revertedWithCustomError(pm, 'PermissionAlreadyCreated'); + }); + + it('should create a permission and call grant for all whos passed', async () => { + const someAddress = await ethers.Wallet.createRandom().getAddress(); + await expect( + pm.createPermission( + pm.address, + ADMIN_PERMISSION_ID, + otherSigner.address, + [someAddress] + ) + ) + .to.emit(pm, 'PermissionCreated') + .withArgs(pm.address, ADMIN_PERMISSION_ID, otherSigner.address, [ + someAddress, + ]); + + expect( + await pm.isGranted(pm.address, someAddress, ADMIN_PERMISSION_ID, '0x') + ).to.be.true; + }); + + it('should create a permission with an owner of full capability', async () => { + const someAddress = await ethers.Wallet.createRandom().getAddress(); + await expect( + pm.createPermission( + pm.address, + ADMIN_PERMISSION_ID, + otherSigner.address, + [someAddress] + ) + ) + .to.emit(pm, 'PermissionCreated') + .withArgs(pm.address, ADMIN_PERMISSION_ID, otherSigner.address, [ + someAddress, + ]); + + expect( + await pm.getPermissionFlags( + pm.address, + ADMIN_PERMISSION_ID, + otherSigner.address + ) + ).to.deep.equal([FULL_OWNER_FLAG, 0]); + + expect( + await pm.getPermissionStatus(pm.address, ADMIN_PERMISSION_ID) + ).to.deep.equal([true, 1, 1]); + }); + }); + + describe('delegatePermission', () => { + beforeEach(async () => { + await pm.createPermission( + pm.address, + ADMIN_PERMISSION_ID, + ownerSigner.address, + [] + ); + }); + + it('should revert if a zero flag is passed', async () => { + await expect( + pm.delegatePermission( + pm.address, + ADMIN_PERMISSION_ID, + otherSigner.address, + 0 + ) + ).to.be.revertedWithCustomError(pm, 'InvalidEmptyFlag'); + }); + + it('should revert if the permission is actually frozen', async () => { + await pm.addOwner( + pm.address, + ADMIN_PERMISSION_ID, + FREEZE_OWNER, + FULL_OWNER_FLAG + ); + await pm.removeOwner(pm.address, ADMIN_PERMISSION_ID, FULL_OWNER_FLAG); + + await expect( + pm.delegatePermission( + pm.address, + ADMIN_PERMISSION_ID, + otherSigner.address, + FULL_OWNER_FLAG + ) + ) + .to.be.revertedWithCustomError(pm, 'PermissionFrozen') + .withArgs(pm.address, ADMIN_PERMISSION_ID); + }); + + it('should revert if the caller is not the owner of the permission he/she wants to delegate', async () => { + await expect( + pm + .connect(otherSigner) + .delegatePermission( + pm.address, + ADMIN_PERMISSION_ID, + otherSigner.address, + FULL_OWNER_FLAG + ) + ) + .to.be.revertedWithCustomError(pm, 'UnauthorizedOwner') + .withArgs(otherSigner.address, 0, FULL_OWNER_FLAG); + }); + + it('should emit PermissionDelegated and correctly store the flags', async () => { + await expect( + pm.delegatePermission( + pm.address, + ADMIN_PERMISSION_ID, + otherSigner.address, + GRANT_OWNER_FLAG + ) + ) + .to.emit(pm, 'PermissionDelegated') + .withArgs( + pm.address, + ADMIN_PERMISSION_ID, + otherSigner.address, + GRANT_OWNER_FLAG + ); + + expect( + await pm.getPermissionFlags( + pm.address, + ADMIN_PERMISSION_ID, + otherSigner.address + ) + ).to.deep.equal([0, GRANT_OWNER_FLAG]); + + await expect( + pm.delegatePermission( + pm.address, + ADMIN_PERMISSION_ID, + otherSigner.address, + REVOKE_OWNER_FLAG + ) + ) + .to.emit(pm, 'PermissionDelegated') + .withArgs( + pm.address, + ADMIN_PERMISSION_ID, + otherSigner.address, + FULL_OWNER_FLAG + ); + + expect( + await pm.getPermissionFlags( + pm.address, + ADMIN_PERMISSION_ID, + otherSigner.address + ) + ).to.deep.equal([0, FULL_OWNER_FLAG]); + }); + + it('should not emit PermissionDelegated if the same flags are added', async () => { + const delegatee = signers[2]; + + await pm.delegatePermission( + pm.address, + ADMIN_PERMISSION_ID, + delegatee.address, + FULL_OWNER_FLAG + ); + + await expect( + pm.delegatePermission( + pm.address, + ADMIN_PERMISSION_ID, + delegatee.address, + GRANT_OWNER_FLAG + ) + ).to.not.emit(pm, 'PermissionDelegated'); + }); + }); + + describe('undelegatePermission', () => { + beforeEach(async () => { + await pm.createPermission( + pm.address, + ADMIN_PERMISSION_ID, + ownerSigner.address, + [] + ); + + await pm.delegatePermission( + pm.address, + ADMIN_PERMISSION_ID, + otherSigner.address, + FULL_OWNER_FLAG + ); + }); + + it('should revert when a zero flag is passed', async () => { + await expect( + pm.undelegatePermission( + pm.address, + ADMIN_PERMISSION_ID, + otherSigner.address, + 0 + ) + ).to.be.revertedWithCustomError(pm, 'InvalidEmptyFlag'); + }); + + it('should revert if the caller is not the owner of the permission he/she wants to undelegate', async () => { + await expect( + pm + .connect(otherSigner) + .undelegatePermission( + pm.address, + ADMIN_PERMISSION_ID, + otherSigner.address, + FULL_OWNER_FLAG + ) + ) + .to.be.revertedWithCustomError(pm, 'UnauthorizedOwner') + .withArgs(otherSigner.address, 0, FULL_OWNER_FLAG); + }); + + it('should revert if flags to undelegate are not delegated', async () => { + const bob = signers[3]; + await pm.delegatePermission( + pm.address, + ADMIN_PERMISSION_ID, + bob.address, + GRANT_OWNER_FLAG + ); + + await expect( + pm.undelegatePermission( + pm.address, + ADMIN_PERMISSION_ID, + bob.address, + FULL_OWNER_FLAG + ) + ) + .to.be.revertedWithCustomError(pm, 'InvalidMissingFlags') + .withArgs(GRANT_OWNER_FLAG, FULL_OWNER_FLAG); + }); + + it('should emit PermissionUndelegated and correctly update the flags', async () => { + await expect( + pm.undelegatePermission( + pm.address, + ADMIN_PERMISSION_ID, + otherSigner.address, + REVOKE_OWNER_FLAG + ) + ) + .to.emit(pm, 'PermissionUndelegated') + .withArgs( + pm.address, + ADMIN_PERMISSION_ID, + otherSigner.address, + GRANT_OWNER_FLAG + ); + + expect( + await pm.getPermissionFlags( + pm.address, + ADMIN_PERMISSION_ID, + otherSigner.address + ) + ).to.deep.equal([0, GRANT_OWNER_FLAG]); + }); + }); + + describe('grant', () => { + it('should add permission', async () => { + await pm.grant(pm.address, otherSigner.address, ADMIN_PERMISSION_ID); + const permission = await pm.getAuthPermission( + pm.address, + otherSigner.address, + ADMIN_PERMISSION_ID + ); + expect(permission).to.be.equal(ALLOW_FLAG); + }); + + it('reverts if both `_who == ANY_ADDR` and `_where == ANY_ADDR', async () => { + await expect( + pm.grant(ANY_ADDR, ANY_ADDR, DAO_PERMISSIONS.ROOT_PERMISSION_ID) + ).to.be.revertedWithCustomError(pm, 'PermissionsForAnyAddressDisallowed'); + }); + + it('reverts if permissionId is restricted and `_who == ANY_ADDR` or `_where == ANY_ADDR`', async () => { + for (let i = 0; i < RESTRICTED_PERMISSIONS_FOR_ANY_ADDR.length; i++) { + await expect( + pm.grant(pm.address, ANY_ADDR, RESTRICTED_PERMISSIONS_FOR_ANY_ADDR[i]) + ).to.be.revertedWithCustomError( + pm, + 'PermissionsForAnyAddressDisallowed' + ); + await expect( + pm.grant(ANY_ADDR, pm.address, RESTRICTED_PERMISSIONS_FOR_ANY_ADDR[i]) + ).to.be.revertedWithCustomError( + pm, + 'PermissionsForAnyAddressDisallowed' + ); + } + }); + + it('reverts if permissionId is not restricted and`_who == ANY_ADDR` or `_where == ANY_ADDR`', async () => { + await expect( + pm.grant(pm.address, ANY_ADDR, ADMIN_PERMISSION_ID) + ).to.be.revertedWithCustomError(pm, 'PermissionsForAnyAddressDisallowed'); + + await expect( + pm.grant(ANY_ADDR, pm.address, ADMIN_PERMISSION_ID) + ).to.be.revertedWithCustomError(pm, 'PermissionsForAnyAddressDisallowed'); + }); + + it('reverts if special `APPLY_TARGET_PERMISSION_ID` is granted to non-allowed address', async () => { + const allowedApplyTargetMethodGrantee = + await pm.applyTargetMethodGrantee(); + + await expect( + pm.grant(pm.address, otherSigner.address, APPLY_TARGET_PERMISSION_ID) + ) + .to.be.revertedWithCustomError( + pm, + 'UnrecognizedApplyPermissionsGrantee' + ) + .withArgs(allowedApplyTargetMethodGrantee); + + // It should succeed if it's allowed. + await pm.setApplyTargetMethodGrantee(otherSigner.address); + + await expect( + pm.grant(pm.address, otherSigner.address, APPLY_TARGET_PERMISSION_ID) + ).to.not.be.reverted; + }); + + it('should emit Granted', async () => { + await expect( + pm.grant(pm.address, otherSigner.address, ADMIN_PERMISSION_ID) + ).to.emit(pm, 'Granted'); + }); + + it('should not emit granted event if already granted', async () => { + await pm.grant(pm.address, otherSigner.address, ADMIN_PERMISSION_ID); + await expect( + pm.grant(pm.address, otherSigner.address, ADMIN_PERMISSION_ID) + ).to.not.emit(pm, 'Granted'); + }); + + it('should not allow grant', async () => { + await expect( + pm + .connect(otherSigner) + .grant(pm.address, otherSigner.address, ADMIN_PERMISSION_ID) + ) + .to.be.revertedWithCustomError(pm, 'Unauthorized') + .withArgs(pm.address, otherSigner.address, ADMIN_PERMISSION_ID); + }); + + it('should not allow for non ROOT', async () => { + await pm.grant(pm.address, ownerSigner.address, ADMIN_PERMISSION_ID); + await expect( + pm + .connect(otherSigner) + .grant( + pm.address, + otherSigner.address, + DAO_PERMISSIONS.ROOT_PERMISSION_ID + ) + ) + .to.be.revertedWithCustomError(pm, 'Unauthorized') + .withArgs( + pm.address, + otherSigner.address, + DAO_PERMISSIONS.ROOT_PERMISSION_ID + ); + }); + + it('should revert if the permission is frozen', async () => { + await pm + .connect(ownerSigner) + .createPermission(someWhere, somePermissionId, ownerSigner.address, [ + ownerSigner.address, + ]); + + await pm + .connect(ownerSigner) + .addOwner(someWhere, somePermissionId, FREEZE_OWNER, GRANT_OWNER_FLAG); + + await pm + .connect(ownerSigner) + .removeOwner(someWhere, somePermissionId, FULL_OWNER_FLAG); + + await expect( + pm + .connect(ownerSigner) + .grant(someWhere, otherSigner.address, somePermissionId) + ) + .to.be.revertedWithCustomError(pm, 'PermissionFrozen') + .withArgs(someWhere, somePermissionId); + }); + + it('should revert if the caller does not have the grant flag set', async () => { + await pm + .connect(ownerSigner) + .createPermission(someWhere, somePermissionId, ownerSigner.address, [ + otherSigner.address, + ]); + + await expect( + pm + .connect(otherSigner) + .grant(someWhere, otherSigner.address, somePermissionId) + ) + .to.be.revertedWithCustomError(pm, 'Unauthorized') + .withArgs(someWhere, otherSigner.address, somePermissionId); + }); + + it('should allow the root user as fallback if the permission is created but no grant owners are existing anymore', async () => { + await pm + .connect(ownerSigner) + .createPermission(someWhere, somePermissionId, otherSigner.address, [ + ownerSigner.address, + ]); + + await pm + .connect(otherSigner) + .removeOwner(someWhere, somePermissionId, GRANT_OWNER_FLAG); + + await expect( + pm + .connect(ownerSigner) + .grant(someWhere, otherSigner.address, somePermissionId) + ).to.emit(pm, 'Granted'); + }); + + it('should allow the root user as fallback if the permission is not created', async () => { + await expect( + pm + .connect(ownerSigner) + .grant(someWhere, otherSigner.address, somePermissionId) + ).to.emit(pm, 'Granted'); + }); + + it('should allow the delegatee to call grant once', async () => { + await pm + .connect(ownerSigner) + .createPermission(someWhere, somePermissionId, ownerSigner.address, [ + ownerSigner.address, + ]); + + await pm + .connect(ownerSigner) + .delegatePermission( + someWhere, + somePermissionId, + otherSigner.address, + GRANT_OWNER_FLAG + ); + + await expect( + pm + .connect(otherSigner) + .grant(someWhere, otherSigner.address, somePermissionId) + ).to.emit(pm, 'Granted'); + + await expect( + pm + .connect(otherSigner) + .grant(someWhere, otherSigner.address, somePermissionId) + ) + .to.be.revertedWithCustomError(pm, 'Unauthorized') + .withArgs(someWhere, otherSigner.address, somePermissionId); + }); + + it('should succeed if a caller is both delegated and an owner but holds different flags', async () => { + let owner = signers[3]; + let alice = signers[4]; + + await pm.createPermission(someWhere, somePermissionId, owner.address, []); + + // Alice became a delegate of `GRANT_OWNER_FLAG` on this permission + await pm + .connect(owner) + .delegatePermission( + someWhere, + somePermissionId, + alice.address, + REVOKE_OWNER_FLAG + ); + await pm + .connect(owner) + .addOwner(someWhere, somePermissionId, alice.address, GRANT_OWNER_FLAG); + + await pm.connect(alice).grant(someWhere, someWhere, somePermissionId); + await pm.connect(alice).revoke(someWhere, someWhere, somePermissionId); + }); + + it('should still keep the flag for delegation if a caller is an owner and delegate and calls grant', async () => { + let owner = signers[4]; + + await pm.createPermission(someWhere, somePermissionId, owner.address, []); + + await pm + .connect(owner) + .delegatePermission( + someWhere, + somePermissionId, + owner.address, + GRANT_OWNER_FLAG + ); + + await pm.connect(owner).grant(someWhere, someWhere, somePermissionId); + + expect( + await pm.getPermissionFlags(someWhere, somePermissionId, owner.address) + ).to.deep.equal([FULL_OWNER_FLAG, GRANT_OWNER_FLAG]); + }); + + it('should still keep `REVOKE_OWNER_FLAG` for delegatee if only `GRANT_OWNER_FLAG` is used/depleted', async () => { + let owner = signers[3]; + let alice = signers[4]; + + await pm.createPermission(someWhere, somePermissionId, owner.address, []); + + // Alice became a delegate of `GRANT_OWNER_FLAG` on this permission + await pm + .connect(owner) + .delegatePermission( + someWhere, + somePermissionId, + alice.address, + FULL_OWNER_FLAG + ); + + await pm.connect(alice).grant(someWhere, someWhere, somePermissionId); + + let currentFlags = await pm.getPermissionFlags( + someWhere, + somePermissionId, + alice.address + ); + expect(currentFlags).to.deep.equal([0, REVOKE_OWNER_FLAG]); + await expect( + pm.connect(alice).revoke(someWhere, someWhere, somePermissionId) + ).to.not.be.reverted; + }); + + it('should revert if undelegate got called before grantWithCondition got called by the delegatee', async () => { + await pm + .connect(ownerSigner) + .createPermission(someWhere, somePermissionId, ownerSigner.address, [ + otherSigner.address, + ]); + + await pm + .connect(ownerSigner) + .delegatePermission( + someWhere, + somePermissionId, + otherSigner.address, + REVOKE_OWNER_FLAG + ); + + await pm + .connect(ownerSigner) + .undelegatePermission( + someWhere, + somePermissionId, + otherSigner.address, + REVOKE_OWNER_FLAG + ); + + await expect( + pm + .connect(otherSigner) + .grant(someWhere, otherSigner.address, somePermissionId) + ) + .to.be.revertedWithCustomError(pm, 'Unauthorized') + .withArgs(someWhere, otherSigner.address, somePermissionId); + }); + }); + + describe('grantWithCondition', () => { + before(async () => { + conditionMock = await new PermissionConditionMock__factory( + signers[0] + ).deploy(); + }); + + it('reverts if the condition address is not a contract', async () => { + await expect( + pm.grantWithCondition( + pm.address, + otherSigner.address, + ADMIN_PERMISSION_ID, + ethers.constants.AddressZero + ) + ) + .to.be.revertedWithCustomError(pm, 'ConditionNotAContract') + .withArgs(ethers.constants.AddressZero); + }); + + it('reverts if the condition contract does not support `IPermissionConditon`', async () => { + const nonConditionContract = + await new PluginUUPSUpgradeableV1Mock__factory(signers[0]).deploy(); + + await expect( + pm.grantWithCondition( + pm.address, + otherSigner.address, + ADMIN_PERMISSION_ID, + nonConditionContract.address + ) + ) + .to.be.revertedWithCustomError(pm, 'ConditionInterfaceNotSupported') + .withArgs(nonConditionContract.address); + }); + + it('reverts if special `APPLY_TARGET_PERMISSION_ID` is granted to non-allowed address', async () => { + const allowedApplyTargetMethodGrantee = + await pm.applyTargetMethodGrantee(); + + await expect( + pm.grantWithCondition( + pm.address, + otherSigner.address, + APPLY_TARGET_PERMISSION_ID, + conditionMock.address + ) + ) + .to.be.revertedWithCustomError( + pm, + 'UnrecognizedApplyPermissionsGrantee' + ) + .withArgs(allowedApplyTargetMethodGrantee); + + // It should succeed if it's allowed. + await pm.setApplyTargetMethodGrantee(otherSigner.address); + + await expect( + pm.grantWithCondition( + pm.address, + otherSigner.address, + APPLY_TARGET_PERMISSION_ID, + conditionMock.address + ) + ).to.not.be.reverted; + }); + + it('reverts if both `_who == ANY_ADDR` and `_where == ANY_ADDR', async () => { + await expect( + pm.grantWithCondition( + ANY_ADDR, + ANY_ADDR, + DAO_PERMISSIONS.ROOT_PERMISSION_ID, + conditionMock.address + ) + ).to.be.revertedWithCustomError(pm, 'AnyAddressDisallowedForWhoAndWhere'); + }); + + it('should add permission', async () => { + await pm.grantWithCondition( + pm.address, + otherSigner.address, + ADMIN_PERMISSION_ID, + conditionMock.address + ); + const permission = await pm.getAuthPermission( + pm.address, + otherSigner.address, + ADMIN_PERMISSION_ID + ); + expect(permission).to.be.equal(conditionMock.address); + }); + + it('should emit Granted', async () => { + await expect( + pm.grantWithCondition( + pm.address, + otherSigner.address, + ADMIN_PERMISSION_ID, + conditionMock.address + ) + ).to.emit(pm, 'Granted'); + }); + + it('should emit Granted when condition is present and `who == ANY_ADDR` or `where == ANY_ADDR`', async () => { + await expect( + pm.grantWithCondition( + pm.address, + ANY_ADDR, + ADMIN_PERMISSION_ID, + conditionMock.address + ) + ).to.emit(pm, 'Granted'); + + await expect( + pm.grantWithCondition( + ANY_ADDR, + pm.address, + ADMIN_PERMISSION_ID, + conditionMock.address + ) + ).to.emit(pm, 'Granted'); + }); + + it('should not emit Granted with already granted with the same condition or ALLOW_FLAG', async () => { + await pm.grantWithCondition( + pm.address, + otherSigner.address, + ADMIN_PERMISSION_ID, + conditionMock.address + ); + await expect( + pm.grantWithCondition( + pm.address, + otherSigner.address, + ADMIN_PERMISSION_ID, + conditionMock.address + ) + ).to.not.emit(pm, 'Granted'); + }); + + it('reverts if tries to grant the same permission, but with different condition', async () => { + await pm.grantWithCondition( + pm.address, + otherSigner.address, + ADMIN_PERMISSION_ID, + conditionMock.address + ); + + const newConditionMock = await new PermissionConditionMock__factory( + signers[0] + ).deploy(); + + await expect( + pm.grantWithCondition( + pm.address, + otherSigner.address, + ADMIN_PERMISSION_ID, + newConditionMock.address + ) + ) + .to.be.revertedWithCustomError( + pm, + 'PermissionAlreadyGrantedForDifferentCondition' + ) + .withArgs( + pm.address, + otherSigner.address, + ADMIN_PERMISSION_ID, + conditionMock.address, + newConditionMock.address + ); + }); + + it('should set PermissionCondition', async () => { + await pm.grantWithCondition( + pm.address, + otherSigner.address, + ADMIN_PERMISSION_ID, + conditionMock.address + ); + expect( + await pm.getAuthPermission( + pm.address, + otherSigner.address, + ADMIN_PERMISSION_ID + ) + ).to.be.equal(conditionMock.address); + }); + + it('should not allow grant', async () => { + await expect( + pm + .connect(otherSigner) + .grantWithCondition( + pm.address, + otherSigner.address, + ADMIN_PERMISSION_ID, + conditionMock.address + ) + ) + .to.be.revertedWithCustomError(pm, 'Unauthorized') + .withArgs(pm.address, otherSigner.address, ADMIN_PERMISSION_ID); + }); + + it('reverts if the caller does not have `ROOT_PERMISSION_ID`', async () => { + await pm.grantWithCondition( + pm.address, + ownerSigner.address, + ADMIN_PERMISSION_ID, + conditionMock.address + ); + await expect( + pm + .connect(otherSigner) + .grantWithCondition( + pm.address, + otherSigner.address, + DAO_PERMISSIONS.ROOT_PERMISSION_ID, + conditionMock.address + ) + ) + .to.be.revertedWithCustomError(pm, 'Unauthorized') + .withArgs( + pm.address, + otherSigner.address, + DAO_PERMISSIONS.ROOT_PERMISSION_ID + ); + }); + + it('should revert if the permission is frozen', async () => { + await pm + .connect(ownerSigner) + .createPermission(someWhere, somePermissionId, ownerSigner.address, [ + ownerSigner.address, + ]); + + await pm + .connect(ownerSigner) + .addOwner(someWhere, somePermissionId, FREEZE_OWNER, GRANT_OWNER_FLAG); + + await pm + .connect(ownerSigner) + .removeOwner(someWhere, somePermissionId, FULL_OWNER_FLAG); + + await expect( + pm + .connect(ownerSigner) + .grantWithCondition( + someWhere, + otherSigner.address, + somePermissionId, + conditionMock.address + ) + ) + .to.be.revertedWithCustomError(pm, 'PermissionFrozen') + .withArgs(someWhere, somePermissionId); + }); + + it('should revert if the caller doesnt have the grantWithCondition flag set', async () => { + await pm + .connect(ownerSigner) + .createPermission(someWhere, somePermissionId, ownerSigner.address, [ + otherSigner.address, + ]); + + await expect( + pm + .connect(otherSigner) + .grantWithCondition( + someWhere, + otherSigner.address, + somePermissionId, + conditionMock.address + ) + ) + .to.be.revertedWithCustomError(pm, 'Unauthorized') + .withArgs(someWhere, otherSigner.address, somePermissionId); + }); + + it('should allow the root user as fallback if the permission is created but no grantWithCondition owners are existing anymore', async () => { + await pm + .connect(ownerSigner) + .createPermission(someWhere, somePermissionId, otherSigner.address, [ + ownerSigner.address, + ]); + + await pm + .connect(otherSigner) + .removeOwner(someWhere, somePermissionId, GRANT_OWNER_FLAG); + + await expect( + pm + .connect(ownerSigner) + .grantWithCondition( + someWhere, + otherSigner.address, + somePermissionId, + conditionMock.address + ) + ).to.emit(pm, 'Granted'); + }); + + it('should allow the root user as fallback if the permission isnt created', async () => { + await expect( + pm + .connect(ownerSigner) + .grantWithCondition( + someWhere, + otherSigner.address, + somePermissionId, + conditionMock.address + ) + ).to.emit(pm, 'Granted'); + }); + + it('should allow the delegatee to call grantWithCondition once', async () => { + await pm + .connect(ownerSigner) + .createPermission(someWhere, somePermissionId, ownerSigner.address, [ + ownerSigner.address, + ]); + + await pm + .connect(ownerSigner) + .delegatePermission( + someWhere, + somePermissionId, otherSigner.address, - DAO_PERMISSIONS.ROOT_PERMISSION_ID + GRANT_OWNER_FLAG ); - }); - it('reverts if the caller does not have `ROOT_PERMISSION_ID`', async () => { - await pm.grantWithCondition( - pm.address, - ownerSigner.address, - ADMIN_PERMISSION_ID, - conditionMock.address - ); await expect( pm .connect(otherSigner) .grantWithCondition( - pm.address, + someWhere, otherSigner.address, - DAO_PERMISSIONS.ROOT_PERMISSION_ID, + somePermissionId, + conditionMock.address + ) + ).to.emit(pm, 'Granted'); + + await expect( + pm + .connect(otherSigner) + .grantWithCondition( + someWhere, + otherSigner.address, + somePermissionId, conditionMock.address ) ) .to.be.revertedWithCustomError(pm, 'Unauthorized') - .withArgs( - pm.address, + .withArgs(someWhere, otherSigner.address, somePermissionId); + }); + + it('should revert if undelegate got called before grantWithCondition got called by the delegatee', async () => { + await pm + .connect(ownerSigner) + .createPermission(someWhere, somePermissionId, ownerSigner.address, [ otherSigner.address, - DAO_PERMISSIONS.ROOT_PERMISSION_ID + ]); + + await pm + .connect(ownerSigner) + .delegatePermission( + someWhere, + somePermissionId, + otherSigner.address, + REVOKE_OWNER_FLAG + ); + + await pm + .connect(ownerSigner) + .undelegatePermission( + someWhere, + somePermissionId, + otherSigner.address, + REVOKE_OWNER_FLAG ); + + await expect( + pm + .connect(otherSigner) + .grantWithCondition( + someWhere, + otherSigner.address, + somePermissionId, + conditionMock.address + ) + ) + .to.be.revertedWithCustomError(pm, 'Unauthorized') + .withArgs(someWhere, otherSigner.address, somePermissionId); }); }); @@ -388,11 +1537,7 @@ describe('Core: PermissionManager', function () { .revoke(pm.address, otherSigner.address, ADMIN_PERMISSION_ID) ) .to.be.revertedWithCustomError(pm, 'Unauthorized') - .withArgs( - pm.address, - otherSigner.address, - DAO_PERMISSIONS.ROOT_PERMISSION_ID - ); + .withArgs(pm.address, otherSigner.address, ADMIN_PERMISSION_ID); }); it('should not emit revoked if already revoked', async () => { @@ -410,11 +1555,7 @@ describe('Core: PermissionManager', function () { .revoke(pm.address, otherSigner.address, ADMIN_PERMISSION_ID) ) .to.be.revertedWithCustomError(pm, 'Unauthorized') - .withArgs( - pm.address, - otherSigner.address, - DAO_PERMISSIONS.ROOT_PERMISSION_ID - ); + .withArgs(pm.address, otherSigner.address, ADMIN_PERMISSION_ID); }); it('should not allow for non ROOT', async () => { @@ -423,6 +1564,226 @@ describe('Core: PermissionManager', function () { pm .connect(otherSigner) .revoke(pm.address, otherSigner.address, ADMIN_PERMISSION_ID) + ) + .to.be.revertedWithCustomError(pm, 'Unauthorized') + .withArgs(pm.address, otherSigner.address, ADMIN_PERMISSION_ID); + }); + + it('should revert if the permission is frozen', async () => { + await pm + .connect(ownerSigner) + .createPermission(someWhere, somePermissionId, ownerSigner.address, [ + ownerSigner.address, + ]); + + await pm + .connect(ownerSigner) + .addOwner(someWhere, somePermissionId, FREEZE_OWNER, GRANT_OWNER_FLAG); + + await pm + .connect(ownerSigner) + .removeOwner(someWhere, somePermissionId, FULL_OWNER_FLAG); + + await expect( + pm + .connect(ownerSigner) + .revoke(someWhere, otherSigner.address, somePermissionId) + ) + .to.be.revertedWithCustomError(pm, 'PermissionFrozen') + .withArgs(someWhere, somePermissionId); + }); + + it('should revert if the caller doesnt have the revoke flag set', async () => { + await pm + .connect(ownerSigner) + .createPermission(someWhere, somePermissionId, ownerSigner.address, [ + otherSigner.address, + ]); + + await expect( + pm + .connect(otherSigner) + .revoke(someWhere, otherSigner.address, somePermissionId) + ) + .to.be.revertedWithCustomError(pm, 'Unauthorized') + .withArgs(someWhere, otherSigner.address, somePermissionId); + }); + + it('should allow the root user as fallback if the permission is created but no revoke owners are existing anymore', async () => { + await pm + .connect(ownerSigner) + .createPermission(someWhere, somePermissionId, otherSigner.address, [ + otherSigner.address, + ]); + + await pm + .connect(otherSigner) + .removeOwner(someWhere, somePermissionId, REVOKE_OWNER_FLAG); + + await expect( + pm + .connect(ownerSigner) + .revoke(someWhere, otherSigner.address, somePermissionId) + ).to.emit(pm, 'Revoked'); + }); + + it('should allow the root user as fallback if the permission isnt created', async () => { + await pm + .connect(ownerSigner) + .grant(someWhere, otherSigner.address, somePermissionId); + + await expect( + pm + .connect(ownerSigner) + .revoke(someWhere, otherSigner.address, somePermissionId) + ).to.emit(pm, 'Revoked'); + }); + + it('should allow the delegatee to call revoke once', async () => { + await pm + .connect(ownerSigner) + .createPermission(someWhere, somePermissionId, ownerSigner.address, [ + otherSigner.address, + ]); + + await pm + .connect(ownerSigner) + .delegatePermission( + someWhere, + somePermissionId, + otherSigner.address, + REVOKE_OWNER_FLAG + ); + + await expect( + pm + .connect(otherSigner) + .revoke(someWhere, otherSigner.address, somePermissionId) + ).to.emit(pm, 'Revoked'); + + await expect( + pm + .connect(otherSigner) + .revoke(someWhere, otherSigner.address, somePermissionId) + ) + .to.be.revertedWithCustomError(pm, 'Unauthorized') + .withArgs(someWhere, otherSigner.address, somePermissionId); + }); + + it('should still keep the flag for delegation if a caller is an owner and delegate and calls revoke', async () => { + let owner = signers[4]; + + await pm.createPermission(someWhere, somePermissionId, owner.address, []); + + await pm + .connect(owner) + .delegatePermission( + someWhere, + somePermissionId, + owner.address, + REVOKE_OWNER_FLAG + ); + + await pm.connect(owner).revoke(someWhere, someWhere, somePermissionId); + + expect( + await pm.getPermissionFlags(someWhere, somePermissionId, owner.address) + ).to.deep.equal([FULL_OWNER_FLAG, REVOKE_OWNER_FLAG]); + }); + + it('should still keep `GRANT_OWNER_FLAG` for delegatee if only `REVOKE_OWNER_FLAG` is used/depleted', async () => { + let owner = signers[3]; + let alice = signers[4]; + + await pm.createPermission(someWhere, somePermissionId, owner.address, []); + + // Alice became a delegate of `GRANT_OWNER_FLAG` on this permission + await pm + .connect(owner) + .delegatePermission( + someWhere, + somePermissionId, + alice.address, + FULL_OWNER_FLAG + ); + + await pm.connect(alice).revoke(someWhere, someWhere, somePermissionId); + + let currentFlags = await pm.getPermissionFlags( + someWhere, + somePermissionId, + alice.address + ); + expect(currentFlags).to.deep.equal([0, GRANT_OWNER_FLAG]); + await expect( + pm.connect(alice).grant(someWhere, someWhere, somePermissionId) + ).to.not.be.reverted; + }); + + it('should revert if undelegate got called before revoke got called by the delegatee', async () => { + await pm + .connect(ownerSigner) + .createPermission(someWhere, somePermissionId, ownerSigner.address, [ + otherSigner.address, + ]); + + await pm + .connect(ownerSigner) + .delegatePermission( + someWhere, + somePermissionId, + otherSigner.address, + REVOKE_OWNER_FLAG + ); + + await pm + .connect(ownerSigner) + .undelegatePermission( + someWhere, + somePermissionId, + otherSigner.address, + REVOKE_OWNER_FLAG + ); + + await expect( + pm + .connect(otherSigner) + .revoke(someWhere, otherSigner.address, somePermissionId) + ) + .to.be.revertedWithCustomError(pm, 'Unauthorized') + .withArgs(someWhere, otherSigner.address, somePermissionId); + }); + }); + + describe('setApplyTargetMethodGrantee', () => { + beforeEach(async () => { + await pm.createPermission( + ownerSigner.address, + ADMIN_PERMISSION_ID, + otherSigner.address, + ['0xb794F5eA0ba39494cE839613fffBA74279579268'] + ); + }); + + it('should store the applyTargetMethodGrantee ', async () => { + expect(await pm.applyTargetMethodGrantee()).to.be.equal(addressZero); + await pm.setApplyTargetMethodGrantee(ownerSigner.address); + + expect(await pm.applyTargetMethodGrantee()).to.be.equal( + ownerSigner.address + ); + }); + + it('should emit ApplyTargetMethodGranteeSet', async () => { + await expect(pm.setApplyTargetMethodGrantee(ownerSigner.address)).to.emit( + pm, + 'ApplyTargetMethodGranteeSet' + ); + }); + + it('should revert if not allowed', async () => { + await expect( + pm.connect(otherSigner).setApplyTargetMethodGrantee(ownerSigner.address) ) .to.be.revertedWithCustomError(pm, 'Unauthorized') .withArgs( @@ -434,66 +1795,251 @@ describe('Core: PermissionManager', function () { }); describe('bulk on multiple target', () => { - it('should bulk grant ADMIN_PERMISSION on different targets', async () => { - const signers = await ethers.getSigners(); - await pm.grant(pm.address, signers[0].address, ADMIN_PERMISSION_ID); - const bulkItems: MultiTargetPermission[] = [ + let bulkItem: MultiTargetPermission; + + before(async () => { + bulkItem = { + operation: Operation.Grant, + where: someWhere, + who: someWhere, + condition: addressZero, + permissionId: ADMIN_PERMISSION_ID, + }; + }); + + it("throws `Unauthorized` error when caller does not have `APPLY_TARGET_PERMISSION` and isn't root", async () => { + let caller = signers[3]; + + await expect(pm.connect(caller).applyMultiTargetPermissions([bulkItem])) + .to.be.revertedWithCustomError(pm, 'Unauthorized') + .withArgs(pm.address, caller.address, APPLY_TARGET_PERMISSION_ID); + }); + + it('should bulk grant multiple permissions', async () => { + const items: MultiTargetPermission[] = [ { operation: Operation.Grant, where: signers[1].address, who: signers[2].address, condition: addressZero, - permissionId: ADMIN_PERMISSION_ID, + permissionId: ethers.utils.id('TEST_PERMISSION_1'), }, { operation: Operation.Grant, - where: signers[2].address, - who: signers[3].address, + where: signers[3].address, + who: signers[4].address, condition: addressZero, - permissionId: ADMIN_PERMISSION_ID, + permissionId: ethers.utils.id('TEST_PERMISSION_2'), }, ]; - await pm.applyMultiTargetPermissions(bulkItems); - for (const item of bulkItems) { - const permission = await pm.getAuthPermission( - item.where, - item.who, - item.permissionId + await pm.applyMultiTargetPermissions(items); + for (const item of items) { + const permission = await pm.getAuthPermission( + item.where, + item.who, + item.permissionId + ); + expect(permission).to.be.equal(ALLOW_FLAG); + } + }); + + describe('When permission does not have an owner', async () => { + it('should succeed if caller has ROOT', async () => { + let caller = signers[2]; + + await pm.grant( + pm.address, + caller.address, + DAO_PERMISSIONS.ROOT_PERMISSION_ID + ); + + await expect( + pm.connect(caller).applyMultiTargetPermissions([bulkItem]) + ).to.emit(pm, 'Granted'); + + const permission = await pm.getAuthPermission( + bulkItem.where, + bulkItem.who, + bulkItem.permissionId + ); + + expect(permission).to.be.equal(ALLOW_FLAG); + }); + + it('should succeed if caller has `APPLY_TARGET_PERMISSION`', async () => { + let caller = signers[2]; + + await pm.setApplyTargetMethodGrantee(caller.address); + await pm.grant(pm.address, caller.address, APPLY_TARGET_PERMISSION_ID); + + await expect( + pm.connect(caller).applyMultiTargetPermissions([bulkItem]) + ).to.emit(pm, 'Granted'); + + const permission = await pm.getAuthPermission( + bulkItem.where, + bulkItem.who, + bulkItem.permissionId + ); + + expect(permission).to.be.equal(ALLOW_FLAG); + }); + }); + + describe('When permission has an owner', async () => { + it('should revert if caller has `APPLY_TARGET_PERMISSION but is not an owner', async () => { + let owner = signers[3]; + let caller = signers[2]; + + await pm.createPermission( + someWhere, + ADMIN_PERMISSION_ID, + owner.address, + [] + ); + + await pm.setApplyTargetMethodGrantee(caller.address); + await pm.grant(pm.address, caller.address, APPLY_TARGET_PERMISSION_ID); + + await expect( + pm.connect(caller).applyMultiTargetPermissions([bulkItem]) + ).to.be.revertedWithCustomError(pm, 'Unauthorized'); + }); + + it('should revert if caller has `ROOT` but is not an owner', async () => { + let caller = signers[2]; + let owner = signers[3]; + + await pm.createPermission( + someWhere, + ADMIN_PERMISSION_ID, + owner.address, + [] + ); + + await pm.grant( + pm.address, + caller.address, + DAO_PERMISSIONS.ROOT_PERMISSION_ID + ); + + await expect( + pm.connect(caller).applyMultiTargetPermissions([bulkItem]) + ).to.be.revertedWithCustomError(pm, 'Unauthorized'); + }); + + it('should succeed if caller has `APPLY_TARGET_PERMISSION` and is an owner', async () => { + let caller = signers[3]; + + await pm.createPermission( + someWhere, + ADMIN_PERMISSION_ID, + caller.address, + [] + ); + + await pm.setApplyTargetMethodGrantee(caller.address); + await pm.grant(pm.address, caller.address, APPLY_TARGET_PERMISSION_ID); + + await expect( + pm.connect(caller).applyMultiTargetPermissions([bulkItem]) + ).to.emit(pm, 'Granted'); + + const permission = await pm.getAuthPermission( + bulkItem.where, + bulkItem.who, + bulkItem.permissionId + ); + + expect(permission).to.be.equal(ALLOW_FLAG); + }); + + it('should succeed if caller has `ROOT` and is an owner', async () => { + let caller = signers[3]; + + await pm.createPermission( + someWhere, + ADMIN_PERMISSION_ID, + caller.address, + [] + ); + + await pm.grant( + pm.address, + caller.address, + DAO_PERMISSIONS.ROOT_PERMISSION_ID + ); + + await expect( + pm.connect(caller).applyMultiTargetPermissions([bulkItem]) + ).to.emit(pm, 'Granted'); + + const permission = await pm.getAuthPermission( + bulkItem.where, + bulkItem.who, + bulkItem.permissionId + ); + + expect(permission).to.be.equal(ALLOW_FLAG); + }); + + it('should succeed if caller is delegated', async () => { + let caller = signers[3]; + let owner = signers[2]; + + await pm.createPermission( + someWhere, + ADMIN_PERMISSION_ID, + owner.address, + [] + ); + + await pm.grant( + pm.address, + caller.address, + DAO_PERMISSIONS.ROOT_PERMISSION_ID ); - expect(permission).to.be.equal(ALLOW_FLAG); - } + + await expect( + pm.connect(caller).applyMultiTargetPermissions([bulkItem]) + ).to.be.revertedWithCustomError(pm, 'Unauthorized'); + + await pm + .connect(owner) + .delegatePermission( + someWhere, + ADMIN_PERMISSION_ID, + caller.address, + GRANT_OWNER_FLAG + ); + + await expect( + pm.connect(caller).applyMultiTargetPermissions([bulkItem]) + ).to.emit(pm, 'Granted'); + }); }); it('should bulk revoke', async () => { - const signers = await ethers.getSigners(); - await pm.grant( - signers[1].address, - signers[0].address, - ADMIN_PERMISSION_ID - ); - await pm.grant( - signers[2].address, - signers[0].address, - ADMIN_PERMISSION_ID - ); const bulkItems: MultiTargetPermission[] = [ { operation: Operation.Revoke, - where: signers[1].address, - who: signers[0].address, + where: otherSigner.address, + who: ownerSigner.address, condition: addressZero, permissionId: ADMIN_PERMISSION_ID, }, { operation: Operation.Revoke, - where: signers[2].address, - who: signers[0].address, + where: otherSigner.address, + who: ownerSigner.address, condition: addressZero, permissionId: ADMIN_PERMISSION_ID, }, ]; + await pm.applyMultiTargetPermissions(bulkItems); + for (const item of bulkItems) { const permission = await pm.getAuthPermission( item.where, @@ -505,30 +2051,29 @@ describe('Core: PermissionManager', function () { }); it('should grant with condition', async () => { - const signers = await ethers.getSigners(); - const conditionMock2 = await new PermissionConditionMock__factory( signers[0] ).deploy(); - await pm.grant(pm.address, signers[0].address, ADMIN_PERMISSION_ID); const bulkItems: MultiTargetPermission[] = [ { operation: Operation.GrantWithCondition, - where: signers[1].address, - who: signers[0].address, - condition: conditionMock.address, - permissionId: ADMIN_PERMISSION_ID, + where: signers[3].address, + who: signers[3].address, + condition: conditionMock2.address, + permissionId: ethers.utils.id('TEST_PERMISSION_1'), }, { operation: Operation.GrantWithCondition, - where: signers[2].address, - who: signers[0].address, + where: signers[4].address, + who: signers[4].address, condition: conditionMock2.address, - permissionId: ADMIN_PERMISSION_ID, + permissionId: ethers.utils.id('TEST_PERMISSION_2'), }, ]; + await pm.applyMultiTargetPermissions(bulkItems); + for (const item of bulkItems) { const permission = await pm.getAuthPermission( item.where, @@ -539,54 +2084,82 @@ describe('Core: PermissionManager', function () { } }); - it('throws Unauthorized error when caller does not have ROOT_PERMISSION_ID permission', async () => { - const signers = await ethers.getSigners(); + it('should revert if at least one of the permission is frozen', async () => { + const permissionId1 = ethers.utils.id('TEST_PERMISSION_1'); + const permissionId2 = ethers.utils.id('TEST_PERMISSION_2'); + + const where = someWhere; + const bulkItems: MultiTargetPermission[] = [ { operation: Operation.Grant, - where: signers[1].address, - who: signers[0].address, + where: where, + who: otherSigner.address, condition: addressZero, - permissionId: ADMIN_PERMISSION_ID, + permissionId: permissionId1, + }, + { + operation: Operation.Grant, + where: where, + who: otherSigner.address, + condition: addressZero, + permissionId: permissionId2, }, ]; + // Let's freeze one of the permission. + await pm.createPermission(where, permissionId2, ownerSigner.address, []); + await pm.addOwner(where, permissionId2, FREEZE_OWNER, FULL_OWNER_FLAG); + await pm.removeOwner(where, permissionId2, FULL_OWNER_FLAG); + + // make sure that permission is really frozen. + expect(await pm.isPermissionFrozen(where, permissionId2)).to.be.true; + + await expect(pm.applyMultiTargetPermissions(bulkItems)) + .to.be.revertedWithCustomError(pm, 'PermissionFrozen') + .withArgs(where, permissionId2); + }); + }); + + describe('bulk on single target', () => { + let bulkItem: SingleTargetPermission; + + before(async () => { + bulkItem = { + operation: Operation.Grant, + who: someWhere, + permissionId: ADMIN_PERMISSION_ID, + }; + }); + + it('throws `Unauthorized` error when caller does not have `APPLY_TARGET_PERMISSION` and isnt root', async () => { + let caller = signers[3]; + await expect( - pm.connect(signers[2]).applyMultiTargetPermissions(bulkItems) + pm.connect(caller).applySingleTargetPermissions(someWhere, [bulkItem]) ) .to.be.revertedWithCustomError(pm, 'Unauthorized') - .withArgs( - pm.address, - signers[2].address, - DAO_PERMISSIONS.ROOT_PERMISSION_ID - ); + .withArgs(pm.address, caller.address, APPLY_TARGET_PERMISSION_ID); }); - }); - describe('bulk on single target', () => { - it('should bulk grant ADMIN_PERMISSION', async () => { - const signers = await ethers.getSigners(); - const bulkItems: SingleTargetPermission[] = [ + it('should bulk grant multiple permissions', async () => { + const items: MultiTargetPermission[] = [ { operation: Operation.Grant, who: signers[1].address, - permissionId: ADMIN_PERMISSION_ID, + permissionId: ethers.utils.id('TEST_PERMISSION_1'), }, { operation: Operation.Grant, who: signers[2].address, - permissionId: ADMIN_PERMISSION_ID, - }, - { - operation: Operation.Grant, - who: signers[3].address, - permissionId: ADMIN_PERMISSION_ID, + permissionId: ethers.utils.id('TEST_PERMISSION_2'), }, ]; - await pm.applySingleTargetPermissions(pm.address, bulkItems); - for (const item of bulkItems) { + + await pm.applySingleTargetPermissions(someWhere, items); + for (const item of items) { const permission = await pm.getAuthPermission( - pm.address, + someWhere, item.who, item.permissionId ); @@ -594,45 +2167,187 @@ describe('Core: PermissionManager', function () { } }); - it('should bulk revoke', async () => { - const signers = await ethers.getSigners(); - await pm.grant(pm.address, signers[1].address, ADMIN_PERMISSION_ID); - await pm.grant(pm.address, signers[2].address, ADMIN_PERMISSION_ID); - await pm.grant(pm.address, signers[3].address, ADMIN_PERMISSION_ID); - const bulkItems: SingleTargetPermission[] = [ - { - operation: Operation.Revoke, - who: signers[1].address, - permissionId: ADMIN_PERMISSION_ID, - }, - { - operation: Operation.Revoke, - who: signers[2].address, - permissionId: ADMIN_PERMISSION_ID, - }, - { - operation: Operation.Revoke, - who: signers[3].address, - permissionId: ADMIN_PERMISSION_ID, - }, - ]; - await pm.applySingleTargetPermissions(pm.address, bulkItems); - for (const item of bulkItems) { + describe('When permission does not have an owner', async () => { + it('should succeed if caller has ROOT', async () => { + let caller = signers[2]; + + await pm.grant( + pm.address, + caller.address, + DAO_PERMISSIONS.ROOT_PERMISSION_ID + ); + + await expect( + pm.connect(caller).applySingleTargetPermissions(someWhere, [bulkItem]) + ).to.emit(pm, 'Granted'); + + const permission = await pm.getAuthPermission( + someWhere, + bulkItem.who, + bulkItem.permissionId + ); + + expect(permission).to.be.equal(ALLOW_FLAG); + }); + + it('should succeed if caller has `APPLY_TARGET_PERMISSION`', async () => { + let caller = signers[2]; + + await pm.setApplyTargetMethodGrantee(caller.address); + await pm.grant(pm.address, caller.address, APPLY_TARGET_PERMISSION_ID); + + await expect( + pm.connect(caller).applySingleTargetPermissions(someWhere, [bulkItem]) + ).to.emit(pm, 'Granted'); + const permission = await pm.getAuthPermission( + someWhere, + bulkItem.who, + bulkItem.permissionId + ); + + expect(permission).to.be.equal(ALLOW_FLAG); + }); + }); + + describe('When permission has an owner', async () => { + it('should revert if caller has `APPLY_TARGET_PERMISSION but is not an owner', async () => { + let owner = signers[3]; + let caller = signers[2]; + + await pm.createPermission( + someWhere, + ADMIN_PERMISSION_ID, + owner.address, + [] + ); + + await pm.setApplyTargetMethodGrantee(caller.address); + await pm.grant(pm.address, caller.address, APPLY_TARGET_PERMISSION_ID); + + await expect( + pm.connect(caller).applySingleTargetPermissions(someWhere, [bulkItem]) + ).to.be.revertedWithCustomError(pm, 'Unauthorized'); + }); + + it('should revert if caller has `ROOT` but is not an owner', async () => { + let owner = signers[3]; + let caller = signers[2]; + + await pm.createPermission( + someWhere, + ADMIN_PERMISSION_ID, + owner.address, + [] + ); + + await pm.grant( pm.address, - item.who, - item.permissionId + caller.address, + DAO_PERMISSIONS.ROOT_PERMISSION_ID ); - expect(permission).to.be.equal(UNSET_FLAG); - } + + await expect( + pm.connect(caller).applySingleTargetPermissions(someWhere, [bulkItem]) + ).to.be.revertedWithCustomError(pm, 'Unauthorized'); + }); + + it('should succeed if caller has `APPLY_TARGET_PERMISSION` and is an owner', async () => { + let caller = signers[3]; + + await pm.createPermission( + someWhere, + ADMIN_PERMISSION_ID, + caller.address, + [] + ); + + await pm.setApplyTargetMethodGrantee(caller.address); + await pm.grant(pm.address, caller.address, APPLY_TARGET_PERMISSION_ID); + + await expect( + pm.connect(caller).applySingleTargetPermissions(someWhere, [bulkItem]) + ).to.emit(pm, 'Granted'); + + const permission = await pm.getAuthPermission( + someWhere, + bulkItem.who, + bulkItem.permissionId + ); + + expect(permission).to.be.equal(ALLOW_FLAG); + }); + + it('should succeed if caller has `ROOT` and is an owner', async () => { + let caller = signers[3]; + + await pm.createPermission( + someWhere, + ADMIN_PERMISSION_ID, + caller.address, + [] + ); + + await pm.grant( + pm.address, + caller.address, + DAO_PERMISSIONS.ROOT_PERMISSION_ID + ); + + await expect( + pm.connect(caller).applySingleTargetPermissions(someWhere, [bulkItem]) + ).to.emit(pm, 'Granted'); + + const permission = await pm.getAuthPermission( + someWhere, + bulkItem.who, + bulkItem.permissionId + ); + + expect(permission).to.be.equal(ALLOW_FLAG); + }); + + it('should succeed if caller is delegated', async () => { + let caller = signers[3]; + let owner = signers[2]; + + await pm.createPermission( + someWhere, + ADMIN_PERMISSION_ID, + owner.address, + [] + ); + + await pm.grant( + pm.address, + caller.address, + DAO_PERMISSIONS.ROOT_PERMISSION_ID + ); + + await expect( + pm.connect(caller).applySingleTargetPermissions(someWhere, [bulkItem]) + ).to.be.revertedWithCustomError(pm, 'Unauthorized'); + + await pm + .connect(owner) + .delegatePermission( + someWhere, + ADMIN_PERMISSION_ID, + caller.address, + GRANT_OWNER_FLAG + ); + + await expect( + pm.connect(caller).applySingleTargetPermissions(someWhere, [bulkItem]) + ).to.emit(pm, 'Granted'); + }); }); it('reverts for `Operation.GrantWithCondition` ', async () => { - const signers = await ethers.getSigners(); const bulkItems: SingleTargetPermission[] = [ { operation: Operation.GrantWithCondition, - who: signers[1].address, + who: ownerSigner.address, permissionId: ADMIN_PERMISSION_ID, }, ]; @@ -641,19 +2356,61 @@ describe('Core: PermissionManager', function () { ).to.be.revertedWithCustomError(pm, 'GrantWithConditionNotSupported'); }); + it('should revert if at least one of the permission is frozen', async () => { + const permissionId1 = ethers.utils.id('TEST_PERMISSION_1'); + const permissionId2 = ethers.utils.id('TEST_PERMISSION_2'); + + const where = someWhere; + + const bulkItems: MultiTargetPermission[] = [ + { + operation: Operation.Grant, + who: otherSigner.address, + condition: addressZero, + permissionId: permissionId1, + }, + { + operation: Operation.Grant, + who: otherSigner.address, + condition: addressZero, + permissionId: permissionId2, + }, + ]; + + // Let's freeze one of the permission. + await pm.createPermission(where, permissionId2, ownerSigner.address, []); + await pm.addOwner(where, permissionId2, FREEZE_OWNER, FULL_OWNER_FLAG); + await pm.removeOwner(where, permissionId2, FULL_OWNER_FLAG); + + // make sure that permission is really frozen. + expect(await pm.isPermissionFrozen(where, permissionId2)).to.be.true; + + await expect(pm.applySingleTargetPermissions(where, bulkItems)) + .to.be.revertedWithCustomError(pm, 'PermissionFrozen') + .withArgs(where, permissionId2); + }); + it('should handle bulk mixed', async () => { - const signers = await ethers.getSigners(); - await pm.grant(pm.address, signers[1].address, ADMIN_PERMISSION_ID); + await pm.createPermission( + pm.address, + ADMIN_PERMISSION_ID, + ownerSigner.address, + [otherSigner.address] + ); + + const permissionId1 = ethers.utils.id('TEST_PERMISSION_1'); + const permissionId2 = ethers.utils.id('TEST_PERMISSION_2'); + const bulkItems: SingleTargetPermission[] = [ { operation: Operation.Revoke, - who: signers[1].address, - permissionId: ADMIN_PERMISSION_ID, + who: otherSigner.address, + permissionId: permissionId1, }, { operation: Operation.Grant, - who: signers[2].address, - permissionId: ADMIN_PERMISSION_ID, + who: ownerSigner.address, + permissionId: permissionId2, }, ]; @@ -661,31 +2418,30 @@ describe('Core: PermissionManager', function () { expect( await pm.getAuthPermission( pm.address, - signers[1].address, - ADMIN_PERMISSION_ID + otherSigner.address, + permissionId1 ) ).to.be.equal(UNSET_FLAG); expect( await pm.getAuthPermission( pm.address, - signers[2].address, - ADMIN_PERMISSION_ID + ownerSigner.address, + permissionId2 ) ).to.be.equal(ALLOW_FLAG); }); it('should emit correct events on bulk', async () => { - const signers = await ethers.getSigners(); - await pm.grant(pm.address, signers[1].address, ADMIN_PERMISSION_ID); + await pm.grant(pm.address, ownerSigner.address, ADMIN_PERMISSION_ID); const bulkItems: SingleTargetPermission[] = [ { operation: Operation.Revoke, - who: signers[1].address, + who: ownerSigner.address, permissionId: ADMIN_PERMISSION_ID, }, { operation: Operation.Grant, - who: signers[2].address, + who: ownerSigner.address, permissionId: ADMIN_PERMISSION_ID, }, ]; @@ -696,67 +2452,24 @@ describe('Core: PermissionManager', function () { ADMIN_PERMISSION_ID, ownerSigner.address, pm.address, - signers[1].address + ownerSigner.address ) .to.emit(pm, 'Granted') .withArgs( ADMIN_PERMISSION_ID, ownerSigner.address, pm.address, - signers[2].address, + ownerSigner.address, ALLOW_FLAG ); expect( await pm.getAuthPermission( pm.address, - signers[2].address, + ownerSigner.address, ADMIN_PERMISSION_ID ) ).to.be.equal(ALLOW_FLAG); }); - - it('should not allow', async () => { - const bulkItems: SingleTargetPermission[] = [ - { - operation: Operation.Grant, - who: otherSigner.address, - permissionId: ADMIN_PERMISSION_ID, - }, - ]; - await expect( - pm - .connect(otherSigner) - .applySingleTargetPermissions(pm.address, bulkItems) - ) - .to.be.revertedWithCustomError(pm, 'Unauthorized') - .withArgs( - pm.address, - otherSigner.address, - DAO_PERMISSIONS.ROOT_PERMISSION_ID - ); - }); - - it('should not allow for non ROOT', async () => { - await pm.grant(pm.address, otherSigner.address, ADMIN_PERMISSION_ID); - const bulkItems: SingleTargetPermission[] = [ - { - operation: Operation.Grant, - who: otherSigner.address, - permissionId: ADMIN_PERMISSION_ID, - }, - ]; - await expect( - pm - .connect(otherSigner) - .applySingleTargetPermissions(pm.address, bulkItems) - ) - .to.be.revertedWithCustomError(pm, 'Unauthorized') - .withArgs( - pm.address, - otherSigner.address, - DAO_PERMISSIONS.ROOT_PERMISSION_ID - ); - }); }); describe('isGranted', () => { diff --git a/packages/contracts/test/framework/dao/dao-factory.ts b/packages/contracts/test/framework/dao/dao-factory.ts index 707e218ee..c23bf4095 100644 --- a/packages/contracts/test/framework/dao/dao-factory.ts +++ b/packages/contracts/test/framework/dao/dao-factory.ts @@ -45,8 +45,11 @@ import {PluginUUPSUpgradeableV2Mock__factory} from '@aragon/osx-ethers-v1.2.0'; import {anyValue} from '@nomicfoundation/hardhat-chai-matchers/withArgs'; import {SignerWithAddress} from '@nomiclabs/hardhat-ethers/signers'; import {expect} from 'chai'; +import {defaultAbiCoder} from 'ethers/lib/utils'; import {ethers} from 'hardhat'; +const APPLY_TARGET_PERMISSION_ID = ethers.utils.id('APPLY_TARGET_PERMISSION'); + const EVENTS = { PluginRepoRegistered: 'PluginRepoRegistered', DAORegistered: 'DAORegistered', @@ -59,6 +62,7 @@ const EVENTS = { NewURI: 'NewURI', Revoked: 'Revoked', Granted: 'Granted', + ApplyMethodGranteeSet: 'ApplyTargetMethodGranteeSet', }; const ALLOW_FLAG = '0x0000000000000000000000000000000000000002'; @@ -433,10 +437,14 @@ describe('DAOFactory: ', function () { const daoContract = factory.attach(dao); // Check that events were emitted. + await expect(tx) + .to.emit(daoContract, EVENTS.ApplyMethodGranteeSet) + .withArgs(psp.address); + await expect(tx) .to.emit(daoContract, EVENTS.Revoked) .withArgs( - DAO_PERMISSIONS.ROOT_PERMISSION_ID, + APPLY_TARGET_PERMISSION_ID, daoFactory.address, dao, psp.address @@ -478,6 +486,15 @@ describe('DAOFactory: ', function () { ) ).to.be.false; + expect( + await daoContract.hasPermission( + dao, + psp.address, + APPLY_TARGET_PERMISSION_ID, + '0x' + ) + ).to.be.false; + expect( await daoContract.hasPermission( psp.address, diff --git a/packages/contracts/test/framework/plugin/plugin-setup-processor.ts b/packages/contracts/test/framework/plugin/plugin-setup-processor.ts index c2c2c495e..c68fa1b23 100644 --- a/packages/contracts/test/framework/plugin/plugin-setup-processor.ts +++ b/packages/contracts/test/framework/plugin/plugin-setup-processor.ts @@ -109,6 +109,8 @@ const EMPTY_DATA = '0x'; const ADDRESS_TWO = `0x${'00'.repeat(19)}02`; +const APPLY_TARGET_PERMISSION_ID = ethers.utils.id('APPLY_TARGET_PERMISSION'); + describe('PluginSetupProcessor', function () { let signers: SignerWithAddress[]; let psp: PluginSetupProcessor; @@ -579,7 +581,7 @@ describe('PluginSetupProcessor', function () { .withArgs( targetDao.address, psp.address, - DAO_PERMISSIONS.ROOT_PERMISSION_ID + APPLY_TARGET_PERMISSION_ID ); }); @@ -1165,7 +1167,7 @@ describe('PluginSetupProcessor', function () { .withArgs( targetDao.address, psp.address, - DAO_PERMISSIONS.ROOT_PERMISSION_ID + APPLY_TARGET_PERMISSION_ID ); }); @@ -1827,7 +1829,7 @@ describe('PluginSetupProcessor', function () { .withArgs( targetDao.address, psp.address, - DAO_PERMISSIONS.ROOT_PERMISSION_ID + APPLY_TARGET_PERMISSION_ID ); }); diff --git a/packages/contracts/test/test-utils/proxy.ts b/packages/contracts/test/test-utils/proxy.ts index f7df90bab..4993d2c6f 100644 --- a/packages/contracts/test/test-utils/proxy.ts +++ b/packages/contracts/test/test-utils/proxy.ts @@ -23,7 +23,7 @@ export async function deployWithProxy( return upgrades.deployProxy(contractFactory, [], { kind: options.proxyType || 'uups', initializer: false, - unsafeAllow: ['constructor'], + unsafeAllow: ['constructor', 'delegatecall'], constructorArgs: options.constructurArgs || [], }) as unknown as Promise; } diff --git a/packages/contracts/test/test-utils/uups-upgradeable.ts b/packages/contracts/test/test-utils/uups-upgradeable.ts index b2bcd3da6..0cc171f0c 100644 --- a/packages/contracts/test/test-utils/uups-upgradeable.ts +++ b/packages/contracts/test/test-utils/uups-upgradeable.ts @@ -149,7 +149,7 @@ export async function deployAndUpgradeFromToCheck( } // Upgrade the proxy to a new implementation from a different factory - await upgrades.upgradeProxy(proxy.address, to.connect(upgrader), { + proxy = await upgrades.upgradeProxy(proxy.address, to.connect(upgrader), { unsafeAllow: ['constructor'], constructorArgs: [], });