refactor: (C4#126) disallow setting keyManager for lsp20 extension (#648

)
lukso-network · Aug 9, 2023 · c6cc1a4 · c6cc1a4
1 parent 53f6326
commit c6cc1a4
Show file tree

Hide file tree

Showing 3 changed files with 104 additions and 1 deletion.
diff --git a/contracts/LSP6KeyManager/LSP6Errors.sol b/contracts/LSP6KeyManager/LSP6Errors.sol
@@ -165,3 +165,8 @@ error RelayCallBeforeStartTime();
  * @dev reverts when the period to execute the relay call has expired
  */
 error RelayCallExpired();
+
+/**
+ * @dev reverts when the address of the Key Manager is being set as extensions for lsp20 functions
+ */
+error KeyManagerCannotBeSetAsExtensionForLSP20Functions();
diff --git a/contracts/LSP6KeyManager/LSP6Modules/LSP6SetDataModule.sol b/contracts/LSP6KeyManager/LSP6Modules/LSP6SetDataModule.sol
@@ -1,6 +1,11 @@
 // SPDX-License-Identifier: Apache-2.0
 pragma solidity ^0.8.5;
 
+// interfaces
+import {
+    ILSP20CallVerifier as ILSP20
+} from "../../LSP20CallVerification/ILSP20CallVerifier.sol";
+
 // modules
 import {ERC725Y} from "@erc725/smart-contracts/contracts/ERC725Y.sol";
 import {
@@ -43,7 +48,8 @@ import {
     InvalidEncodedAllowedERC725YDataKeys,
     NoERC725YDataKeysAllowed,
     NotAllowedERC725YDataKey,
-    NotAuthorised
+    NotAuthorised,
+    KeyManagerCannotBeSetAsExtensionForLSP20Functions
 } from "../LSP6Errors.sol";
 
 abstract contract LSP6SetDataModule {
@@ -299,6 +305,18 @@ abstract contract LSP6SetDataModule {
 
             // LSP17Extension:<bytes4>
         } else if (bytes12(inputDataKey) == _LSP17_EXTENSION_PREFIX) {
+            // reverts when the address of the Key Manager is being set as extensions for lsp20 functions
+            bytes4 selector = bytes4(inputDataKey << 96);
+
+            if (
+                (selector == ILSP20.lsp20VerifyCall.selector ||
+                    selector == ILSP20.lsp20VerifyCallResult.selector)
+            ) {
+                if (address(bytes20(inputDataValue)) == address(this)) {
+                    revert KeyManagerCannotBeSetAsExtensionForLSP20Functions();
+                }
+            }
+
             // same as above. If controller has both permissions, do not read the `target` storage
             // to save gas by avoiding an extra external `view` call.
             if (

diff --git a/tests/LSP6KeyManager/Admin/PermissionChangeAddExtensions.test.ts b/tests/LSP6KeyManager/Admin/PermissionChangeAddExtensions.test.ts
@@ -193,6 +193,86 @@ export const shouldBehaveLikePermissionChangeOrAddExtensions = (
           const result = await context.universalProfile.getData(payloadParam.dataKey);
           expect(result).to.equal(payloadParam.dataValue);
         });
+
+        it('should revert when adding the KeyManager address as extensions for lsp20VerifyCall function', async () => {
+          const lsp20VerifyCallSelector =
+            context.keyManager.interface.getSighash('lsp20VerifyCall');
+
+          const payloadParam = {
+            dataKey:
+              ERC725YDataKeys.LSP17.LSP17ExtensionPrefix +
+              lsp20VerifyCallSelector.slice(2) +
+              '00000000000000000000000000000000', // zero padded,
+            dataValue: context.keyManager.address,
+          };
+
+          const payload = context.universalProfile.interface.encodeFunctionData('setData', [
+            payloadParam.dataKey,
+            payloadParam.dataValue,
+          ]);
+
+          await expect(
+            context.keyManager.connect(context.owner).execute(payload),
+          ).to.be.revertedWithCustomError(
+            context.keyManager,
+            'KeyManagerCannotBeSetAsExtensionForLSP20Functions',
+          );
+        });
+
+        it('should revert when adding the KeyManager address as extensions for lsp20VerifyCallResult function', async () => {
+          const lsp20VerifyCallResultSelector =
+            context.keyManager.interface.getSighash('lsp20VerifyCallResult');
+
+          const payloadParam = {
+            dataKey:
+              ERC725YDataKeys.LSP17.LSP17ExtensionPrefix +
+              lsp20VerifyCallResultSelector.slice(2) +
+              '00000000000000000000000000000000', // zero padded,
+            dataValue: context.keyManager.address,
+          };
+
+          const payload = context.universalProfile.interface.encodeFunctionData('setData', [
+            payloadParam.dataKey,
+            payloadParam.dataValue,
+          ]);
+
+          await expect(
+            context.keyManager.connect(context.owner).execute(payload),
+          ).to.be.revertedWithCustomError(
+            context.keyManager,
+            'KeyManagerCannotBeSetAsExtensionForLSP20Functions',
+          );
+        });
+
+        it('should pass when adding the random address as extensions for lsp20VerifyCall and lsp20VerifyCallResult functions', async () => {
+          const lsp20VerifyCallSelector =
+            context.keyManager.interface.getSighash('lsp20VerifyCall');
+
+          const lsp20VerifyCallResultSelector =
+            context.keyManager.interface.getSighash('lsp20VerifyCallResult');
+
+          const payloadParam = {
+            dataKeys: [
+              ERC725YDataKeys.LSP17.LSP17ExtensionPrefix +
+                lsp20VerifyCallSelector.slice(2) +
+                '00000000000000000000000000000000',
+              ERC725YDataKeys.LSP17.LSP17ExtensionPrefix +
+                lsp20VerifyCallResultSelector.slice(2) +
+                '00000000000000000000000000000000',
+            ], // zero padded,
+            dataValues: [context.accounts[2].address, context.accounts[3].address],
+          };
+
+          const payload = context.universalProfile.interface.encodeFunctionData('setDataBatch', [
+            payloadParam.dataKeys,
+            payloadParam.dataValues,
+          ]);
+
+          await context.keyManager.connect(context.owner).execute(payload);
+
+          const result = await context.universalProfile.getDataBatch(payloadParam.dataKeys);
+          expect(result).to.deep.equal(payloadParam.dataValues);
+        });
       });
 
       describe('when caller is an address with ADD/CHANGE Extensions permission', () => {