fix: check for plugin method collisions

test: adjust tests in light of new contract logic
test: improve writing in state trees

src/PRBProxyRegistry.sol

@@ -191,7 +191,16 @@ contract PRBProxyRegistry is IPRBProxyRegistry {
         // Install every method in the list.
         address owner = msg.sender;
         for (uint256 i = 0; i < length;) {
-            _plugins[owner][methodList[i]] = plugin;
+            // Check for collisions.
+            bytes4 method = methodList[i];
+            if (address(_plugins[owner][method]) != address(0)) {
+                revert PRBProxyRegistry_PluginMethodCollision({
+                    currentPlugin: _plugins[owner][method],
+                    newPlugin: plugin,
+                    method: method
+                });
+            }
+            _plugins[owner][method] = plugin;
             unchecked {
                 i += 1;
             }

src/interfaces/IPRBProxy.sol

@@ -56,13 +56,13 @@ interface IPRBProxy {
                                NON-CONSTANT FUNCTIONS
     //////////////////////////////////////////////////////////////////////////*/
 
-    /// @notice Delegate calls to the provided target contract, forwarding the data. It returns the data it
+    /// @notice Delegate calls to the provided target contract by forwarding the data. It returns the data it
     /// gets back, and bubbles up any potential revert.
     ///
     /// @dev Emits an {Execute} event.
     ///
     /// Requirements:
-    /// - The caller must be either an owner or an envoy with permission.
+    /// - The caller must be either the owner or an envoy with permission.
     /// - `target` must be a contract.
     ///
     /// @param target The address of the target contract.

src/interfaces/IPRBProxyRegistry.sol

@@ -18,9 +18,15 @@ interface IPRBProxyRegistry {
     /// @notice Thrown when an action requires the owner to not have a proxy.
     error PRBProxyRegistry_OwnerHasProxy(address owner, IPRBProxy proxy);
 
-    /// @notice Thrown when installing or uninstall a plugin, and the plugin doesn't implement any method.
+    /// @notice Thrown when trying to install or uninstall a plugin, and the plugin doesn't implement any method.
     error PRBProxyRegistry_PluginEmptyMethodList(IPRBProxyPlugin plugin);
 
+    /// @notice Thrown when trying to install a plugin that implements a method that is already implemented by another
+    /// installed plugin.
+    error PRBProxyRegistry_PluginMethodCollision(
+        IPRBProxyPlugin currentPlugin, IPRBProxyPlugin newPlugin, bytes4 method
+    );
+
     /*//////////////////////////////////////////////////////////////////////////
                                        EVENTS
     //////////////////////////////////////////////////////////////////////////*/
@@ -135,13 +141,14 @@ interface IPRBProxyRegistry {
     function deploy() external returns (IPRBProxy proxy);
 
     /// @notice Deploys a new proxy via CREATE2 by setting the caller as the owner, and delegate calls to the provided
-    /// target contract by forwarding the data. It returns the data it gets back, bubbling up any potential revert.
+    /// target contract by forwarding the data. It returns the data it gets back, and bubbles up any potential revert.
     ///
     /// @dev Emits a {DeployProxy} and an {Execute} event.
     ///
     /// Requirements:
     /// - The caller must not have a proxy.
-    /// - All from {PRBProxy.execute}.
+    /// - `target` must be a contract.
+    /// - `target` must not be the registry.
     ///
     /// @param target The address of the target contract.
     /// @param data Function selector plus ABI encoded data.
@@ -164,13 +171,14 @@ interface IPRBProxyRegistry {
     /// @dev Emits an {InstallPlugin} event.
     ///
     /// Notes:
-    /// - Does not revert if the plugin is installed.
     /// - Installing a plugin is a potentially dangerous operation, because anyone will be able to run the plugin.
+    /// - Plugin methods that have the same selector as {PRBProxy.execute} will be installed, but they will never be run
+    /// by the proxy.
     ///
     /// Requirements:
     /// - The caller must have a proxy.
     /// - The plugin must have at least one implemented method.
-    /// - By design, the plugin cannot implement any method that is also implemented by the proxy itself.
+    /// - There must be no method collision with any other installed plugin.
     ///
     /// @param plugin The address of the plugin to install.
     function installPlugin(IPRBProxyPlugin plugin) external;

test/Base.t.sol

@@ -10,11 +10,13 @@ import { IPRBProxyRegistry } from "../src/interfaces/IPRBProxyRegistry.sol";
 import { PRBProxy } from "../src/PRBProxy.sol";
 import { PRBProxyRegistry } from "../src/PRBProxyRegistry.sol";
 
+import { PluginCollider } from "./mocks/plugins/PluginCollider.sol";
 import { PluginDummy } from "./mocks/plugins/PluginDummy.sol";
 import { PluginEcho } from "./mocks/plugins/PluginEcho.sol";
 import { PluginEmpty } from "./mocks/plugins/PluginEmpty.sol";
 import { PluginPanic } from "./mocks/plugins/PluginPanic.sol";
 import { PluginReverter } from "./mocks/plugins/PluginReverter.sol";
+import { PluginSablier } from "./mocks/plugins/PluginSablier.sol";
 import { PluginSelfDestructer } from "./mocks/plugins/PluginSelfDestructer.sol";
 import { TargetDummy } from "./mocks/targets/TargetDummy.sol";
 import { TargetDummyWithFallback } from "./mocks/targets/TargetDummyWithFallback.sol";
@@ -32,11 +34,13 @@ abstract contract Base_Test is Assertions, Events, StdCheats, StdUtils {
     //////////////////////////////////////////////////////////////////////////*/
 
     struct Plugins {
+        PluginCollider collider;
         PluginDummy dummy;
         PluginEcho echo;
         PluginEmpty empty;
         PluginPanic panic;
         PluginReverter reverter;
+        PluginSablier sablier;
         PluginSelfDestructer selfDestructer;
     }
 
@@ -95,11 +99,13 @@ abstract contract Base_Test is Assertions, Events, StdCheats, StdUtils {
 
         // Create the plugins.
         plugins = Plugins({
+            collider: new PluginCollider(),
             dummy: new PluginDummy(),
             echo: new PluginEcho(),
             empty: new PluginEmpty(),
             panic: new PluginPanic(),
             reverter: new PluginReverter(),
+            sablier: new PluginSablier(),
             selfDestructer: new PluginSelfDestructer()
         });
 

test/mocks/plugins/PluginCollider.sol

@@ -0,0 +1,19 @@
+// SPDX-License-Identifier: MIT
+pragma solidity >=0.8.18;
+
+import { IPRBProxyPlugin } from "../../../src/interfaces/IPRBProxyPlugin.sol";
+
+contract PluginCollider is IPRBProxyPlugin {
+    function methodList() external pure override returns (bytes4[] memory) {
+        bytes4[] memory methods = new bytes4[](1);
+        methods[0] = this.onAddictionFeesRefunded.selector;
+        return methods;
+    }
+
+    /// @dev The selector for this method is 0x72eba203, which is the same as the selector for
+    /// `onStreamCanceled(uint256,address,uint128,uint128)`
+    function onAddictionFeesRefunded(uint248 loanId, int168, uint192 feeAmount, int248) external pure {
+        loanId;
+        feeAmount;
+    }
+}

test/mocks/plugins/PluginSablier.sol

@@ -0,0 +1,15 @@
+// SPDX-License-Identifier: MIT
+pragma solidity >=0.8.18;
+
+import { IPRBProxyPlugin } from "../../../src/interfaces/IPRBProxyPlugin.sol";
+
+contract PluginSablier is IPRBProxyPlugin {
+    function methodList() external pure override returns (bytes4[] memory) {
+        bytes4[] memory methods = new bytes4[](1);
+        methods[0] = this.onStreamCanceled.selector;
+        return methods;
+    }
+
+    /// @dev The 4-byte selector for this method is 0x72eba203.
+    function onStreamCanceled(uint256, address, uint128, uint128) external pure { }
+}

test/registry/install-plugin/installPlugin.t.sol

@@ -31,32 +31,24 @@ contract InstallPlugin_Test is Registry_Test {
         _;
     }
 
-    function test_InstallPlugin_PluginInstalledBefore()
-        external
-        whenCallerHasProxy
-        whenPluginListNotEmpty
-        whenPluginNotInstalled
-    {
-        // Install a dummy plugin that has some methods.
-        registry.installPlugin(plugins.dummy);
-
-        // Install the same plugin again.
-        registry.installPlugin(plugins.dummy);
-
-        // Assert that every plugin method has been installed.
-        bytes4[] memory pluginMethods = plugins.dummy.methodList();
-        for (uint256 i = 0; i < pluginMethods.length; ++i) {
-            IPRBProxyPlugin actualPlugin = registry.getPluginByOwner({ owner: users.alice, method: pluginMethods[i] });
-            IPRBProxyPlugin expectedPlugin = plugins.dummy;
-            assertEq(actualPlugin, expectedPlugin, "plugin method not installed");
-        }
+    function test_RevertWhen_MethodCollision() external whenCallerHasProxy whenPluginListNotEmpty {
+        registry.installPlugin(plugins.sablier);
+        vm.expectRevert(
+            abi.encodeWithSelector(
+                IPRBProxyRegistry.PRBProxyRegistry_PluginMethodCollision.selector,
+                plugins.sablier,
+                plugins.collider,
+                plugins.sablier.onStreamCanceled.selector
+            )
+        );
+        registry.installPlugin(plugins.collider);
     }
 
-    modifier whenPluginNotInstalled() {
+    modifier whenNoMethodCollision() {
         _;
     }
 
-    function test_InstallPlugin() external whenCallerHasProxy whenPluginListNotEmpty whenPluginNotInstalled {
+    function test_InstallPlugin() external whenCallerHasProxy whenPluginListNotEmpty whenNoMethodCollision {
         // Install a dummy plugin that has some methods.
         registry.installPlugin(plugins.dummy);
 
@@ -69,7 +61,7 @@ contract InstallPlugin_Test is Registry_Test {
         }
     }
 
-    function test_InstallPlugin_Event() external whenCallerHasProxy whenPluginListNotEmpty whenPluginNotInstalled {
+    function test_InstallPlugin_Event() external whenCallerHasProxy whenPluginListNotEmpty whenNoMethodCollision {
         vm.expectEmit({ emitter: address(registry) });
         emit InstallPlugin({ owner: users.alice, proxy: proxy, plugin: plugins.dummy });
         registry.installPlugin(plugins.dummy);

test/registry/install-plugin/installPlugin.tree

@@ -5,8 +5,8 @@ installPlugin.t.sol
    ├── when the plugin list is empty
    │  └── it should revert
    └── when the plugin list is not empty
-      ├── when the plugin has been installed before
-      │  └── it should do nothing
-      └── when the plugin has not been installed
+      ├── when there is a method collision
+      │  └── it should revert
+      └── when there is no method collision
          ├── it should install the plugin
          └── it should emit an {InstallPlugin} event

test/registry/uninstall-plugin/uninstallPlugin.t.sol

@@ -35,7 +35,7 @@ contract UninstallPlugin_Test is Registry_Test {
         // Uninstall the plugin.
         registry.uninstallPlugin(plugins.dummy);
 
-        // Assert that every plugin method has been uninstalled.
+        // Assert that every plugin method has remained uninstalled.
         bytes4[] memory pluginMethods = plugins.dummy.methodList();
         for (uint256 i = 0; i < pluginMethods.length; ++i) {
             IPRBProxyPlugin actualPlugin = registry.getPluginByOwner({ owner: users.alice, method: pluginMethods[i] });
@@ -44,13 +44,13 @@ contract UninstallPlugin_Test is Registry_Test {
         }
     }
 
-    modifier whenPluginInstalled() {
+    modifier whenPluginInstalledBefore() {
         // Install the dummy plugin.
         registry.installPlugin(plugins.dummy);
         _;
     }
 
-    function test_UninstallPlugin() external whenCallerHasProxy whenPluginHasMethods whenPluginInstalled {
+    function test_UninstallPlugin() external whenCallerHasProxy whenPluginHasMethods whenPluginInstalledBefore {
         // Uninstall the plugin.
         registry.uninstallPlugin(plugins.dummy);
 
@@ -63,7 +63,7 @@ contract UninstallPlugin_Test is Registry_Test {
         }
     }
 
-    function test_UninstallPlugin_Event() external whenCallerHasProxy whenPluginHasMethods whenPluginInstalled {
+    function test_UninstallPlugin_Event() external whenCallerHasProxy whenPluginHasMethods whenPluginInstalledBefore {
         vm.expectEmit({ emitter: address(registry) });
         emit UninstallPlugin({ owner: users.alice, proxy: proxy, plugin: plugins.dummy });
         registry.uninstallPlugin(plugins.dummy);

test/registry/uninstall-plugin/uninstallPlugin.tree

@@ -7,6 +7,6 @@ uninstallPlugin.t.sol
    └── when the plugin list is not empty
       ├── when the plugin has not been installed before
       │  └── it should do nothing
-      └── when the plugin has been installed
+      └── when the plugin has been installed before
          ├── it should uninstall the plugin
          └── it should emit an {UninstallPlugin} event