From 14a2ab7ab54f44fab6b404c0d2c9993b8b8985c6 Mon Sep 17 00:00:00 2001 From: dd0sxx Date: Tue, 9 Jan 2024 18:26:45 +0100 Subject: [PATCH] made changes --- .../set-role-holders/SetRoleHoldersGuard.sol | 40 ++++++++------- .../SetRoleHoldersGuardFactory.sol | 18 ++++--- test/guards/SetRoleHoldersGuard.t.sol | 49 +++++++++---------- 3 files changed, 56 insertions(+), 51 deletions(-) diff --git a/src/guards/set-role-holders/SetRoleHoldersGuard.sol b/src/guards/set-role-holders/SetRoleHoldersGuard.sol index 88c0c39e3..65724f5e6 100644 --- a/src/guards/set-role-holders/SetRoleHoldersGuard.sol +++ b/src/guards/set-role-holders/SetRoleHoldersGuard.sol @@ -4,6 +4,7 @@ pragma solidity 0.8.19; import {ILlamaActionGuard} from "src/interfaces/ILlamaActionGuard.sol"; import {LlamaUtils} from "src/lib/LlamaUtils.sol"; import {ActionInfo, RoleHolderData} from "src/lib/Structs.sol"; +import {AuthorizeSetRoleHolderData} from "src/guards/set-role-holders/SetRoleHoldersGuardFactory.sol"; /// @title Set Role Holders Guard /// @author Llama (devsdosomething@llama.xyz) @@ -31,9 +32,6 @@ contract SetRoleHoldersGuard is ILlamaActionGuard { // ======== Storage Variables ======== // =================================== - /// @notice BYPASS_PROTECTION_ROLE can be set to 0 to disable this feature. - /// This also means the all holders role cannot be set as the BYPASS_PROTECTION_ROLE. - uint8 public immutable BYPASS_PROTECTION_ROLE; /// @notice The `LlamaExecutor` contract address that controls this guard contract. address public immutable EXECUTOR; @@ -44,9 +42,9 @@ contract SetRoleHoldersGuard is ILlamaActionGuard { // ======== Contract Creation ======== // =================================== - constructor(uint8 _BYPASS_PROTECTION_ROLE, address _executor) { - BYPASS_PROTECTION_ROLE = _BYPASS_PROTECTION_ROLE; + constructor(address _executor, AuthorizeSetRoleHolderData[] memory authorizeSetRoleHolderData) { EXECUTOR = _executor; + _setAuthorizedSetRoleHolder(authorizeSetRoleHolderData); } // ================================ @@ -56,25 +54,20 @@ contract SetRoleHoldersGuard is ILlamaActionGuard { /// @inheritdoc ILlamaActionGuard /// @notice Performs a validation check at action creation time that the action creator is authorized to set the role. function validateActionCreation(ActionInfo calldata actionInfo) external view { - if (BYPASS_PROTECTION_ROLE == 0 || actionInfo.creatorRole != BYPASS_PROTECTION_ROLE) { - RoleHolderData[] memory roleHolderData = abi.decode(actionInfo.data[4:], (RoleHolderData[])); // skip selector - uint256 length = roleHolderData.length; - for (uint256 i = 0; i < length; LlamaUtils.uncheckedIncrement(i)) { - if (!authorizedSetRoleHolder[actionInfo.creatorRole][roleHolderData[i].role]) { - revert UnauthorizedSetRoleHolder(actionInfo.creatorRole, roleHolderData[i].role); - } + RoleHolderData[] memory roleHolderData = abi.decode(actionInfo.data[4:], (RoleHolderData[])); // skip selector + uint256 length = roleHolderData.length; + for (uint256 i = 0; i < length; LlamaUtils.uncheckedIncrement(i)) { + if (!authorizedSetRoleHolder[actionInfo.creatorRole][roleHolderData[i].role]) { + revert UnauthorizedSetRoleHolder(actionInfo.creatorRole, roleHolderData[i].role); } } } /// @notice Allows the EXECUTOR to set the authorizedSetRoleHolder mapping. - /// @param actionCreatorRole The role that is is being authorized or unauthorized to set the targetRole. - /// @param targetRole The role that the actionCreatorRole is being authorized or unauthorized to set. - /// @param isAuthorized Whether the actionCreatorRole is authorized to set the targetRole. - function setAuthorizedSetRoleHolder(uint8 actionCreatorRole, uint8 targetRole, bool isAuthorized) external { + /// @param authorizeSetRoleHolderData The data to set the authorizedSetRoleHolder mapping. + function setAuthorizedSetRoleHolder(AuthorizeSetRoleHolderData[] memory authorizeSetRoleHolderData) external { if (msg.sender != EXECUTOR) revert OnlyLlamaExecutor(); - authorizedSetRoleHolder[actionCreatorRole][targetRole] = isAuthorized; - emit AuthorizedSetRoleHolder(actionCreatorRole, targetRole, isAuthorized); + _setAuthorizedSetRoleHolder(authorizeSetRoleHolderData); } /// @inheritdoc ILlamaActionGuard @@ -82,4 +75,15 @@ contract SetRoleHoldersGuard is ILlamaActionGuard { /// @inheritdoc ILlamaActionGuard function validatePostActionExecution(ActionInfo calldata actionInfo) external pure {} + + // ================================ + // ======== Internal Logic ======== + // ================================ + function _setAuthorizedSetRoleHolder(AuthorizeSetRoleHolderData[] memory data) internal { + uint256 length = data.length; + for (uint256 i = 0; i < length; LlamaUtils.uncheckedIncrement(i)) { + authorizedSetRoleHolder[data[i].actionCreatorRole][data[i].targetRole] = data[i].isAuthorized; + emit AuthorizedSetRoleHolder(data[i].actionCreatorRole, data[i].targetRole, data[i].isAuthorized); + } + } } diff --git a/src/guards/set-role-holders/SetRoleHoldersGuardFactory.sol b/src/guards/set-role-holders/SetRoleHoldersGuardFactory.sol index 8c3e400b9..abaf099f6 100644 --- a/src/guards/set-role-holders/SetRoleHoldersGuardFactory.sol +++ b/src/guards/set-role-holders/SetRoleHoldersGuardFactory.sol @@ -3,6 +3,12 @@ pragma solidity 0.8.19; import {SetRoleHoldersGuard} from "src/guards/set-role-holders/SetRoleHoldersGuard.sol"; +struct AuthorizeSetRoleHolderData { + uint8 actionCreatorRole; + uint8 targetRole; + bool isAuthorized; +} + /// @title Set Role Holders Guard Factory /// @author Llama (devsdosomething@llama.xyz) /// @notice A factory contract that deploys `SetRoleHoldersGuard` contracts. @@ -10,18 +16,16 @@ import {SetRoleHoldersGuard} from "src/guards/set-role-holders/SetRoleHoldersGua /// on the `setRoleHolders` function in the `LlamaGovernanceScript` contract. contract SetRoleHoldersGuardFactory { /// @notice Emitted when a new `SetRoleHoldersGuard` contract is deployed. - event SetRoleHoldersGuardCreated( - address indexed deployer, address indexed executor, address guard, uint8 bypassProtectionRole - ); + event SetRoleHoldersGuardCreated(address indexed deployer, address indexed executor, address guard); /// @notice Deploys a new `SetRoleHoldersGuard` contract. - /// @param bypassProtectionRole The role that can bypass the protection. /// @param executor The address of the executor contract. - function deploySetRoleHoldersGuard(uint8 bypassProtectionRole, address executor) + /// @param authorizeSetRoleHolderData The initial role authorizations. + function deploySetRoleHoldersGuard(address executor, AuthorizeSetRoleHolderData[] memory authorizeSetRoleHolderData) external returns (SetRoleHoldersGuard guard) { - guard = new SetRoleHoldersGuard(bypassProtectionRole, executor); - emit SetRoleHoldersGuardCreated(msg.sender, executor, address(guard), bypassProtectionRole); + guard = new SetRoleHoldersGuard(executor, authorizeSetRoleHolderData); + emit SetRoleHoldersGuardCreated(msg.sender, executor, address(guard)); } } diff --git a/test/guards/SetRoleHoldersGuard.t.sol b/test/guards/SetRoleHoldersGuard.t.sol index 41202aa5e..5c38ea84c 100644 --- a/test/guards/SetRoleHoldersGuard.t.sol +++ b/test/guards/SetRoleHoldersGuard.t.sol @@ -4,7 +4,10 @@ pragma solidity ^0.8.19; import {Test, console2} from "forge-std/Test.sol"; import {SetRoleHoldersGuard} from "src/guards/set-role-holders/SetRoleHoldersGuard.sol"; -import {SetRoleHoldersGuardFactory} from "src/guards/set-role-holders/SetRoleHoldersGuardFactory.sol"; +import { + SetRoleHoldersGuardFactory, + AuthorizeSetRoleHolderData +} from "src/guards/set-role-holders/SetRoleHoldersGuardFactory.sol"; import {ActionInfo, PermissionData, RoleHolderData} from "src/lib/Structs.sol"; import {LlamaGovernanceScript} from "src/llama-scripts/LlamaGovernanceScript.sol"; import {Roles, LlamaTestSetup} from "test/utils/LlamaTestSetup.sol"; @@ -19,7 +22,8 @@ contract SetRoleHolderTest is LlamaGovernanceScriptTest { function setUp() public override { LlamaGovernanceScriptTest.setUp(); setRoleHoldersGuardFactory = new SetRoleHoldersGuardFactory(); - guard = setRoleHoldersGuardFactory.deploySetRoleHoldersGuard(uint8(0), address(mpExecutor)); + guard = + setRoleHoldersGuardFactory.deploySetRoleHoldersGuard(address(mpExecutor), new AuthorizeSetRoleHolderData[](0)); vm.prank(address(mpExecutor)); mpCore.setGuard(address(govScript), SET_ROLE_HOLDERS_SELECTOR, guard); } @@ -34,7 +38,7 @@ contract ValidateActionCreation is SetRoleHolderTest { bytes memory data = abi.encodeWithSelector(SET_ROLE_HOLDERS_SELECTOR, roleHolderData); - // There is no bypass role, and we have not set any authorizations, so this should always revert. + // We have not set any authorizations, so this should always revert. vm.expectRevert( abi.encodeWithSelector( SetRoleHoldersGuard.UnauthorizedSetRoleHolder.selector, uint8(Roles.ActionCreator), targetRole @@ -44,34 +48,20 @@ contract ValidateActionCreation is SetRoleHolderTest { mpCore.createAction(uint8(Roles.ActionCreator), mpStrategy2, address(govScript), 0, data, ""); } - function test_BypassProtectionRoleWorksWithAllExisingRoles(uint8 targetRole) public { - targetRole = uint8(bound(targetRole, 1, 8)); // number of existing roles excluding all holders role - - // create a new guard with a bypass role - guard = setRoleHoldersGuardFactory.deploySetRoleHoldersGuard(uint8(Roles.ActionCreator), address(mpExecutor)); - vm.prank(address(mpExecutor)); - mpCore.setGuard(address(govScript), SET_ROLE_HOLDERS_SELECTOR, guard); - - RoleHolderData[] memory roleHolderData = new RoleHolderData[](1); - roleHolderData[0] = - RoleHolderData({role: targetRole, policyholder: approverAdam, quantity: 1, expiration: type(uint64).max}); - - bytes memory data = abi.encodeWithSelector(SET_ROLE_HOLDERS_SELECTOR, roleHolderData); - - ActionInfo memory actionInfo = _createAndApproveAndQueueAction(data); - mpCore.executeAction(actionInfo); - - assertEq(mpPolicy.hasRole(approverAdam, targetRole), true); - } - function test_AuthorizedSetRoleHolder(uint8 targetRole) public { targetRole = uint8(bound(targetRole, 1, 8)); // number of existing roles excluding all holders role // set role authorization vm.prank(address(mpExecutor)); vm.expectEmit(); + AuthorizeSetRoleHolderData[] memory authorizeSetRoleHolderData = new AuthorizeSetRoleHolderData[](1); + authorizeSetRoleHolderData[0] = AuthorizeSetRoleHolderData({ + actionCreatorRole: uint8(Roles.ActionCreator), + targetRole: targetRole, + isAuthorized: true + }); emit AuthorizedSetRoleHolder(uint8(Roles.ActionCreator), targetRole, true); - guard.setAuthorizedSetRoleHolder(uint8(Roles.ActionCreator), targetRole, true); + guard.setAuthorizedSetRoleHolder(authorizeSetRoleHolderData); RoleHolderData[] memory roleHolderData = new RoleHolderData[](1); roleHolderData[0] = @@ -92,8 +82,14 @@ contract ValidateActionCreation is SetRoleHolderTest { // set role authorization vm.prank(address(mpExecutor)); vm.expectEmit(); + AuthorizeSetRoleHolderData[] memory authorizeSetRoleHolderData = new AuthorizeSetRoleHolderData[](1); + authorizeSetRoleHolderData[0] = AuthorizeSetRoleHolderData({ + actionCreatorRole: uint8(Roles.ActionCreator), + targetRole: targetRole, + isAuthorized: true + }); emit AuthorizedSetRoleHolder(uint8(Roles.ActionCreator), targetRole, true); - guard.setAuthorizedSetRoleHolder(uint8(Roles.ActionCreator), targetRole, true); + guard.setAuthorizedSetRoleHolder(authorizeSetRoleHolderData); RoleHolderData[] memory roleHolderData = new RoleHolderData[](1); roleHolderData[0] = @@ -106,8 +102,9 @@ contract ValidateActionCreation is SetRoleHolderTest { // setting role authorization to false mid action vm.prank(address(mpExecutor)); vm.expectEmit(); + authorizeSetRoleHolderData[0].isAuthorized = false; emit AuthorizedSetRoleHolder(uint8(Roles.ActionCreator), targetRole, false); - guard.setAuthorizedSetRoleHolder(uint8(Roles.ActionCreator), targetRole, false); + guard.setAuthorizedSetRoleHolder(authorizeSetRoleHolderData); mpCore.executeAction(actionInfo);