feat: Protected SetRoleHolders Guard

src/guards/ProtectedSetRoleHoldersGuard.sol

@@ -0,0 +1,61 @@
+// SPDX-License-Identifier: MIT
+pragma solidity 0.8.19;
+
+import {ActionInfo, RoleHolderData} from "src/lib/Structs.sol";
+import {ILlamaActionGuard} from "src/interfaces/ILlamaActionGuard.sol";
+
+/// @title Protected Set Role Holder Guard
+/// @author Llama ([email protected])
+/// @notice A guard that protects against unauthorized calls to setRoleHolders on the LlamaGovernanceScript.
+contract ProtectedSetRoleHoldersGuard is ILlamaActionGuard {
+  /// @dev Throws if called by any account other than the EXECUTOR.
+  error OnlyLlamaExecutor();
+  /// @dev Throws if the setterRole is not authorized to set the targetRole.
+  error UnauthorizedSetRoleHolder(uint8 setterRole, uint8 targetRole);
+
+  /// @dev Emitted when the authorizedSetRoleHolder mapping is updated.
+  event AuthorizedSetRoleHolder(uint8 indexed setterRole, uint8 indexed targetRole, bool isAuthorized);
+
+  /// @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;
+
+  /// @notice A mapping to keep track of which roles the setterRole is authorized to set.
+  mapping(uint8 => mapping(uint8 => bool)) public authorizedSetRoleHolder;
+
+  constructor(uint8 _BYPASS_PROTECTION_ROLE, address _executor) {
+    BYPASS_PROTECTION_ROLE = _BYPASS_PROTECTION_ROLE;
+    EXECUTOR = _executor;
+  }
+
+  /// @inheritdoc ILlamaActionGuard
+  /// @dev 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[]));
+      for (uint256 i = 0; i < roleHolderData.length; 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 setterRole The role that is is being authorized or unauthorized to set the targetRole.
+  /// @param targetRole The role that the setterRole is being authorized or unauthorized to set.
+  /// @param isAuthorized Whether the setterRole is authorized to set the targetRole.
+  function setAuthorizedSetRoleHolder(uint8 setterRole, uint8 targetRole, bool isAuthorized) external {
+    if (msg.sender != EXECUTOR) revert OnlyLlamaExecutor();
+    authorizedSetRoleHolder[setterRole][targetRole] = isAuthorized;
+    emit AuthorizedSetRoleHolder(setterRole, targetRole, isAuthorized);
+  }
+
+  /// @inheritdoc ILlamaActionGuard
+  function validatePreActionExecution(ActionInfo calldata actionInfo) external pure {}
+
+  /// @inheritdoc ILlamaActionGuard
+  function validatePostActionExecution(ActionInfo calldata actionInfo) external pure {}
+}

test/guards/ProtectedSetRoleHoldersGuard.t.sol

@@ -0,0 +1,113 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.19;
+
+import {Test, console2} from "forge-std/Test.sol";
+
+import {ProtectedSetRoleHoldersGuard} from "src/guards/ProtectedSetRoleHoldersGuard.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";
+import {LlamaGovernanceScriptTest} from "test/llama-scripts/LlamaGovernanceScript.t.sol";
+
+contract ProtectedSetRoleHolderTest is LlamaGovernanceScriptTest {
+  event AuthorizedSetRoleHolder(uint8 indexed setterRole, uint8 indexed targetRole, bool isAuthorized);
+
+  ProtectedSetRoleHoldersGuard public guard;
+
+  function setUp() public override {
+    LlamaGovernanceScriptTest.setUp();
+    guard = new ProtectedSetRoleHoldersGuard(uint8(0), address(mpExecutor));
+    vm.prank(address(mpExecutor));
+    mpCore.setGuard(address(govScript), SET_ROLE_HOLDERS_SELECTOR, guard);
+  }
+}
+
+contract ValidateActionCreation is ProtectedSetRoleHolderTest {
+  function test_RevertIf_UnauthorizedSetRoleHolder(uint8 targetRole) public {
+    targetRole = uint8(bound(targetRole, 1, 8)); // number of existing roles excluding all holders role
+
+    RoleHolderData[] memory roleHolderData = new RoleHolderData[](1);
+    roleHolderData[0] = RoleHolderData({role: targetRole, policyholder: approverAdam, quantity: 1, expiration: 0});
+
+    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.
+    vm.expectRevert(
+      abi.encodeWithSelector(
+        ProtectedSetRoleHoldersGuard.UnauthorizedSetRoleHolder.selector, uint8(Roles.ActionCreator), targetRole
+      )
+    );
+    vm.prank(actionCreatorAaron);
+    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 = new ProtectedSetRoleHoldersGuard(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();
+    emit AuthorizedSetRoleHolder(uint8(Roles.ActionCreator), targetRole, true);
+    guard.setAuthorizedSetRoleHolder(uint8(Roles.ActionCreator), targetRole, true);
+
+    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_IfAuthorizationChangesBeforeExecution(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();
+    emit AuthorizedSetRoleHolder(uint8(Roles.ActionCreator), targetRole, true);
+    guard.setAuthorizedSetRoleHolder(uint8(Roles.ActionCreator), targetRole, true);
+
+    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);
+
+    // setting role authorization to false mid action
+    vm.prank(address(mpExecutor));
+    vm.expectEmit();
+    emit AuthorizedSetRoleHolder(uint8(Roles.ActionCreator), targetRole, false);
+    guard.setAuthorizedSetRoleHolder(uint8(Roles.ActionCreator), targetRole, false);
+
+    mpCore.executeAction(actionInfo);
+
+    assertEq(mpPolicy.hasRole(approverAdam, targetRole), true);
+  }
+}