code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-06-llama/blob/aac904d31639c1b4b4e97f1c76b9c0f40b8e5cee/src/strategies/LlamaRelativeQuorum.sol#L204-L205 https://github.com/code-423n4/2023-06-llama/blob/aac904d31639c1b4b4e97f1c76b9c0f40b8e5cee/src/strategies/LlamaAbsoluteQuorum.sol#L32-L33 https://github.com/code-423n4/2023-06-llama/blob/aac904d31639c1b4b4e97f1c76b9c0f40b8e5cee/src/strategies/LlamaAbsolutePeerReview.sol#L45-L46 https://github.com/code-423n4/2023-06-llama/blob/aac904d31639c1b4b4e97f1c76b9c0f40b8e5cee/src/strategies/LlamaAbsolutePeerReview.sol#L74-L82 https://github.com/code-423n4/2023-06-llama/blob/aac904d31639c1b4b4e97f1c76b9c0f40b8e5cee/src/strategies/LlamaAbsoluteQuorum.sol#L55-L62 https://github.com/code-423n4/2023-06-llama/blob/aac904d31639c1b4b4e97f1c76b9c0f40b8e5cee/src/strategies/LlamaRelativeQuorum.sol#L229-L232 https://github.com/code-423n4/2023-06-llama/blob/aac904d31639c1b4b4e97f1c76b9c0f40b8e5cee/src/LlamaCore.sol#L516-L562

Vulnerability details

Impact

Every strategy has a validateActionCreation function which checks if anybody holds a approval and disapproval role and reverts if not when a Policyholder tries to create an action. Checking the disapproval role is unnecessary if disapproving is disabled and can prevent Policyholders from creating actions.

The condition check occurs in all strategies here is an example from LlamaRelativeQuorum.sol:

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 possibility to disable disapproving also occurs in all strategies here is a example from LlamaRelativeQuorum.sol on how to check if disapproval is enabled:

function isDisapprovalEnabled(ActionInfo calldata, address, uint8 role) external view {
  if (minDisapprovalPct > ONE_HUNDRED_IN_BPS) revert DisapprovalDisabled();
  if (role != disapprovalRole && !forceDisapprovalRole[role]) revert InvalidRole(disapprovalRole);
}

The validateActionCreation function of the strategy is called inside the _createAction function of LlamaCore.sol which can lead to a DoS for Policyholders.

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();

    PermissionData memory permission = PermissionData(target, bytes4(data), strategy);
    bytes32 permissionId = keccak256(abi.encode(permission));

    // Typically (such as in Governor contracts) this should check that the caller has permission
    // at `block.number|timestamp - 1` but here we're just checking if the caller *currently* has
    // permission. Technically this introduces a race condition if e.g. an action to revoke a role
    // from someone (or revoke a permission from a role) is ready to be executed at the same time as
    // an action is created, as the order of transactions in the block then affects if action
    // creation would succeed. However, we are ok with this tradeoff because it means we don't need
    // to checkpoint the `canCreateAction` mapping which is simpler and cheaper, and in practice
    // this race condition is unlikely to matter.
    if (!policy.hasPermissionId(policyholder, role, permissionId)) revert PolicyholderDoesNotHavePermission();

    // Validate action creation.
    actionId = actionsCount;

    ActionInfo memory actionInfo = ActionInfo(actionId, policyholder, role, strategy, target, value, data);
    strategy.validateActionCreation(actionInfo);

    // ...
  }

Proof of Concept

Create a LlamaStrategy instance with disapproving disabled and a disapproving role without holders.
Try to create a valid action and it will revert.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;

import {Test, console2, StdStorage, stdStorage} from "forge-std/Test.sol";

import {MockAccountLogicContract} from "test/mock/MockAccountLogicContract.sol";
import {MockActionGuard} from "test/mock/MockActionGuard.sol";
import {MockMaliciousExtension} from "test/mock/MockMaliciousExtension.sol";
import {MockPoorlyImplementedAbsolutePeerReview} from "test/mock/MockPoorlyImplementedStrategy.sol";
import {MockProtocol} from "test/mock/MockProtocol.sol";
import {SolarrayLlama} from "test/utils/SolarrayLlama.sol";
import {LlamaCoreSigUtils} from "test/utils/LlamaCoreSigUtils.sol";
import {LlamaFactoryWithoutInitialization} from "test/utils/LlamaFactoryWithoutInitialization.sol";
import {Roles, LlamaTestSetup} from "test/utils/LlamaTestSetup.sol";

import {DeployUtils} from "script/DeployUtils.sol";

import {LlamaAccount} from "src/accounts/LlamaAccount.sol";
import {ILlamaAccount} from "src/interfaces/ILlamaAccount.sol";
import {ILlamaActionGuard} from "src/interfaces/ILlamaActionGuard.sol";
import {ILlamaStrategy} from "src/interfaces/ILlamaStrategy.sol";
import {ActionState} from "src/lib/Enums.sol";
import {Action, ActionInfo, PermissionData, RoleHolderData, RolePermissionData} from "src/lib/Structs.sol";
import {LlamaAbsolutePeerReview} from "src/strategies/LlamaAbsolutePeerReview.sol";
import {LlamaAbsoluteStrategyBase} from "src/strategies/LlamaAbsoluteStrategyBase.sol";
import {LlamaRelativeQuorum} from "src/strategies/LlamaRelativeQuorum.sol";
import {LlamaCore} from "src/LlamaCore.sol";
import {LlamaExecutor} from "src/LlamaExecutor.sol";
import {LlamaFactory} from "src/LlamaFactory.sol";
import {LlamaPolicy} from "src/LlamaPolicy.sol";
import {RoleDescription} from "src/lib/UDVTs.sol";

contract LlamaCoreTest is LlamaTestSetup, LlamaCoreSigUtils {
  function setUp() public virtual override {
    LlamaTestSetup.setUp();

    // Setting Mock Protocol Core's EIP-712 Domain Hash
    setDomainHash(
      LlamaCoreSigUtils.EIP712Domain({
        name: mpCore.name(),
        version: "1",
        chainId: block.chainid,
        verifyingContract: address(mpCore)
      })
    );
  }
}

contract ProofOfConcept is LlamaCoreTest {
    function testProofOfConcept() public {
        // Create a new role
        LlamaPolicy policy = LlamaPolicy(mpCore.policy());
        vm.prank(address(policy.llamaExecutor()));
        policy.initializeRole(RoleDescription.wrap("NewRole"));

        // Create new Strategy with disapproving disabled and the new role set as disapproval role
        LlamaRelativeQuorum strategyLogic = new LlamaRelativeQuorum();
        vm.prank(address(rootExecutor));
        factory.authorizeStrategyLogic(strategyLogic);
        LlamaRelativeQuorum.Config[] memory newStrategyArr = new LlamaRelativeQuorum.Config[](1);
        newStrategyArr[0] = LlamaRelativeQuorum.Config({
        approvalPeriod: 4 days,
        queuingPeriod: 14 days,
        expirationPeriod: 3 days,
        isFixedLengthApprovalPeriod: true,
        minApprovalPct: 0,
        minDisapprovalPct: 10_000, // disables disapproving strategy
        approvalRole: uint8(Roles.Approver),
        disapprovalRole: uint8(policy.numRoles()),
        forceApprovalRoles: new uint8[](0),
        forceDisapprovalRoles: new uint8[](0)
        });
        ILlamaStrategy newStrategy = lens.computeLlamaStrategyAddress(
            address(strategyLogic), DeployUtils.encodeStrategy(newStrategyArr[0]), address(mpCore)
        );
        vm.startPrank(address(mpExecutor));
        mpCore.createStrategies(strategyLogic, DeployUtils.encodeStrategyConfigs(newStrategyArr));

        // Set a permission to create an action with the new strategy
        vm.prank(address(mpExecutor));
        bytes32 newPermissionId = keccak256(abi.encode(address(mockProtocol), PAUSE_SELECTOR, newStrategy));
        policy.setRolePermission(uint8(Roles.ActionCreator), newPermissionId, true);

        // Trying to create a valid action but it reverts with the RoleHasZeroSupply error
        vm.expectRevert(abi.encodeWithSelector(LlamaRelativeQuorum.RoleHasZeroSupply.selector, 9));
        vm.prank(actionCreatorAaron);
        mpCore.createAction(uint8(Roles.ActionCreator), newStrategy, address(mockProtocol), 0, abi.encodeCall(MockProtocol.pause, (true)), "");
    }
}

Tools Used

Manual Review, Foundry, VSCode

Recommended Mitigation Steps

Check first if disapproval is enabled and only check for the disapprovalPolicySupply if it is enabled.

Assessed type

DoS

0xSorryNotSorry commented 1 year ago

This looks like as it's the intended behaviour in the context of halting a governance.

c4-pre-sort commented 1 year ago

0xSorryNotSorry marked the issue as primary issue

c4-sponsor commented 1 year ago

0xrajath marked the issue as sponsor acknowledged

c4-sponsor commented 1 year ago

0xrajath marked the issue as disagree with severity

0xrajath commented 1 year ago

Acknowledging this since it's a valid concern. But can be marked as low severity since there are multiple ways around this. We can just assign the same role to approval role and disapproval role or even assign a dummy role to a dummy address that can be used as a placeholder role.

AustinGreen commented 1 year ago

This is a nice informational find but not a vulnerability and more of a UX concern.

c4-judge commented 1 year ago

gzeon-c4 changed the severity to QA (Quality Assurance)