[#6] Add checks if registry supports valid interfaceId

contracts/SafeProtocolRegistry.sol

@@ -1,13 +1,9 @@
-// SPDX-License-Identifier: SEE LICENSE IN LICENSE
+// SPDX-License-Identifier: LGPL-3.0-only
 pragma solidity ^0.8.18;
 import {ISafeProtocolRegistry} from "./interfaces/Registry.sol";
 import {Ownable2Step} from "@openzeppelin/contracts/access/Ownable2Step.sol";
 
 contract SafeProtocolRegistry is ISafeProtocolRegistry, Ownable2Step {
-    /**
-     * @dev TODO: Determint whether there exists a more gas efficient way to store component info based on the way component information is accessed.
-     *      For simplicity, currently using a struct.
-     */
     mapping(address => ComponentInfo) public listedComponents;
 
     struct ComponentInfo {
@@ -69,4 +65,8 @@ contract SafeProtocolRegistry is ISafeProtocolRegistry, Ownable2Step {
         listedComponents[component] = ComponentInfo(componentInfo.listedAt, uint64(block.timestamp));
         emit ComponentFlagged(component);
     }
+
+    function supportsInterface(bytes4 interfaceId) external view override returns (bool) {
+        return interfaceId == type(ISafeProtocolRegistry).interfaceId || interfaceId == 0x01ffc9a7; //bytes4(keccak256('supportsInterface(bytes4)'));
+    }
 }

contracts/base/RegistryManager.sol

@@ -2,13 +2,15 @@
 pragma solidity ^0.8.18;
 import {Ownable2Step} from "@openzeppelin/contracts/access/Ownable2Step.sol";
 import {ISafeProtocolRegistry} from "../interfaces/Registry.sol";
+import {IERC165} from "@openzeppelin/contracts/utils/introspection/IERC165.sol";
 
 contract RegistryManager is Ownable2Step {
     address public registry;
 
     event RegistryChanged(address indexed oldRegistry, address indexed newRegistry);
 
     error ModuleNotPermitted(address module, uint64 listedAt, uint64 flaggedAt);
+    error AccountDoesNotImplementValidInterfaceId(address account);
 
     modifier onlyPermittedModule(address module) {
         // Only allow registered and non-flagged modules
@@ -21,6 +23,9 @@ contract RegistryManager is Ownable2Step {
 
     constructor(address _registry, address intitalOwner) {
         _transferOwnership(intitalOwner);
+        if (!IERC165(_registry).supportsInterface(type(ISafeProtocolRegistry).interfaceId)) {
+            revert AccountDoesNotImplementValidInterfaceId(_registry);
+        }
         registry = _registry;
     }
 
@@ -29,6 +34,9 @@ contract RegistryManager is Ownable2Step {
      * @param newRegistry Address of new registry
      */
     function setRegistry(address newRegistry) external onlyOwner {
+        if (!IERC165(newRegistry).supportsInterface(type(ISafeProtocolRegistry).interfaceId)) {
+            revert AccountDoesNotImplementValidInterfaceId(newRegistry);
+        }
         emit RegistryChanged(registry, newRegistry);
         registry = newRegistry;
     }

contracts/interfaces/Registry.sol

@@ -1,7 +1,8 @@
-// SPDX-License-Identifier: SEE LICENSE IN LICENSE
+// SPDX-License-Identifier: LGPL-3.0-only
 pragma solidity ^0.8.18;
+import {IERC165} from "@openzeppelin/contracts/utils/introspection/IERC165.sol";
 
-interface ISafeProtocolRegistry {
+interface ISafeProtocolRegistry is IERC165 {
     /// @param component Address of the component that should be checked
     /// @return listedAt MUST return the block number when the component was listed in the registry (or 0 if not listed)
     /// @return flaggedAt MUST return the block number when the component was listed in the flagged as faulty (or 0 if not flagged)

package.json

@@ -13,6 +13,7 @@
     "postinstall": "yarn typechain"
   },
   "devDependencies": {
+    "@gnosis.pm/mock-contract": "gnosis/mock-contract#master",
     "@nomicfoundation/hardhat-chai-matchers": "^2.0.0",
     "@nomicfoundation/hardhat-ethers": "^3.0.0",
     "@nomicfoundation/hardhat-network-helpers": "^1.0.0",

test/base/RegistryManager.spec.ts

@@ -3,6 +3,7 @@ import { loadFixture } from "@nomicfoundation/hardhat-toolbox/network-helpers";
 import { expect } from "chai";
 import { SignerWithAddress } from "@nomicfoundation/hardhat-ethers/signers";
 import { ZeroAddress } from "ethers";
+import { getMockRegistryWithInvalidInterfaceSupport } from "../utils/mockRegistryBuilder";
 
 describe("RegistryManager", async () => {
     let deployer: SignerWithAddress, owner: SignerWithAddress, user1: SignerWithAddress;
@@ -23,15 +24,35 @@ describe("RegistryManager", async () => {
         return { registryManager, safeProtocolRegistry };
     }
 
+    it("Should revert when registry address does not implement valid interfaceId", async () => {
+        const { registryManager } = await loadFixture(deployContractsFixture);
+        await expect(registryManager.connect(owner).setRegistry(ZeroAddress)).to.be.reverted;
+    });
+
+    it("Should revert with AccountDoesNotImplementValidInterfaceId when registry address does not implement valid interfaceId", async () => {
+        const { registryManager } = await loadFixture(deployContractsFixture);
+        const registry = await getMockRegistryWithInvalidInterfaceSupport();
+        await expect(registryManager.connect(owner).setRegistry(registry)).to.be.revertedWithCustomError(
+            registryManager,
+            "AccountDoesNotImplementValidInterfaceId",
+        );
+    });
+
     it("Should emit RegistryChanged change event when registry is updated", async () => {
         const { registryManager } = await loadFixture(deployContractsFixture);
-        await expect(registryManager.connect(user1).setRegistry(ZeroAddress)).to.be.revertedWith("Ownable: caller is not the owner");
+        const safeProtocolRegistryAddress = await (
+            await hre.ethers.deployContract("SafeProtocolRegistry", [owner.address], { signer: deployer })
+        ).getAddress();
+
+        await expect(registryManager.connect(user1).setRegistry(safeProtocolRegistryAddress)).to.be.revertedWith(
+            "Ownable: caller is not the owner",
+        );
 
-        expect(await registryManager.connect(owner).setRegistry(ZeroAddress))
+        expect(await registryManager.connect(owner).setRegistry(safeProtocolRegistryAddress))
             .to.emit(registryManager, "RegistryChanged")
-            .withArgs(await registryManager.getAddress(), ZeroAddress);
+            .withArgs(await registryManager.getAddress(), safeProtocolRegistryAddress);
 
-        expect(await registryManager.registry()).to.be.equal(ZeroAddress);
+        expect(await registryManager.registry()).to.be.equal(safeProtocolRegistryAddress);
     });
 
     it("Should not allow non-owner to update registry", async () => {

test/utils/mockRegistryBuilder.ts

@@ -0,0 +1,8 @@
+import { ISafeProtocolRegistry } from "../../typechain-types";
+import hre from "hardhat";
+
+export const getMockRegistryWithInvalidInterfaceSupport = async (): Promise<ISafeProtocolRegistry> => {
+    const registry = await (await hre.ethers.getContractFactory("MockContract")).deploy();
+    await registry.givenMethodReturnBool("0x01ffc9a7", false);
+    return hre.ethers.getContractAt("ISafeProtocolRegistry", await registry.getAddress());
+};

yarn.lock

@@ -450,6 +450,10 @@
     "@ethersproject/properties" "^5.7.0"
     "@ethersproject/strings" "^5.7.0"
 
+"@gnosis.pm/mock-contract@gnosis/mock-contract#master":
+  version "4.0.0"
+  resolved "https://codeload.github.com/gnosis/mock-contract/tar.gz/b0f735ddc62d5000b50667011d69142a4dee9c71"
+
 "@humanwhocodes/config-array@^0.11.10":
   version "0.11.10"
   resolved "https://registry.yarnpkg.com/@humanwhocodes/config-array/-/config-array-0.11.10.tgz#5a3ffe32cc9306365fb3fd572596cd602d5e12d2"