Llama - auditor0517's results

Lines of code

https://github.com/code-423n4/2023-06-llama/blob/9d641b32e3f4092cc81dbac7b1c451c695e78983/src/strategies/LlamaRelativeQuorum.sol#L223 https://github.com/code-423n4/2023-06-llama/blob/9d641b32e3f4092cc81dbac7b1c451c695e78983/src/strategies/LlamaRelativeQuorum.sol#L242

Vulnerability details

Impact

In LlamaRelativeQuorum, the governance result might be incorrect as it counts the wrong approval/disapproval.

Proof of Concept

The LlamaRelativeQuorum uses approval/disapproval thresholds that are specified as percentages of total supply and the approval/disapproval supplies are set at validateActionCreation() during the action creation.

  function validateActionCreation(ActionInfo calldata actionInfo) external {
    LlamaPolicy llamaPolicy = policy; // Reduce SLOADs.
    uint256 approvalPolicySupply = llamaPolicy.getRoleSupplyAsNumberOfHolders(approvalRole);
    if (approvalPolicySupply == 0) revert RoleHasZeroSupply(approvalRole);

    uint256 disapprovalPolicySupply = llamaPolicy.getRoleSupplyAsNumberOfHolders(disapprovalRole);
    if (disapprovalPolicySupply == 0) revert RoleHasZeroSupply(disapprovalRole);

    // Save off the supplies to use for checking quorum.
    actionApprovalSupply[actionInfo.id] = approvalPolicySupply;
    actionDisapprovalSupply[actionInfo.id] = disapprovalPolicySupply;
  }

As we can see, actionApprovalSupply and actionDisapprovalSupply are set using getRoleSupplyAsNumberOfHolders which means the total number of role holders.

But while counting for totalApprovals/totalDisapprovals in getApprovalQuantityAt()/getDisapprovalQuantityAt(), it adds the quantity instead of role holders(1 for each holder).

function getApprovalQuantityAt(address policyholder, uint8 role, uint256 timestamp) external view returns (uint128) {
    if (role != approvalRole && !forceApprovalRole[role]) return 0;
    uint128 quantity = policy.getPastQuantity(policyholder, role, timestamp);
    return quantity > 0 && forceApprovalRole[role] ? type(uint128).max : quantity; //@audit should return supply, not quantity
}

So the governance result would be wrong with the below example.

1. There are 3 role holders(Alice, Bob, Charlie) and Alice has 2 quantities, others have 1.
2. During the action creation with the LlamaRelativeQuorum strategy, actionApprovalSupply = 3 and there should be 2 approved holders at least when minApprovalPct = 51%.
3. But if Alice approves the action, the result of getApprovalQuantityAt() will be 2 and the action will be approved with only one approval.

It's because getApprovalQuantityAt() return the quantity although actionApprovalSupply equals NumberOfHolders.

Tools Used

Manual Review

Recommended Mitigation Steps

getApprovalQuantityAt() and getDisapprovalQuantityAt() should return 1 instead of quantity for the positive quantity.

I think we can modify these functions like below.

  function getApprovalQuantityAt(address policyholder, uint8 role, uint256 timestamp) external view returns (uint128) {
    if (role != approvalRole && !forceApprovalRole[role]) return 0;
    uint128 quantity = policy.getPastQuantity(policyholder, role, timestamp);
    
    if (quantity > 1) quantity = 1;

    return quantity > 0 && forceApprovalRole[role] ? type(uint128).max : quantity;
  }

  function getDisapprovalQuantityAt(address policyholder, uint8 role, uint256 timestamp)
    external
    view
    returns (uint128)
  {
    if (role != disapprovalRole && !forceDisapprovalRole[role]) return 0;
    uint128 quantity = policy.getPastQuantity(policyholder, role, timestamp);

    if (quantity > 1) quantity = 1;

    return quantity > 0 && forceDisapprovalRole[role] ? type(uint128).max : quantity;
  }

Assessed type

Governance

Lines of code

https://github.com/code-423n4/2023-06-llama/blob/9d641b32e3f4092cc81dbac7b1c451c695e78983/src/strategies/LlamaRelativeQuorum.sol#L199

Vulnerability details

Impact

The governance result might be manipulated seriously because the approval/disapproval supplies can be changed anytime by an attacker.

Proof of Concept

The LlamaRelativeQuorum uses approval/disapproval thresholds that are specified as percentages of total supply and the approval/disapproval supplies are set at validateActionCreation() during the action creation.

  function validateActionCreation(ActionInfo calldata actionInfo) external {
    LlamaPolicy llamaPolicy = policy; // Reduce SLOADs.
    uint256 approvalPolicySupply = llamaPolicy.getRoleSupplyAsNumberOfHolders(approvalRole);
    if (approvalPolicySupply == 0) revert RoleHasZeroSupply(approvalRole);

    uint256 disapprovalPolicySupply = llamaPolicy.getRoleSupplyAsNumberOfHolders(disapprovalRole);
    if (disapprovalPolicySupply == 0) revert RoleHasZeroSupply(disapprovalRole);

    // Save off the supplies to use for checking quorum.
    actionApprovalSupply[actionInfo.id] = approvalPolicySupply; //@audit can be changed again after creation
    actionDisapprovalSupply[actionInfo.id] = disapprovalPolicySupply;
  }

After that, these supplies are used to check if the action is approved/disapproved.

  function isActionApproved(ActionInfo calldata actionInfo) public view returns (bool) {
    Action memory action = llamaCore.getAction(actionInfo.id);
    return action.totalApprovals >= _getMinimumAmountNeeded(actionApprovalSupply[actionInfo.id], minApprovalPct);
  }

  /// @inheritdoc ILlamaStrategy
  function isActionDisapproved(ActionInfo calldata actionInfo) public view returns (bool) {
    Action memory action = llamaCore.getAction(actionInfo.id);
    return
      action.totalDisapprovals >= _getMinimumAmountNeeded(actionDisapprovalSupply[actionInfo.id], minDisapprovalPct);
  }

But validateActionCreation() can be called by any user to change these supplies and it might bring the wrong governance result.

1. An action was created and actionApprovalSupply = 10, minApprovalPct = 5100(51%) at the creation time.
2. During the approval period, 5 policyholders approved the action and totalApprovals = 5 which doesn't still meet the approval threshold.
3. At that time, 1 policyholder of the approval role was revoked and one user who wants to execute the proposal called validateActionCreation() with the current action id.
4. Then actionApprovalSupply will be changed to 9 and the action can be approved immediately because 5 >= 9 * 51%(=5).

Like this comment explains, actionApprovalSupply shouldn't be changed again.

Tools Used

Manual Review

Recommended Mitigation Steps

We should add a modifier like onlyLlamaCore to validateActionCreation() so that this function can be called during the action creation only.

Assessed type

Access Control

Lines of code

https://github.com/code-423n4/2023-06-llama/blob/9d641b32e3f4092cc81dbac7b1c451c695e78983/src/LlamaPolicy.sol#L408

Vulnerability details

Impact

The revoking logic wouldn't work properly by an expired role holder.

Proof of Concept

_setRoleHolder() is used to set/revoke policy roles and it calls _assertNoActionCreationsAtCurrentTimestamp() to check if there is no action creation at the same time.

  function _assertNoActionCreationsAtCurrentTimestamp() internal view {
    if (llamaExecutor == address(0)) return; // Skip check during initialization.
    address llamaCore = LlamaExecutor(llamaExecutor).LLAMA_CORE();
    uint256 lastActionCreation = LlamaCore(llamaCore).getLastActionTimestamp();
    if (lastActionCreation == block.timestamp) revert ActionCreationAtSameTimestamp(); //@audit possible DOS
  }

It's introduced to prevent checkpoint manipulation as the current protocol uses block.timestamp instead of block.timestamp - 1.

But it brings a DoS opportunity for some policyholders.

1. Alice had a role to create an action and the role was expired.
2. Now her role can be revoked by anyone using revokeExpiredRole() but Alice wants to keep her role.
3. So when revokeExpiredRole() is called, Alice creates an action with meaningless settings by frontrunning, and revokeExpiredRole() will revert.
4. As it doesn't cost to create an action for Alice(just high gas cost for frontrunning), it's very likely to keep the expired role.

Tools Used

Manual Review

Recommended Mitigation Steps

As one mitigation, I think we can prevent for the expired roles to create a new action. Then it would reduce the DoS opportunity to keep the expired roles by creating a new action.

Assessed type

DoS