fix: (C4 #97) add check for length == 0 to prevent mask to allow …

…any data key (#659) * fix: add check for `length == 0` to prevent `mask` to allow any data key * test: add Foundry tests for Allowed ERC725Y Data Keys with 0 length `0x0000` in the list * test: add JS tests for `0x0000` as Allowed ERC725Y Data Keys
lukso-network · Aug 15, 2023 · aaf594f · aaf594f
1 parent 184bf36
commit aaf594f
Show file tree

Hide file tree

Showing 4 changed files with 342 additions and 34 deletions.
diff --git a/contracts/LSP6KeyManager/LSP6Modules/LSP6SetDataModule.sol b/contracts/LSP6KeyManager/LSP6Modules/LSP6SetDataModule.sol
@@ -570,13 +570,14 @@ abstract contract LSP6SetDataModule {
              * The length of a data key is 32 bytes.
              * Therefore we can have a fixed allowed data key which has
              * a length of 32 bytes or we can have a dynamic data key
-             * which can have a length of up to 31 bytes.
+             * which can have a length from 1 up to 31 bytes.
              */
-            if (length > 32)
+            if (length == 0 || length > 32) {
                 revert InvalidEncodedAllowedERC725YDataKeys(
                     allowedERC725YDataKeysCompacted,
                     "couldn't DECODE from storage"
                 );
+            }
 
             /**
              * The bitmask discard the last `32 - length` bytes of the input data key via ANDing &
@@ -690,13 +691,14 @@ abstract contract LSP6SetDataModule {
              * The length of a data key is 32 bytes.
              * Therefore we can have a fixed allowed data key which has
              * a length of 32 bytes or we can have a dynamic data key
-             * which can have a length of up to 31 bytes.
+             * which can have a length from 1 up to 31 bytes.
              */
-            if (length > 32)
+            if (length == 0 || length > 32) {
                 revert InvalidEncodedAllowedERC725YDataKeys(
                     allowedERC725YDataKeysCompacted,
                     "couldn't DECODE from storage"
                 );
+            }
 
             /**
              * The bitmask discard the last `32 - length` bytes of the input data key via ANDing &

diff --git a/tests/LSP6KeyManager/SetData/AllowedERC725YDataKeys.test.ts b/tests/LSP6KeyManager/SetData/AllowedERC725YDataKeys.test.ts
@@ -1712,4 +1712,46 @@ export const shouldBehaveLikeAllowedERC725YDataKeys = (
       });
     });
   });
+
+  describe('`0x0000` set under AllowedERC725YDataKeys', () => {
+    let controllerCanSetSomeKeys: SignerWithAddress;
+
+    const compactBytesArrayOfAllowedERC725YDataKeys = '0x0000';
+
+    before(async () => {
+      context = await buildContext();
+
+      controllerCanSetSomeKeys = context.accounts[1];
+
+      const permissionKeys = [
+        ERC725YDataKeys.LSP6['AddressPermissions:Permissions'] + context.owner.address.substring(2),
+        ERC725YDataKeys.LSP6['AddressPermissions:Permissions'] +
+          controllerCanSetSomeKeys.address.substring(2),
+        ERC725YDataKeys.LSP6['AddressPermissions:AllowedERC725YDataKeys'] +
+          controllerCanSetSomeKeys.address.substring(2),
+      ];
+
+      const permissionValues = [
+        ALL_PERMISSIONS,
+        PERMISSIONS.SETDATA,
+        compactBytesArrayOfAllowedERC725YDataKeys,
+      ];
+
+      await setupKeyManager(context, permissionKeys, permissionValues);
+    });
+
+    it('should revert when trying to set `bytes32(0)` as an input data key', async () => {
+      const dataKey = '0x0000000000000000000000000000000000000000000000000000000000000000';
+      const dataValue = '0x';
+
+      const setDataPayload = context.universalProfile.interface.encodeFunctionData('setData', [
+        dataKey,
+        dataValue,
+      ]);
+
+      await expect(context.keyManager.connect(controllerCanSetSomeKeys).execute(setDataPayload))
+        .to.be.revertedWithCustomError(context.keyManager, 'InvalidEncodedAllowedERC725YDataKeys')
+        .withArgs(compactBytesArrayOfAllowedERC725YDataKeys, "couldn't DECODE from storage");
+    });
+  });
 };
diff --git a/tests/LSP6KeyManager/internals/AllowedERC725YDataKeys.internal.ts b/tests/LSP6KeyManager/internals/AllowedERC725YDataKeys.internal.ts
@@ -14,6 +14,7 @@ export const testAllowedERC725YDataKeysInternals = (
 ) => {
   describe('Testing CheckAllowedERC725YDataKeys', () => {
     let context: LSP6InternalsTestContext;
+
     let dataKeys: {
       firstDynamicKey: DataKey;
       secondDynamicKey: DataKey;
@@ -24,6 +25,7 @@ export const testAllowedERC725YDataKeysInternals = (
       thirdFixedKey: DataKey;
       fourthFixedKey: DataKey;
     };
+
     let dataKeysToReturn: BytesLike[];
     let compactBytesArray_2d: BytesLike;
     let compactBytesArray_2f: BytesLike;
@@ -32,6 +34,7 @@ export const testAllowedERC725YDataKeysInternals = (
 
     before(async () => {
       context = await buildContext();
+
       dataKeys = {
         firstDynamicKey: {
           length: '0x0011',
@@ -66,6 +69,7 @@ export const testAllowedERC725YDataKeysInternals = (
           key: '0x2b58178172d258515ef1d9e7c467f6f6a09510e863ef5ad383dbfc50721183df',
         },
       };
+
       dataKeysToReturn = [
         '0x6fae27edb0b5020ca98b9af9014331fcc79241c7f12d6afbcaea07f00e53b45d',
         '0xfd63b8f031e1f4c43fbb4956e08c686aa350a051d4d3e77a1e1c7f70366207b2',
@@ -78,16 +82,19 @@ export const testAllowedERC725YDataKeysInternals = (
         '0x9fedfe7c7366c70c52b6f11531196d0c5baa77abd16117ae30dc4eeb16dd6da2',
         '0x8741530fb57ca8556c5e7b45ffac62c178d8c0ff070ff1c99652e3f099997fa6',
       ];
+
       compactBytesArray_2d =
         dataKeys.firstDynamicKey.length +
         dataKeys.firstDynamicKey.key.toString().substring(2) +
         dataKeys.secondDynamicKey.length.toString().substring(2) +
         dataKeys.secondDynamicKey.key.toString().substring(2);
+
       compactBytesArray_2f =
         dataKeys.firstFixedKey.length +
         dataKeys.firstFixedKey.key.toString().substring(2) +
         dataKeys.secondFixedKey.length.toString().substring(2) +
         dataKeys.secondFixedKey.key.toString().substring(2);
+
       compactBytesArray_2d_2f =
         dataKeys.firstDynamicKey.length +
         dataKeys.firstDynamicKey.key.toString().substring(2) +
@@ -97,6 +104,7 @@ export const testAllowedERC725YDataKeysInternals = (
         dataKeys.firstFixedKey.key.toString().substring(2) +
         dataKeys.secondFixedKey.length.toString().substring(2) +
         dataKeys.secondFixedKey.key.toString().substring(2);
+
       compactBytesArray_mixed_d_f =
         dataKeys.firstDynamicKey.length +
         dataKeys.firstDynamicKey.key.toString().substring(2) +
@@ -237,8 +245,8 @@ export const testAllowedERC725YDataKeysInternals = (
               )
               .substring(2);
 
-          expect(
-            await context.keyManagerInternalTester.verifyAllowedERC725YSingleKey(
+          await expect(
+            context.keyManagerInternalTester.verifyAllowedERC725YSingleKey(
               context.universalProfile.address,
               checkedDataKey,
               compactBytesArray_2d,
@@ -257,8 +265,8 @@ export const testAllowedERC725YDataKeysInternals = (
               )
               .substring(2);
 
-          expect(
-            await context.keyManagerInternalTester.verifyAllowedERC725YSingleKey(
+          await expect(
+            context.keyManagerInternalTester.verifyAllowedERC725YSingleKey(
               context.universalProfile.address,
               checkedDataKey,
               compactBytesArray_2d,
@@ -290,8 +298,8 @@ export const testAllowedERC725YDataKeysInternals = (
         it('checking first fixed key: should return true', async () => {
           const checkedDataKey = dataKeys.firstFixedKey.key;
 
-          expect(
-            await context.keyManagerInternalTester.verifyAllowedERC725YSingleKey(
+          await expect(
+            context.keyManagerInternalTester.verifyAllowedERC725YSingleKey(
               context.universalProfile.address,
               checkedDataKey,
               compactBytesArray_2f,
@@ -302,8 +310,8 @@ export const testAllowedERC725YDataKeysInternals = (
         it('checking second fixed key: should return true', async () => {
           const checkedDataKey = dataKeys.secondFixedKey.key;
 
-          expect(
-            await context.keyManagerInternalTester.verifyAllowedERC725YSingleKey(
+          await expect(
+            context.keyManagerInternalTester.verifyAllowedERC725YSingleKey(
               context.universalProfile.address,
               checkedDataKey,
               compactBytesArray_2f,
@@ -343,8 +351,8 @@ export const testAllowedERC725YDataKeysInternals = (
               )
               .substring(2);
 
-          expect(
-            await context.keyManagerInternalTester.verifyAllowedERC725YSingleKey(
+          await expect(
+            context.keyManagerInternalTester.verifyAllowedERC725YSingleKey(
               context.universalProfile.address,
               checkedDataKey,
               compactBytesArray_2d_2f,
@@ -363,8 +371,8 @@ export const testAllowedERC725YDataKeysInternals = (
               )
               .substring(2);
 
-          expect(
-            await context.keyManagerInternalTester.verifyAllowedERC725YSingleKey(
+          await expect(
+            context.keyManagerInternalTester.verifyAllowedERC725YSingleKey(
               context.universalProfile.address,
               checkedDataKey,
               compactBytesArray_2d_2f,
@@ -375,8 +383,8 @@ export const testAllowedERC725YDataKeysInternals = (
         it('checking first fixed key: should return true', async () => {
           const checkedDataKey = dataKeys.firstFixedKey.key;
 
-          expect(
-            await context.keyManagerInternalTester.verifyAllowedERC725YSingleKey(
+          await expect(
+            context.keyManagerInternalTester.verifyAllowedERC725YSingleKey(
               context.universalProfile.address,
               checkedDataKey,
               compactBytesArray_2d_2f,
@@ -387,8 +395,8 @@ export const testAllowedERC725YDataKeysInternals = (
         it('checking second fixed key: should return true', async () => {
           const checkedDataKey = dataKeys.secondFixedKey.key;
 
-          expect(
-            await context.keyManagerInternalTester.verifyAllowedERC725YSingleKey(
+          await expect(
+            context.keyManagerInternalTester.verifyAllowedERC725YSingleKey(
               context.universalProfile.address,
               checkedDataKey,
               compactBytesArray_2d_2f,
@@ -428,8 +436,8 @@ export const testAllowedERC725YDataKeysInternals = (
               )
               .substring(2);
 
-          expect(
-            await context.keyManagerInternalTester.verifyAllowedERC725YSingleKey(
+          await expect(
+            context.keyManagerInternalTester.verifyAllowedERC725YSingleKey(
               context.universalProfile.address,
               checkedDataKey,
               compactBytesArray_mixed_d_f,
@@ -448,8 +456,8 @@ export const testAllowedERC725YDataKeysInternals = (
               )
               .substring(2);
 
-          expect(
-            await context.keyManagerInternalTester.verifyAllowedERC725YSingleKey(
+          await expect(
+            context.keyManagerInternalTester.verifyAllowedERC725YSingleKey(
               context.universalProfile.address,
               checkedDataKey,
               compactBytesArray_mixed_d_f,
@@ -468,8 +476,8 @@ export const testAllowedERC725YDataKeysInternals = (
               )
               .substring(2);
 
-          expect(
-            await context.keyManagerInternalTester.verifyAllowedERC725YSingleKey(
+          await expect(
+            context.keyManagerInternalTester.verifyAllowedERC725YSingleKey(
               context.universalProfile.address,
               checkedDataKey,
               compactBytesArray_mixed_d_f,
@@ -488,8 +496,8 @@ export const testAllowedERC725YDataKeysInternals = (
               )
               .substring(2);
 
-          expect(
-            await context.keyManagerInternalTester.verifyAllowedERC725YSingleKey(
+          await expect(
+            context.keyManagerInternalTester.verifyAllowedERC725YSingleKey(
               context.universalProfile.address,
               checkedDataKey,
               compactBytesArray_mixed_d_f,
@@ -500,8 +508,8 @@ export const testAllowedERC725YDataKeysInternals = (
         it('checking first fixed key: should return true', async () => {
           const checkedDataKey = dataKeys.firstFixedKey.key;
 
-          expect(
-            await context.keyManagerInternalTester.verifyAllowedERC725YSingleKey(
+          await expect(
+            context.keyManagerInternalTester.verifyAllowedERC725YSingleKey(
               context.universalProfile.address,
               checkedDataKey,
               compactBytesArray_mixed_d_f,
@@ -512,8 +520,8 @@ export const testAllowedERC725YDataKeysInternals = (
         it('checking second fixed key: should return true', async () => {
           const checkedDataKey = dataKeys.secondFixedKey.key;
 
-          expect(
-            await context.keyManagerInternalTester.verifyAllowedERC725YSingleKey(
+          await expect(
+            context.keyManagerInternalTester.verifyAllowedERC725YSingleKey(
               context.universalProfile.address,
               checkedDataKey,
               compactBytesArray_mixed_d_f,
@@ -524,8 +532,8 @@ export const testAllowedERC725YDataKeysInternals = (
         it('checking third fixed key: should return true', async () => {
           const checkedDataKey = dataKeys.thirdFixedKey.key;
 
-          expect(
-            await context.keyManagerInternalTester.verifyAllowedERC725YSingleKey(
+          await expect(
+            context.keyManagerInternalTester.verifyAllowedERC725YSingleKey(
               context.universalProfile.address,
               checkedDataKey,
               compactBytesArray_mixed_d_f,
@@ -574,6 +582,32 @@ export const testAllowedERC725YDataKeysInternals = (
             .withArgs(compactBytesArray_with_invalid_length, "couldn't DECODE from storage");
         });
       });
+
+      describe('checking a CompactBytesArray of AllowedERC725YDataKeys containing `0x0000`', () => {
+        let context: LSP6InternalsTestContext;
+
+        before(async () => {
+          context = await buildContext();
+        });
+
+        it('should revert when passing `bytes32(0)` as input data key', async () => {
+          const compactBytesArrayOfAllowedERC725YDataKeys = '0x0000';
+          const inputDataKey = '0x0000000000000000000000000000000000000000000000000000000000000000';
+
+          await expect(
+            context.keyManagerInternalTester.verifyAllowedERC725YSingleKey(
+              context.universalProfile.address,
+              inputDataKey,
+              compactBytesArrayOfAllowedERC725YDataKeys,
+            ),
+          )
+            .to.be.revertedWithCustomError(
+              context.keyManagerInternalTester,
+              'InvalidEncodedAllowedERC725YDataKeys',
+            )
+            .withArgs(compactBytesArrayOfAllowedERC725YDataKeys, "couldn't DECODE from storage");
+        });
+      });
     });
 
     describe('`verifyAllowedERC725YDataKeys(..)`', () => {