applySingleTargetPermissions
function in PermissionManager fails to verify operation of type GrantWithCondition
The function applySingleTargetPermissions
present in the PermissionManager
contract is one of the ways provided by the contract to apply permissions in batch.
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.
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, ""));
}
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 {