feat: Protected SetRoleHolders Guard #507

dd0sxx · 2024-01-05T15:43:05Z

Motivation:

To enable a common usecase with Llama where some roles are allowed to call setRoleHolders, but should only be able to set specific roles. The ProtectedSetRoleHoldersGuard provides a check on action creation that the action creator has authorization to set the role they are trying to set.

Modifications:

created a ProtectedSetRoleHoldersGuard contract that inherits the ILlamaActionGuard interface
Added a new dir src/guards
Wrote a test suite for ProtectedSetRoleHoldersGuard in a new dir tests/guards/

Result:

Llama users can unlock new usecases by using the protected setRoleHolders

github-actions · 2024-01-05T16:20:34Z

Coverage after merging theo/protectedSetRoleHolder into main will be

88.62%

Coverage Report

File	Stmts	Branches	Funcs	Lines	Uncovered Lines
src
LlamaCore.sol	99.65%	98.72%	100%	100%	299
LlamaExecutor.sol	80%	50%	100%	100%	33
LlamaFactory.sol	100%	100%	100%	100%
LlamaLens.sol	72.09%	20%	100%	82.61%	174–175, 178, 178, 178–179, 179, 179–180, 180, 180, 188
LlamaPolicy.sol	91.53%	88.89%	94.59%	91.38%	353, 392, 392, 392–394, 394, 394, 396–399, 401, 419
LlamaPolicyMetadata.sol	100%	100%	100%	100%
src/accounts
LlamaAccount.sol	100%	100%	100%	100%
LlamaAccountWithDelegation.sol	100%	100%	100%	100%
src/guards
ProtectedSetRoleHoldersGuard.sol	83.33%	50%	100%	100%	36, 39, 51
src/lib
ERC721NonTransferableMinimalProxy.sol	70.42%	72.73%	72.73%	68.42%	102, 104, 106, 118–120, 190, 88, 88, 88, 90, 90, 90, 92, 92, 92, 97, 99
LlamaUtils.sol	100%	100%	100%	100%
PolicyholderCheckpoints.sol	55.88%	50%	81.82%	53.62%	130, 184–186, 186, 186–187, 189, 192, 222, 229, 229–231, 233, 233–235, 237, 237–239, 241, 241–243, 262, 265–271, 278, 46, 46–48, 48, 48–49, 51
SupplyCheckpoints.sol	57.14%	50%	83.33%	54.93%	131, 183–185, 185, 185–186, 188, 191, 235, 242, 242–244, 246, 246–248, 250, 250–252, 254, 254–256, 275, 278–284, 291, 50, 50–52, 52, 52–53, 55
src/llama-scripts
LlamaAccountTokenDelegationScript.sol	100%	100%	100%	100%
LlamaGovernanceScript.sol	96.24%	64.29%	100%	100%	131, 133, 160, 171, 228
LlamaInstanceConfigBase.sol	100%	100%	100%	100%
LlamaInstanceConfigScriptTemplate.sol	100%	100%	100%	100%
src/strategies/absolute
LlamaAbsolutePeerReview.sol	100%	100%	100%	100%
LlamaAbsoluteQuorum.sol	100%	100%	100%	100%
LlamaAbsoluteStrategyBase.sol	94.81%	87.50%	90.91%	98%	248, 251, 301
src/strategies/relative
LlamaRelativeHolderQuorum.sol	91.67%	75%	100%	100%	52, 59
LlamaRelativeQuantityQuorum.sol	0%	0%	0%	0%	28, 28, 28–30, 42, 42, 42–44, 51–52, 52, 52–53, 58–59, 59, 59–60
LlamaRelativeStrategyBase.sol	97.62%	90.91%	100%	100%	208, 310
LlamaRelativeUniqueHolderQuorum.sol	93.33%	83.33%	100%	100%	54, 61

0xrajath · 2024-01-05T21:37:00Z

src/guards/ProtectedSetRoleHoldersGuard.sol

+  /// @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) {


If BYPASS_PROTECTION_ROLE is 0 it'll always enter this if scope right? How is that bypassing it?

it's clear that this is not true and when it's equal to 0 the bypass role is disabled

Wait why is that? If BYPASS_PROTECTION_ROLE == 0, your if condition is satisfied, so you always enter the if scope. But the idea is to bypass it right?

so shouldn't the condition be if (BYPASS_PROTECTION_ROLE != 0 && actionInfo.creatorRole != BYPASS_PROTECTION_ROLE) ?

0xrajath · 2024-01-05T21:40:44Z

src/guards/ProtectedSetRoleHoldersGuard.sol

+  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++) {


Two things to save gas (since we're on v0.8.19):

Define length outside the loop

Use LlamaUtils.uncheckedIncrement(i) for the index increment.

0xrajath · 2024-01-05T21:43:31Z

src/guards/ProtectedSetRoleHoldersGuard.sol

+
+  /// @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;


What's the idea behind a Bypass Protection Role? To allow some role to bypass the action creation check? Why just 1 role in that case (and why not multiple achievable through another mapping)?

I'd assume there would be cases where multiple roles will want to bypass the check.

Yeah I don't really see the point of this either. If the concern setRoleHolders gets bricked then the guard can be disabled. setRoleHolder can also be used if needed. I think it would simplify things not to have it.

the idea is to provide a better UX. if there is a core team role that can set every role, they shouldn't need to iteratively update the permissions mapping and update it every time a new role is created.

It makes me think the whole design of this is wrong then. We should either make one of the following two assumptions:

If this guard is enabled, all roles should default to not having any setRoleHolders access and any access to this function must be updated here.

If this guard is enabled, all roles should default to having normal setRoleHolders access and any restrictions to this function must be updated here.

This conversations makes me think we should go with 2. Having a dedicated bypass role is an extension to the Llama role system that just increases complexity.

Think about explaining this process to a smart crypto-native friend:

LlamaCore::executeAction runs the guard -> LlamaExecutor::execute -> LlamaGovernanceScript::setRoleHolders -> LlamaPolicy::setRoleHolder

It's already complicated enough.

I like the idea of 2.

0xrajath · 2024-01-05T21:45:13Z

src/guards/ProtectedSetRoleHoldersGuard.sol

+  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.


Some about this screams icky to me. If the ALL HOLDERS ROLE ever held permission to call setRoleHolders on the Gov Script, it would always bypass the check? (Far fetched I know, but we should account for all possibilities.).

no, that is not true at all. if you read the code, or the comment, it's clear that it would never bypass the check and setting BYPASS_PROTECTION_ROLE to 0 (default value) disables the bypass role entirely.

0xrajath · 2024-01-05T21:58:48Z

src/guards/ProtectedSetRoleHoldersGuard.sol

+  /// @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[]));


Took me a second to figure out what's going on here. Might be worth it to leave a comment saying that you're slicing the bytes array to get the function calldata from the data field.

Yeah agreed

0xrajath · 2024-01-05T22:04:25Z

src/guards/ProtectedSetRoleHoldersGuard.sol

+  error UnauthorizedSetRoleHolder(uint8 setterRole, uint8 targetRole);
+
+  /// @dev Emitted when the authorizedSetRoleHolder mapping is updated.
+  event AuthorizedSetRoleHolder(uint8 indexed setterRole, uint8 indexed targetRole, bool isAuthorized);


Could call it actionCreatorRole rather than setterRole which seems to be a bit more accurate.

0xrajath · 2024-01-05T22:04:36Z

src/guards/ProtectedSetRoleHoldersGuard.sol

+  /// @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 {


Could call it actionCreatorRole rather than setterRole which seems to be a bit more accurate.

0xrajath · 2024-01-05T22:05:20Z

src/guards/ProtectedSetRoleHoldersGuard.sol

+  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;


Can we do named mappings here?

0xrajath · 2024-01-05T22:06:08Z

src/guards/ProtectedSetRoleHoldersGuard.sol

+  /// @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);


Could call it actionCreatorRole rather than setterRole which seems to be a bit more accurate.

AustinGreen · 2024-01-05T23:40:56Z

src/guards/ProtectedSetRoleHoldersGuard.sol

+
+/// @title Protected Set Role Holder Guard
+/// @author Llama (devsdosomething@llama.xyz)
+/// @notice A guard that protects against unauthorized calls to setRoleHolders on the LlamaGovernanceScript.


We should be more descriptive here. Unauthorized callers can never call setRoleHolders. It's more about defining which roles can add policyholders to other roles

AustinGreen · 2024-01-05T23:41:54Z

src/guards/ProtectedSetRoleHoldersGuard.sol

+import {ActionInfo, RoleHolderData} from "src/lib/Structs.sol";
+import {ILlamaActionGuard} from "src/interfaces/ILlamaActionGuard.sol";


The order here should be flipped.

AustinGreen · 2024-01-05T23:44:05Z

src/guards/ProtectedSetRoleHoldersGuard.sol

+  /// @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.


Thrown not Throws

And setterRole and targetRole should be in backticks

src/guards/ProtectedSetRoleHoldersGuard.sol

AustinGreen · 2024-01-05T23:51:05Z

src/guards/ProtectedSetRoleHoldersGuard.sol

+  /// @inheritdoc ILlamaActionGuard
+  /// @dev Performs a validation check at action creation time that the action creator is authorized to set the role.


We shouldn't inheritdoc here right? We should just write proper natspec for these params and what the function does

I don't see why not to inherit doc, or why this isn't "proper". The inherit doc already describes the params. I changed the additional comment to @notice instead of dev which defines what the function is doing.

src/guards/ProtectedSetRoleHoldersGuard.sol

AustinGreen · 2024-01-08T18:25:57Z

src/guards/ProtectedSetRoleHoldersGuardFactory.sol

+
+/// @title Protected Set Role Holder Guard Factory
+/// @author Llama (devsdosomething@llama.xyz)
+/// @notice A factory contract that deploys `ProtectedSetRoleHoldersGuard` contracts.


Let's put some more thought into what helpful documentation what would look like here.

Just be reading this, a Llama instance policyholder should know the purpose of this guard.

AustinGreen · 2024-01-08T18:26:57Z

test/guards/ProtectedSetRoleHoldersGuard.t.sol

I think SetRoleHolderGuard is a suitable name. It's a lot shorter and the Protected adjective doesn't do much. The purpose of every guard is to protect something.

Should be SetRoleHoldersGuard IMO. Since this is protecting setRoleHolders and not setRoleHolder.

AustinGreen · 2024-01-08T18:27:20Z

src/guards/ProtectedSetRoleHoldersGuard.sol

https://github.com/llamaxyz/llama/pull/507/files#r1445116039

I don't know why but this link doesn't seem to take me to anything useful. can you paste what you'd like me to see here or send a screenshot

AustinGreen · 2024-01-08T18:27:55Z

src/guards/ProtectedSetRoleHoldersGuard.sol

+
+  /// @dev Thrown if called by any account other than the EXECUTOR.
+  error OnlyLlamaExecutor();
+  /// @dev Throw if the `actionCreatorRole` is not authorized to set the `targetRole`.


Should be an empty line here

AustinGreen · 2024-01-08T18:28:11Z

src/guards/ProtectedSetRoleHoldersGuard.sol

+
+  /// @dev Thrown if called by any account other than the EXECUTOR.
+  error OnlyLlamaExecutor();
+  /// @dev Throw if the `actionCreatorRole` is not authorized to set the `targetRole`.


Should be Thrown not Throw

0xrajath · 2024-01-08T18:53:52Z

src/guards/ProtectedSetRoleHoldersGuardFactory.sol

Small nit. But if we have a factory and a guard. We should move both files into a set-role-holders/ directory withing src/guards. We can then just keep adding directories for other guards we might build in the future.

0xrajath · 2024-01-08T18:59:47Z

src/guards/ProtectedSetRoleHoldersGuardFactory.sol

+  event ProtectedSetRoleHoldersGuardDeployed(
+    address indexed guard, uint8 indexed bypassProtectionRole, address indexed executor
+  );


I would make this event:

event ProtectedSetRoleHoldersGuardCreated(address indexed deployer, address indexed executor, address guard, uint8 bypassProtectionRole);

For factory deploys always have the deployer/caller of the deploy as the first indexed event parameter.

The only things you ever need to do indexed on is things that data consumers would want to filter by. In this case the deployer and executor. The guard is already unique so there's no need to index it. And There's no point filtering on bypassProtectionRole since it could be different roles for different instances.

We seem to be following *Created instead of *Deployed for all our factory events. Let's stick to the same.

AustinGreen · 2024-01-09T06:51:13Z

src/guards/set-role-holders/SetRoleHoldersGuard.sol

+import {LlamaUtils} from "src/lib/LlamaUtils.sol";
+import {ActionInfo, RoleHolderData} from "src/lib/Structs.sol";
+
+/// @title Protected Set Role Holder Guard


Should this be "Set Role Holders Guard" to reflect the contract name?

dd0sxx added 2 commits January 5, 2024 14:29

test_RevertIf_UnauthorizedSetRoleHolder

f77f497

tests

47bbde2

dd0sxx requested review from AustinGreen and 0xrajath as code owners January 5, 2024 15:43

dd0sxx added 2 commits January 5, 2024 17:06

scopelint

dd088d5

typo

d51f8d5

0xrajath reviewed Jan 5, 2024

View reviewed changes

AustinGreen reviewed Jan 5, 2024

View reviewed changes

src/guards/ProtectedSetRoleHoldersGuard.sol Outdated Show resolved Hide resolved

AustinGreen reviewed Jan 5, 2024

View reviewed changes

src/guards/ProtectedSetRoleHoldersGuard.sol Outdated Show resolved Hide resolved

dd0sxx added 3 commits January 6, 2024 14:34

res to comments

b5d9aeb

factory

dba00f8

fmt

94b4d2f

AustinGreen reviewed Jan 8, 2024

View reviewed changes

0xrajath reviewed Jan 8, 2024

View reviewed changes

dd0sxx added 12 commits January 8, 2024 21:22

changed to SetRoleHoldersGuard

9ffb328

some fixes

31c20bf

nit

b45d967

event

ca8f267

fmt

58a9c53

compiler fixes

03de99d

more docs

aea175a

pinning foundry version

6007a92

pinning

a51121d

pin

1bab540

trying the previous version

0e1a8c1

pin

cd3f332

AustinGreen reviewed Jan 9, 2024

View reviewed changes

dd0sxx added 2 commits January 9, 2024 15:47

comments

c0a7d0f

made changes

14a2ab7

AustinGreen closed this Jan 11, 2024

		import {ActionInfo, RoleHolderData} from "src/lib/Structs.sol";
		import {ILlamaActionGuard} from "src/interfaces/ILlamaActionGuard.sol";

		/// @inheritdoc ILlamaActionGuard
		/// @dev Performs a validation check at action creation time that the action creator is authorized to set the role.

feat: Protected SetRoleHolders Guard #507

feat: Protected SetRoleHolders Guard #507

Conversation

dd0sxx commented Jan 5, 2024 • edited Loading

github-actions bot commented Jan 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xrajath Jan 8, 2024 • edited Loading

Choose a reason for hiding this comment

0xrajath Jan 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dd0sxx commented Jan 5, 2024 •

edited

Loading

0xrajath Jan 8, 2024 •

edited

Loading

0xrajath Jan 8, 2024 •

edited

Loading