Check for role expiration is not implemented during action creation/approval/disapproval.

[code423n4]

Lines of code

https://github.com/code-423n4/2023-06-llama/blob/main/src/LlamaCore.sol#L516 https://github.com/code-423n4/2023-06-llama/blob/main/src/LlamaCore.sol#L565 https://github.com/code-423n4/2023-06-llama/blob/main/src/LlamaCore.sol#L576

Vulnerability details

Impact

The functions _createAction, _castApproval, and castDisapproval doesn't check if the policyHolder roles is expired using isRoleExpired.

The system already has a function to remove expired roles at LlamaPolicy::revokeExpiredRole but this function must be called manually and by someone who monitor the role. This design introduce 2 impacts:

1. User can take advantage of his permission until someone revoke his role manually.
2. User can front-run the TX to revokeExpiredRole and use his privilege permission even it's expired.

Proof of Concept

1. Implementation of _createAction, _castApproval, and castDisapproval doesn't check if the role is expired:

https://github.com/code-423n4/2023-06-llama/blob/main/src/LlamaCore.sol#L516

function _createAction(
    address policyholder,
    uint8 role,
    ILlamaStrategy strategy,
    address target,
    uint256 value,
    bytes calldata data,
    string memory description
  ) internal returns (uint256 actionId) {
    if (target == address(executor)) revert CannotSetExecutorAsTarget();
    if (!strategies[strategy]) revert InvalidStrategy();

https://github.com/code-423n4/2023-06-llama/blob/main/src/LlamaCore.sol#L565

function _castApproval(address policyholder, uint8 role, ActionInfo calldata actionInfo, string memory reason)
    internal
  {
    (Action storage action, uint128 quantity) = _preCastAssertions(actionInfo, policyholder, role, ActionState.Active);

    action.totalApprovals = _newCastCount(action.totalApprovals, quantity);
    approvals[actionInfo.id][policyholder] = true;
    emit ApprovalCast(actionInfo.id, policyholder, role, quantity, reason);
  }

https://github.com/code-423n4/2023-06-llama/blob/main/src/LlamaCore.sol#L576

function _castDisapproval(address policyholder, uint8 role, ActionInfo calldata actionInfo, string memory reason)
    internal
  {
    (Action storage action, uint128 quantity) = _preCastAssertions(actionInfo, policyholder, role, ActionState.Queued);

    action.totalDisapprovals = _newCastCount(action.totalDisapprovals, quantity);
    disapprovals[actionInfo.id][policyholder] = true;
    emit DisapprovalCast(actionInfo.id, policyholder, role, quantity, reason);
  }

2. We can see below is the implementation of revokeExpiredRole which should be called manually. https://github.com/code-423n4/2023-06-llama/blob/main/src/LlamaPolicy.sol#L217

   function _revokeExpiredRole(uint8 role, address policyholder) internal {
   // Read the most recent checkpoint for the policyholder's role balance.
   if (!isRoleExpired(policyholder, role)) revert InvalidRoleHolderInput();
   _setRoleHolder(role, policyholder, 0, 0);
   }

Tools Used

Manual

Recommended Mitigation Steps

The functions mentioned above should add the check below:

if (isRoleExpired(policyholder, role)) revert InvalidRoleHolderInput();

You can also call revokeExpiredRole to revoke the role without reverting and use return to not make any further changes.

Assessed type

Context

[c4-pre-sort]

0xSorryNotSorry marked the issue as primary issue

[c4-sponsor]

AustinGreen marked the issue as sponsor disputed

[AustinGreen]

Roles must be explicitly revoked or else the expiration hasn't taken effect. This is how the system is intended to be used.

[c4-judge]

gzeon-c4 marked the issue as unsatisfactory: Invalid

[Co0nan]

Hi @gzeon-c4,

I want to know your thoughts about this one from a security perspective. I don't agree with the sponsor's point here.

Some issues could be considered an improvement design but this design simply introduce different attack vectors.

The system is designed to be dynamic as possible, when instance owners set new policyHolders with different users they set the expiration role and they except that it will expire at that time, but due to the design implemented the owner may not revoke the policyHolder roles. Consider the following scenario:

1. Llama instance wants to give some users a role in order to participate in voting for action.
2. They call setRoleHolder and are set to be expired at the time the action gets executed.
3. Based on this they expect the user's role will be valid only for this action period.

However, due to the design here, the users will keep their permission until it is noticed by the llama. This is clearly not an intended behavior or even excepted by the users.

[gzeon-c4]

clearly documented design https://github.com/code-423n4/2023-06-llama/blob/9d422c264b57657098c2784aa951852cad32e01c/src/LlamaPolicy.sol#L214-L216

[Co0nan]

I know it's clearly design, my point is that this is not a secure design. Have you thought about front-run attacks that will be introduced based on this design? Does the scenario I have mentioned above is applicable?

With total respect, I feel like you are not adding more context from security PoV about why my attack vectors are not valid. either for this one or other issues. This will help me improve my self when I get feedback about what I miss. Thanks!

[gzeon-c4]

I know it's clearly design, my point is that this is not a secure design. Have you thought about front-run attacks that will be introduced based on this design? Does the scenario I have mentioned above is applicable?

With total respect, I feel like you are not adding more context from security PoV about why my attack vectors are not valid. either for this one or other issues. This will help me improve my self when I get feedback about what I miss. Thanks!

Just like a timelock, the role revocation is not automatic after the delay, but can be executed at anytime after the delay. If the user failed to run an offchain monitoring script to execute the revocation it would be considered as an user error and is out-of-scope. The protocol defines the expiry like a timelock delay, not a hard expiration, as also documented here: https://github.com/code-423n4/2023-06-llama/blob/9d422c264b57657098c2784aa951852cad32e01c/src/LlamaPolicy.sol#L436-L438

The policy contract has an invariant that even when a role is expired, i.e. block.timestamp > expiration, that role is still active until explicitly revoked with revokeExpiredRole.

There are certainly trade-off with every design, but it's not a security issue just because one misunderstood a well documented behavior.