Llama - 0xnev's results

Lines of code

https://github.com/code-423n4/2023-06-llama/blob/main/src/strategies/LlamaRelativeQuorum.sol#L281-L292 https://github.com/code-423n4/2023-06-llama/blob/main/src/strategies/LlamaRelativeQuorum.sol#L318-L321 https://github.com/code-423n4/2023-06-llama/blob/main/src/strategies/LlamaRelativeQuorum.sol#L199-L210

Vulnerability details

Impact

Actions can be incorrectly approved/disapproved by LlamaRelativeQuorum strategy types contracts due to incorrect threshold calculations.

Based on the following isActionApproved()/isActionDisapproved() functions check, total quantity of votes from approvals are checked against the total number of holders numHolders instead of totalQuantity based on approvalRole.

LlamaRelativeQuorum.sol#L281-L292

  /// @inheritdoc ILlamaStrategy
  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);
  }

The computation of approval/disapproval thresholds is done via _getMinimumAmountNeeded that uses the mapping of actionApprovalSupply/actionDisapprovalSupply.

LlamaRelativeQuorum.sol#L318-L321

  function _getMinimumAmountNeeded(uint256 supply, uint256 minPct) internal pure returns (uint256) {
    // Rounding Up
    return FixedPointMathLib.mulDivUp(supply, minPct, ONE_HUNDRED_IN_BPS);
  }

the mapping of actionApprovalSupply/actionDisapprovalSupply is set in validateActionCreation(), which in turn calls LlamaPolicy.getRoleSupplyAsNumberOfHolders(), which returns the number of unique policy holders having the approval/disapproval role.

LlamaRelativeQuorum.sol#L199-L210

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

Since quantity is a uint128 and can be greater than 1 but number of policyHolders holding approval/disapproval roles are determined by number of NFT holders, it is almost certian that numHolders will be much smaller than totalQuantity of approval/disapproval roles, unless all approver/disapprover role quantity are assigned a quantity of 1.

As such, the checks should instead use LlamaPolicy.getRoleSupplyAsQuantitySum() similar to other strategy types, so that thresholds are not underestimated.

This could result in 2 scenarios:

1. LlamaCore using such strategies might have actions that is approved prematurely when minimum approval threshold is not reached yet due to underestimation of minimum quantity of votes for approval.

<br/>

2. LlamaCore using such strategies might have actions that is disapproved prematurely when minimum disapproval threshold is not reached yet due to underestimation of minimum quantity of votes for disapproval.

Proof of Concept

Consider the following scenario for disapproval and approval: Can be incorrectly approved or incorrectly disapproved

// Details
numApprovers = 5
numDisapprovers = 5
minPctApproval = 5000 // (50%)
minPctDisapproval = 5000 // (50%)

// Approver details
approveRoles = 5
approver1Quantity = 1
approver2Quantity = 2
approver3Quantity = 3
approver4Quantity = 4
approver5Quantity = 5

// Disapprovers details
disapprovalRoles = 5
disapprover1Quantity = 1
disapprover2Quantity = 2
disapprover3Quantity = 3
disapprover4Quantity = 4
disapprover5Quantity = 5

// Calculation of minimum votes required for approval/disapproval
wrongMinimumApprovalAmountNeeded = 5 * 5000 / 10000 = 2.5 = 3 // rounded up
actualMinimumApprovalAmountNeeded = (1 + 2 + 3 + 4 + 5) * 5000 / 10000 = 7.5 = 8 // rounded up

wrongMinimumDisapprovalAmountNeeded = 5 * 5000 / 10000 = 2.5 = 3 // rounded up
actualMinimumDisApprovalAmountNeeded = (1 + 2 + 3 + 4 + 5) * 5000 / 10000 = 7.5 = 8 // rounded up

// Should not be approved but approves
// Assume approver1 and approver3 approves action
// Here, `isActionApproved()` returns true when it should return false
// Action creator can use this to bypass approvals
action.totalApprovals = 1 + 3 = 4
action.totalApprovals >= wrongMinimumApprovalAmountNeeded = true
action.totalApprovals >= actualMinimumApprovalAmountNeeded = false 

// Should not be disapproved but it is disapproved
// Assume disapprover3 and disapprover4 disapproves action
// Here, `isActionDisapproved()` returns true when it should return false
// Action creator can use this to bypass disapprovals
action.totalApprovals = 3 + 4 = 7
action.totalApprovals >= wrongMinimumDisapprovalAmountNeeded = true
action.totalApprovals >= actualMinimumDisApprovalAmountNeeded = false 

Tools Used

Manual Analysis

Recommendation

As mentioned, use LlamaPolicy.getRoleSupplyAsQuantitySum() instead to prevent underestimation of thresholds based on minPCT set.

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

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

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

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2023-06-llama/blob/main/src/LlamaCore.sol#L348-L358 https://github.com/code-423n4/2023-06-llama/blob/main/src/LlamaPolicy.sol#L432 https://github.com/code-423n4/2023-06-llama/blob/main/src/LlamaPolicy.sol#L407-L408

Vulnerability details

Impact

In the event where action creation and cancellation occurs within the same transaction, owner/privelledged roles can be unfairly DoSed from setting roles for new policy holders as creationTime is not updated after action is cancelled. Consider the following:

When LlamaCore.cancelAction() is called by user immediately after creation via LlamaCore.createAction(), only the action.canceled is updated but not the action.creationTime.

LlamaCore.sol#L348-L358

  function cancelAction(ActionInfo calldata actionInfo) external {
    Action storage action = actions[actionInfo.id];
    _validateActionInfoHash(action.infoHash, actionInfo);

    // We don't need an explicit check on action existence because if it doesn't exist the strategy will be the zero
    // address, and Solidity will revert since there is no code at the zero address.
    actionInfo.strategy.validateActionCancelation(actionInfo, msg.sender);
@>   /// @audit , only canceled updated, creationTime not updated
    action.canceled = true;
    emit ActionCanceled(actionInfo.id);
  }

At the same time, when owner/privilledged role calls LlamaPolicy.setRoleHolder(), it in turns call the internal function LlamaPolicy._setRoleHolder().

LlamaPolicy.sol#L432

  function _setRoleHolder(uint8 role, address policyholder, uint128 quantity, uint64 expiration) internal {
@>  _assertNoActionCreationsAtCurrentTimestamp();
    _assertValidRoleHolderUpdate(role, quantity, expiration);
    ...

The first check in _assertNoActionCreationsAtCurrentTimestamp() is to ensure there is no creation of action at the same time roles are being set due to comments stated. This functions calls LlamaCore.getLastActionTimeStamp().

LlamaPolicy.sol#L407-L408

  /// @dev Because role supplies are not checkpointed for simplicity, the following issue can occur
  /// if each of the below is executed within the same timestamp:
  //    1. An action is created that saves off the current role supply.
  //    2. A policyholder is given a new role.
  //    3. Now the total supply in that block is different than what it was at action creation.
  // As a result, we disallow changes to roles if an action was created in the same block.
  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();
  }

However, given creationTime is never updated even when cancelAction() is called (can be called anytime), this can cause owner/priveledged roles to be unecessarily DoSed from setting roles for new policy holders or when revoking policy holders.

Malicious existing policy holders can just spam create and cancelling actions until owners revoke permissions.

Note

I noticed that this is a fix to spearbit audit issue 5.3.4, but introduces another vulnerability where the ability to call cancelAction() anytime by order creator is not accounted for.

Proof of Concept

Add this test in /test

pragma solidity ^0.8.19;

import {Test, console2, StdStorage, stdStorage} from "forge-std/Test.sol";
import {MockProtocol} from "test/mock/MockProtocol.sol";
import {Roles, LlamaTestSetup} from "test/utils/LlamaTestSetup.sol";
import {Action, ActionInfo, PermissionData, RoleHolderData, RolePermissionData} from "src/lib/Structs.sol";
import {LlamaCore} from "src/LlamaCore.sol";
import {LlamaPolicy} from "src/LlamaPolicy.sol";
import {LlamaCoreTest} from "test/LlamaCore.t.sol";

contract LlamaCoreAttack is LlamaCoreTest {
   ActionInfo actionInfo; 

   // 1. Setup
  function setUp() public virtual override {
    LlamaCoreTest.setUp();
  }
  
  function test_setRoleHolderFailure() public {
    // 2. Create arbritrary action using creator Aaron
    bytes memory data = abi.encodeCall(MockProtocol.pause, (true));
    vm.prank(actionCreatorAaron);
    uint256 actionId = mpCore.createAction(uint8(Roles.ActionCreator), mpStrategy1, address(mockProtocol), 0, data, "");
    actionInfo =
      ActionInfo(actionId, actionCreatorAaron, uint8(Roles.ActionCreator), mpStrategy1, address(mockProtocol), 0, data);

    // 3. Cancel action immediately within same transaction, since cancelAction can be called anytime
    vm.prank(actionCreatorAaron);
    mpCore.cancelAction(actionInfo);

    // 4. In the event where at the same `block.timestamp`, owner tries to set role for policy holder but reverts
    // even though actions has already been cancelled
    // As such, owner is unnecessarily DoSed from setting roles for policyholders
    address actionCreatorAustin = makeAddr("actionCreatorAustin");
    vm.prank(address(mpExecutor));
    vm.expectRevert(LlamaPolicy.ActionCreationAtSameTimestamp.selector);
    mpPolicy.setRoleHolder(uint8(Roles.TestRole2), actionCreatorAustin, DEFAULT_ROLE_QTY, DEFAULT_ROLE_EXPIRATION);

    // 5. In the event where at the same `block.timestamp`, owner tries to revoke policy
    // he is prevented from doing so simply because creationTime is never updated after cancellation
    vm.prank(address(mpExecutor));
    vm.expectRevert(LlamaPolicy.ActionCreationAtSameTimestamp.selector);    
    mpPolicy.revokePolicy(actionCreatorAaron);
  }
}

Tools Used

Manual Analysis, Foundry

Recommendation

Simple fix can be made to ensure LlamaCore.cancelAction updates action.creationTime after action is cancelled for specific action:

  function cancelAction(ActionInfo calldata actionInfo) external {
    Action storage action = actions[actionInfo.id];
    _validateActionInfoHash(action.infoHash, actionInfo);

    // We don't need an explicit check on action existence because if it doesn't exist the strategy will be the zero
    // address, and Solidity will revert since there is no code at the zero address.
    actionInfo.strategy.validateActionCancelation(actionInfo, msg.sender);

    action.canceled = true;
++  action.creationTime  = 0;
    emit ActionCanceled(actionInfo.id);
  }

Assessed type

DoS

1. Analysis of Codebase

The Llama governance system provides a unique way for protocol to leverage policies (represented by a non-transferable NFT) to permission action creation till execution. It primarily focuses on 2 mechanisms, Action creation and policy management. To summarize the protocol, here is a step-by-step flow:

1. Protocol owners give policy and set roles (via _setRoleHolder())
2. Protocol owner set permissions (via _setRolePermissions())
3. Permissioned policy holders can create actions (via createAction/createActionBySig)
4. Strategy and custom guards validate action creation, if passes action can be queued (via Strategy and Guard function validateActionCreation())
5. Policy holders with approval/disapproval cast votes during approval period (via castApproval()/castDisapproval())
6. Strategies validate approval/disapproval against minimum thresholds via isActionApproved()/isActionDisapproved()
7. If approved and meets minimum execution time and action is not expired, action can now be executed, if not action is canceled

2. Summary of Vulnerabilities

Refer to vulnerabilities submitted, here is a summary

3. Architecture Improvements

The following architecture improvements and feedback could be considered:

3.1 Incorporate ERC20 tokens for action execution that requires value

Could consider incorporating payment of action execution with common ERC-20 tokens (USDC, USDT, BNB ...). The tokens incorporated can be whitelisted to prevent ERC-20 tokens with other caveats from interacting with protocol until support is implemented (e.g. rebasing, fee-on-transfer)

3.2 Create a new type of strategy for flexibility

Could consider creating a new type of Llama strategy in which approval/disapproval thresholds are specified as percentages of total supply and action creators are not allowed to cast approvals or disapprovals on their own actions for more flexibility

3.3 Checkpoints contracts are deprecated by OpenZeppelin

Checkpoint contracts seems to be deprecated by OpenZeppelin, not sure how this affects Llama contracts but since it affects core parts of the contract logic such as retrieving quantity and expiration data of roles, it might be worth noting.

3.4 Consider changing quantity check logic

Consider changing logic for action creation and checks for role before action creation. Protocol owners cannot set role with 0 quantity coupled with an expiration due to checks in _assertValidRoleHolderUpdate().

Only the approvalRole is required to have quantity. All other roles that do not have approval power but have quantity assigned to them will only incur unecessary computation.

Based on current implementation of _setRoleHolder, protocol owners can never give a policyholder a role with an expiry with no quantity that represents approval/disapproval casting power. In the event where protocol owner wants to give policyholder a role that has 0 quantity of votes, they can never do so.

Furthermore, hasPermissionId() also checks for quantity before allowing creation of actions. This means policyholders can only create actions if some quantity of approval/disapproval votes is assigned to them. Sementically, I don't think the quantity used for voting has relation to action creation.

Although that particular policy holder cannot vote unless approvalRole /disapprovalRole is assigned to them, it can cause confusion where policy holders might think they can vote since some quantity is assigned to them.

The following adjustments can be made:

• You could consider adding a third case in _assertValidRoleHolderUpdate() such as the following:

case3 = quantity == 0 && expiration > block.timestamp;

• Remove quantity > 0 check in LlamaPolicy.hasPermissionId() to allow action creators to create roles even when no quantity is assigned to them, since permissions to create actions are required to be set for policy holders via setRolePermissions() anyway.
• hasRole() can simply check for expiration to determine if policy holder has role
• A separate hasCastRole() can be created to specifically check for approval/disapproval Role

This way, quantity will only ever need to be assigned to policyholders assigned with the approval/disapproval role.

3.5 No actual way to access role descriptions via mapping

In the policy-management.md doc it states that

When roles are created, a description is provided. This description serves as the plaintext mapping from description to role ID, and provides semantic meaning to an otherwise meaningless unsigned integer.

However, there is no actual way to access roleId via role descriptions in contract. Policy holders cannot access role descriptions and roleIds convieniently except via protocol UI.

Hence, protocol could consider adding a new mapping to map roleIds to description and add logic to return role description and Id in LlamaPolicy.updateRoleDescriptions().

3.6 Consider increasing number of unique roles

Since Id 0 is reserved for the bootstrap ALL_HOLDERS_ROLE, the protocol owner could infact only have 254 unique roles. So it may be good to consider using uint16 to allow 65534 unique roles.

4. Centralization risks

4.1 Policy holders with forceApproval/forceDisapproval role can force approvals and disapproval

Policy holders will approval/disapproval role and quantity of type(uint128).max can force approval/disapproval of actions via forceApprovalRole/forceDisapproval mapping.

4.2 Protocol owners can revoke roles of policyholders anytime

Protocol owners can revoke policyholders anytime via LlamaPolicy.revokePolicy() and prevent action creation/queuing/execution and approval/disapproval. It should be noted that as long as action is created, that action can be executed regardless policyholder is revoked or not, unless action is explicitly cancelled or disapproved.

4.3 Any guards can be set for actions before execution

The type of guards can be customized by protocol owners, so at any point of time specific guards can be set for specific action based on data input (selector) and possibly unfairly prevent execution of action via LlamaCore.setGuard().

5. Time Spent

A total of 4 days were spent to cover this audit, broken down into the following:

• 1st Day: Understand protocol docs, action creation flow and policy management
• 2nd Day: Focus on linking docs logic to LLamaCore.sol and LlamaPolicy.sol, coupled with typing reports for vulnerabilities found
• 3rd Day: Focus on different types of strategies contract coupled with typing reports for vulnerabilities found
• 4th Day: Sum up audit by completing QA report and Analysis

Time spent:

96 hours