M-02.md

applySingleTargetPermissions function in PermissionManager fails to verify operation of type GrantWithCondition

Impact

The function applySingleTargetPermissions present in the PermissionManager contract is one of the ways provided by the contract to apply permissions in batch.

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/core/permission/PermissionManager.sol#L146-L163

function applySingleTargetPermissions(
    address _where,
    PermissionLib.SingleTargetPermission[] calldata items
) external virtual auth(ROOT_PERMISSION_ID) {
    for (uint256 i; i < items.length; ) {
        PermissionLib.SingleTargetPermission memory item = items[i];

        if (item.operation == PermissionLib.Operation.Grant) {
            _grant(_where, item.who, item.permissionId);
        } else if (item.operation == PermissionLib.Operation.Revoke) {
            _revoke(_where, item.who, item.permissionId);
        }

        unchecked {
            ++i;
        }
    }
}

Each permission item defines an "operation". The different types of operations defined in PermissionLib: Grant, Revoke and GrantWithCondition, the latter being a type of grant that delegates the permission to a third party contract.

This function doesn't seem to support GrantWithCondition as the SingleTargetPermission struct doesn't have a condition address, as MultiTargetPermission does. However, the applySingleTargetPermissions function fails to consider or validate this case, as the if statements only predicate about the Grant and Revoke types. Any permissions defined with type GrantWithCondition (which is technically allowed as an input parameter) will silently succeed without any effect.

Proof of Concept

In this test we execute an action in the DAO to grant a permission of type GrantWithCondition. While the action is successfully executed, the permission has no effect.

Note: the snippet shows only the relevant code for the test. Full test file can be found here.

function test_PermissionManager_applySingleTargetPermissions_IgnoresGrantWithCondition() public {
    DAO dao = createDao();

    address where = makeAddr("where");
    address who = makeAddr("who");
    bytes32 permissionId = bytes32(uint256(0xdeadbeef));

    PermissionLib.SingleTargetPermission[] memory permissions = new PermissionLib.SingleTargetPermission[](1);
    permissions[0].operation = PermissionLib.Operation.GrantWithCondition;
    permissions[0].who = who;
    permissions[0].permissionId = permissionId;

    DAO.Action[] memory applyPermissionsActions = new DAO.Action[](1);
    applyPermissionsActions[0].to = address(dao);
    applyPermissionsActions[0].data = abi.encodeWithSelector(
        PermissionManager.applySingleTargetPermissions.selector,
        where,
        permissions
    );

    // Execution succeeds...
    dao.execute(bytes32(uint256(0)), applyPermissionsActions, 0);

    // Permission isn't granted
    assertFalse(dao.hasPermission(where, who, permissionId, ""));
}

Recommendation

The applySingleTargetPermissions function should explicitly revert in case of GrantWithCondition as this type of operation is not supported in the case of single target permissions.

diff --git a/src/core/permission/PermissionManager.sol b/src/core/permission/PermissionManager.sol
index d3f392d..b4c10af 100644
--- a/src/core/permission/PermissionManager.sol
+++ b/src/core/permission/PermissionManager.sol
@@ -56,6 +56,8 @@ abstract contract PermissionManager is Initializable {
     /// @notice Thrown for permission grants where `who` and `where` are both `ANY_ADDR`.
     error AnyAddressDisallowedForWhoAndWhere();
 
+    error UnsupportedOperationType(PermissionLib.Operation operation);
+
     /// @notice Emitted when a permission `permission` is granted in the context `here` to the address `_who` for the contract `_where`.
     /// @param permissionId The permission identifier.
     /// @param here The address of the context in which the permission is granted.
@@ -154,6 +156,8 @@ abstract contract PermissionManager is Initializable {
                 _grant(_where, item.who, item.permissionId);
             } else if (item.operation == PermissionLib.Operation.Revoke) {
                 _revoke(_where, item.who, item.permissionId);
+            } else {
+                revert UnsupportedOperationType(item.operation);
             }
 
             unchecked {