condition restrictions

aragon · Oct 29, 2024 · 562330f · 562330f
1 parent dc79242
commit 562330f
Show file tree

Hide file tree

Showing 3 changed files with 52 additions and 19 deletions.
diff --git a/packages/contracts/src/core/permission/PermissionManager.sol b/packages/contracts/src/core/permission/PermissionManager.sol
@@ -191,6 +191,12 @@ abstract contract PermissionManager is Initializable {
             PermissionLib.MultiTargetPermission memory item = _items[i];
 
             if (item.operation == PermissionLib.Operation.Grant) {
+                // Ensure a non-zero condition isn't passed, as `_grant` can't handle conditions.
+                // This avoids the false impression that a conditional grant occurred,
+                // since the transaction would still succeed without conditions.
+                if (item.condition != address(0)) {
+                    revert GrantWithConditionNotSupported();
+                }
                 _grant({_where: item.where, _who: item.who, _permissionId: item.permissionId});
             } else if (item.operation == PermissionLib.Operation.Revoke) {
                 _revoke({_where: item.where, _who: item.who, _permissionId: item.permissionId});
@@ -336,10 +342,19 @@ abstract contract PermissionManager is Initializable {
     /// @param _permissionId The permission identifier.
     /// @dev Note, that granting permissions with `_who` or `_where` equal to `ANY_ADDR` does not replace other permissions with specific `_who` and `_where` addresses that exist in parallel.
     function _grant(address _where, address _who, bytes32 _permissionId) internal virtual {
-        if (_where == ANY_ADDR || _who == ANY_ADDR) {
+        if (_where == ANY_ADDR) {
             revert PermissionsForAnyAddressDisallowed();
         }
 
+        if (_who == ANY_ADDR) {
+            if (
+                _permissionId == ROOT_PERMISSION_ID ||
+                isPermissionRestrictedForAnyAddr(_permissionId)
+            ) {
+                revert PermissionsForAnyAddressDisallowed();
+            }
+        }
+
         bytes32 permHash = permissionHash({
             _where: _where,
             _who: _who,

diff --git a/packages/contracts/test/core/dao/dao.ts b/packages/contracts/test/core/dao/dao.ts
@@ -85,7 +85,7 @@ const EVENTS = {
 export const VALID_ERC1271_SIGNATURE = '0x1626ba7e';
 export const INVALID_ERC1271_SIGNATURE = '0xffffffff';
 
-describe.only('DAO', function () {
+describe('DAO', function () {
   let signers: SignerWithAddress[];
   let ownerAddress: string;
   let dao: DAO;

diff --git a/packages/contracts/test/core/permission/permission-manager.ts b/packages/contracts/test/core/permission/permission-manager.ts
@@ -94,28 +94,24 @@ describe('Core: PermissionManager', function () {
       ).to.be.revertedWithCustomError(pm, 'PermissionsForAnyAddressDisallowed');
     });
 
-    it('reverts if permissionId is restricted and `_who == ANY_ADDR` or `_where == ANY_ADDR`', async () => {
-      for (let i = 0; i < RESTRICTED_PERMISSIONS_FOR_ANY_ADDR.length; i++) {
-        await expect(
-          pm.grant(pm.address, ANY_ADDR, RESTRICTED_PERMISSIONS_FOR_ANY_ADDR[i])
-        ).to.be.revertedWithCustomError(
-          pm,
-          'PermissionsForAnyAddressDisallowed'
-        );
-        await expect(
-          pm.grant(ANY_ADDR, pm.address, RESTRICTED_PERMISSIONS_FOR_ANY_ADDR[i])
-        ).to.be.revertedWithCustomError(
-          pm,
-          'PermissionsForAnyAddressDisallowed'
-        );
-      }
+    it('reverts if permissionId is restricted and `_who == ANY_ADDR`', async () => {
+      await expect(
+        pm.grant(pm.address, ANY_ADDR, RESTRICTED_PERMISSIONS_FOR_ANY_ADDR[0])
+      ).to.be.revertedWithCustomError(pm, 'PermissionsForAnyAddressDisallowed');
     });
 
-    it('reverts if permissionId is not restricted and`_who == ANY_ADDR` or `_where == ANY_ADDR`', async () => {
+    it('succeeds if permissionId is not restricted and `_who == ANY_ADDR`', async () => {
+      await expect(pm.grant(pm.address, ANY_ADDR, ADMIN_PERMISSION_ID)).to.not
+        .be.reverted;
+    });
+
+    it('reverts if permissionId is restricted and `_where == ANY_ADDR`', async () => {
       await expect(
-        pm.grant(pm.address, ANY_ADDR, ADMIN_PERMISSION_ID)
+        pm.grant(ANY_ADDR, pm.address, RESTRICTED_PERMISSIONS_FOR_ANY_ADDR[0])
       ).to.be.revertedWithCustomError(pm, 'PermissionsForAnyAddressDisallowed');
+    });
 
+    it('reverts if permissionId is not restricted and `_where == ANY_ADDR`', async () => {
       await expect(
         pm.grant(ANY_ADDR, pm.address, ADMIN_PERMISSION_ID)
       ).to.be.revertedWithCustomError(pm, 'PermissionsForAnyAddressDisallowed');
@@ -504,6 +500,28 @@ describe('Core: PermissionManager', function () {
       }
     });
 
+    it('should revert if non-zero condition is used with `grant` operation type', async () => {
+      const signers = await ethers.getSigners();
+
+      const conditionMock = await new PermissionConditionMock__factory(
+        signers[0]
+      ).deploy();
+
+      const bulkItems: MultiTargetPermission[] = [
+        {
+          operation: Operation.Grant,
+          where: signers[1].address,
+          who: signers[0].address,
+          condition: conditionMock.address,
+          permissionId: ADMIN_PERMISSION_ID,
+        },
+      ];
+
+      await expect(
+        pm.applyMultiTargetPermissions(bulkItems)
+      ).to.be.revertedWithCustomError(pm, 'GrantWithConditionNotSupported');
+    });
+
     it('should grant with condition', async () => {
       const signers = await ethers.getSigners();