Check if plugin supports expected interface Id when enabling (#74)

* [#46] Implement Guard interface in SafeProtocolManager * [#47] Create FunctionHandlerManager.sol and inherit in SafeProtocolManager * [#47] Create BaseManager contract, rename modifier, rename error, check registry while adding function handler * [#47] Update natspec doc * [#46] Setup Safe * [#46] Fix EOF * [#46] User setupTest function * [#46] Fix lint issue * [#46] Add tests * [#46] Add test with delegateCall for hooks flow * [#46] User temporary variable for storing hooks address * [#47] Implement logic for non-static calls to function handler manager, test to set function handler * [#47] Add tests for Function Handler * [#47] Pass sender address in handle function * [#47] Fix test * [#47] Use ZeroAddress from ethers * [#46] Reset tempHooksAddress * [#47] Test static call to function handler * [#47] Fix lint issue * [#47] Fix typo * [#46] Refactor tests for SafeProtocolManager as Guard * [#46] Fix failing test * [#46] Update comment * [#47] Update tests for Function Handler * [#47] Update tests for function handler * [#47] Remove test function handler from .solcover.js * [#47] Return data from handle function * [#47] Verify call data passed to handle(...) * [#47] Update doc string * [#47] Check if function handler is whitelisted * [#47] Make fallback function non-payable, optimize codesize * [#47] Fix lint issue * [#47] Add ERC165 check while adding function handler * [#47] Allow only self calls to manager * [#47] Add tests for function handler * [#47] Add tests * [#47] Rename function to `checkCallerisSender()` * [#47] Rename to OnlyAccountCallable, add docstrings * [#47] Update string in describe * [#47] Update natspec doc for RegistryManager constructor * [#47] Update natspec doc for FunctionHandlerManager * [#73] Check if plugin supports expected interface Id when enabling * [#73] Fix lint issue * [#73] Add test for Manager: block EOA as Plugin
5afe · Aug 23, 2023 · 1fdfc1b · 1fdfc1b
1 parent c8c494d
commit 1fdfc1b
Show file tree

Hide file tree

Showing 2 changed files with 38 additions and 0 deletions.
diff --git a/contracts/SafeProtocolManager.sol b/contracts/SafeProtocolManager.sol
@@ -180,6 +180,11 @@ contract SafeProtocolManager is ISafeProtocolManager, RegistryManager, HooksMana
         address plugin,
         bool allowRootAccess
     ) external noZeroOrSentinelPlugin(plugin) onlyPermittedIntegration(plugin) onlyAccount {
+        // address(0) check omitted because it is not expected to enable it as a plugin and
+        // call to it would fail. Additionally, registry should not permit address(0) as an integration.
+        if (!ISafeProtocolPlugin(plugin).supportsInterface(type(ISafeProtocolPlugin).interfaceId))
+            revert AccountDoesNotImplementValidInterfaceId(plugin);
+
         PluginAccessInfo storage senderSentinelPlugin = enabledPlugins[msg.sender][SENTINEL_MODULES];
         PluginAccessInfo storage senderPlugin = enabledPlugins[msg.sender][plugin];
 

diff --git a/test/SafeProtocolManager.spec.ts b/test/SafeProtocolManager.spec.ts
@@ -94,6 +94,39 @@ describe("SafeProtocolManager", async () => {
                     .withArgs(pluginAddress, 0, 0);
             });
 
+            it("Should not allow a Safe to enable plugin that does not support ERC165", async () => {
+                const { safeProtocolManager, safe, safeProtocolRegistry } = await loadFixture(deployContractsWithPluginFixture);
+                await safe.setModule(await safeProtocolManager.getAddress());
+
+                const mockPlugin = await hre.ethers.deployContract("MockContract");
+                await mockPlugin.givenMethodReturnBool("0x01ffc9a7", true);
+                await safeProtocolRegistry.connect(owner).addIntegration(mockPlugin.target, IntegrationType.Plugin);
+
+                await mockPlugin.givenMethodReturnBool("0x01ffc9a7", false);
+                const data = safeProtocolManager.interface.encodeFunctionData("enablePlugin", [mockPlugin.target, false]);
+                await expect(safe.exec(safe.target, 0, data))
+                    .to.be.revertedWithCustomError(safeProtocolManager, "AccountDoesNotImplementValidInterfaceId")
+                    .withArgs(mockPlugin.target);
+            });
+
+            it("Should not allow a Safe to enable plugin having no code", async () => {
+                // This test simulates case when a Plugin is added in registry but later has no code
+                // because it calls selfdestruct.
+
+                // Setup mock registry. Required to bypass ERC165 checks for user2.address as it is an EOA.
+                const safeProtocolRegistry = await hre.ethers.deployContract("MockContract");
+                await safeProtocolRegistry.givenMethodReturnBool("0x01ffc9a7", true);
+
+                const safeProtocolManager = await (
+                    await hre.ethers.getContractFactory("SafeProtocolManager")
+                ).deploy(owner.address, await safeProtocolRegistry.getAddress());
+
+                const safe = await hre.ethers.deployContract("TestExecutor", [safeProtocolManager.target], { signer: deployer });
+                const data = safeProtocolManager.interface.encodeFunctionData("enablePlugin", [user2.address, false]);
+
+                await expect(safe.exec(safe.target, 0, data)).to.be.reverted;
+            });
+
             it("Should not allow a Safe to enable plugin if flagged in registry", async () => {
                 const { safeProtocolManager, safe, plugin, safeProtocolRegistry } = await loadFixture(deployContractsWithPluginFixture);
                 await safe.setModule(await safeProtocolManager.getAddress());