fix: check for plugin method collisions #129
Conversation
test: adjust tests in light of new contract logic test: improve writing in state trees
|
🤖 OpenAI High-level summary: The changes include improvements to the PRBProxyRegistry contract and related interfaces, as well as updates to test cases. The PRBProxyRegistry contract now checks for method collisions before installing a plugin and reverts if a collision is detected. The IPRBProxy.sol interface has a minor comment modification. The IPRBProxyRegistry.sol interface introduces a new error type and updates the documentation for the installPlugin function. Test cases have been refactored and expanded to cover method collisions and plugin installation scenarios. Table of file summaries:
Chat with 🤖 OpenAI Bot (
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| /// @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 | ||
| ); | ||
|
|
There was a problem hiding this comment.
🤖 OpenAI
The error messages have been updated to provide more specific information about the errors. No issues found.
| 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. | ||
| /// | ||
| /// @param target The address of the target contract. | ||
| /// @param data Function selector plus ABI encoded data. |
There was a problem hiding this comment.
🤖 OpenAI
The deploy function has been modified to include additional documentation and requirements. No issues found.
| 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"; |
There was a problem hiding this comment.
🤖 OpenAI
The import statements have been modified to include additional plugin files. No issues found.
| //////////////////////////////////////////////////////////////////////////*/ | ||
|
|
||
| struct Plugins { | ||
| PluginCollider collider; | ||
| PluginDummy dummy; | ||
| PluginEcho echo; | ||
| PluginEmpty empty; | ||
| PluginPanic panic; | ||
| PluginReverter reverter; | ||
| PluginSablier sablier; | ||
| PluginSelfDestructer selfDestructer; | ||
| } |
There was a problem hiding this comment.
🤖 OpenAI
A new struct Plugins has been introduced, which includes instances of various plugins. No issues found.
| /// @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. |
There was a problem hiding this comment.
🤖 OpenAI
The comment has been modified to clarify that calls are delegated to the target contract by forwarding the data. However, there is a typo in the comment ("It returns the data it gets back"). It should be "It returns the data it receives back". Please update the comment accordingly.
| /// @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. | |
| /// @notice Delegate calls to the provided target contract by forwarding the data. It returns the data it receives back, and bubbles up any potential revert. | |
| /// | |
| /// @dev Emits an {Execute} event. | |
| /// | |
| /// Requirements: | |
| /// - 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. |
| ├── when the plugin list is empty | ||
| │ └── it should revert | ||
| └── when the plugin list is not empty |
There was a problem hiding this comment.
🤖 OpenAI
The test case structure has changed. Please update the description of the test cases accordingly to reflect the new structure.
| ├── when there is a method collision | ||
| │ └── it should revert |
There was a problem hiding this comment.
🤖 OpenAI
The test case structure has changed. Please update the description of the test cases accordingly to reflect the new structure.
| ├── it should install the plugin | ||
| └── it should emit an {InstallPlugin} event |
There was a problem hiding this comment.
🤖 OpenAI
The test case structure has changed. Please update the description of the test cases accordingly to reflect the new structure.
| // 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; | ||
| } |
There was a problem hiding this comment.
🤖 OpenAI
The code introduces a method collision check before installing a plugin in the PRBProxyRegistry contract. This is a good addition to prevent conflicts between plugins. However, there are a few improvements that can be made:
-
Line 198: Instead of comparing the address of
_plugins[owner][method]withaddress(0), you can use the==operator to directly check if it's equal to zero. This simplifies the condition. -
Line 199: The
revertstatement should include a reason string to provide more information about the error. You can use thestringtype for the reason and pass it as an argument to therevertstatement. -
Lines 200-203: The
PRBProxyRegistry_PluginMethodCollisionevent is being reverted with the conflicting plugin and method information. It would be helpful to define this event in the contract and emit it before reverting. This allows external parties to listen to the event and capture the details of the collision. -
Line 207: Instead of using
unchecked, you can incrementiusing the regulari++syntax.
Here's the updated code with the suggested improvements:
// Define the event
event PRBProxyRegistry_PluginMethodCollision(address currentPlugin, address newPlugin, bytes4 method);
// Install every method in the list.
address owner = msg.sender;
for (uint256 i = 0; i < length;) {
// Check for collisions.
bytes4 method = methodList[i];
if (_plugins[owner][method] != address(0)) {
emit PRBProxyRegistry_PluginMethodCollision(_plugins[owner][method], plugin, method);
revert("PRBProxyRegistry: Plugin method collision");
}
_plugins[owner][method] = plugin;
i++;
}Note: Make sure to define the PRBProxyRegistry_PluginMethodCollision event in the contract.
| └── when the plugin list is not empty | ||
| ├── when the plugin has not been installed before |
There was a problem hiding this comment.
🤖 OpenAI
The comment "when the plugin has not been installed before" is incorrect. It should be "when the plugin has not been installed before and there are no method collisions".
| // SPDX-License-Identifier: MIT | ||
| pragma solidity >=0.8.18; |
There was a problem hiding this comment.
🤖 OpenAI
The SPDX license identifier is missing. Please add the appropriate SPDX license identifier to the contract.
| // SPDX-License-Identifier: MIT | |
| pragma solidity >=0.8.18; | |
| // SPDX-License-Identifier: MIT | |
| pragma solidity >=0.8.18; |
6:
The contract PluginCollider implements the IPRBProxyPlugin interface, but it doesn't define the required function name. Please add the name function to the contract.
| // SPDX-License-Identifier: MIT | |
| pragma solidity >=0.8.18; | |
| function name() external pure override returns (string memory) { | |
| return "PluginCollider"; | |
| } |
| function methodList() external pure override returns (bytes4[] memory) { | ||
| bytes4[] memory methods = new bytes4[](1); | ||
| methods[0] = this.onAddictionFeesRefunded.selector; | ||
| return methods; |
There was a problem hiding this comment.
🤖 OpenAI
The methodList function returns an array of bytes4 values representing the selectors of the methods provided by the plugin. Currently, it only returns a single selector (this.onAddictionFeesRefunded.selector). If you intend to have multiple methods in this plugin, you should update the methods array accordingly. Otherwise, if there's only one method, you can simplify the code by directly returning the selector without using an array.
| function methodList() external pure override returns (bytes4[] memory) { | |
| bytes4[] memory methods = new bytes4[](1); | |
| methods[0] = this.onAddictionFeesRefunded.selector; | |
| return methods; | |
| 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; | ||
| } |
There was a problem hiding this comment.
🤖 OpenAI
The onAddictionFeesRefunded function has unused parameters loanId and feeAmount. If these parameters are not needed, you can remove them from the function signature to avoid confusion.
| /// @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; | |
| } | |
| function onAddictionFeesRefunded(uint248, int168, uint192, int248) external pure { | |
| // No need to use the parameters | |
| } |
Overall, the changes made to test/mocks/plugins/PluginCollider.sol look good with the suggested modifications.
| // 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] }); |
There was a problem hiding this comment.
🤖 OpenAI
The code is checking that every plugin method has remained uninstalled after uninstalling the plugin. However, the current implementation retrieves the plugin methods using plugins.dummy.methodList(), which assumes that the plugin is still installed. This can lead to incorrect results if the plugin has already been uninstalled. To fix this, you should retrieve the plugin methods from the registry instead of directly from the plugin. Here's the updated code:
// Assert that every plugin method has remained uninstalled.
bytes4[] memory pluginMethods = registry.getPluginMethods(plugins.dummy);
for (uint256 i = 0; i < pluginMethods.length; ++i) {
IPRBProxyPlugin actualPlugin = registry.getPluginByOwner({ owner: users.alice, method: pluginMethods[i] });
// Rest of the code...
}
| } | ||
| } | ||
|
|
||
| modifier whenPluginInstalled() { | ||
| modifier whenPluginInstalledBefore() { | ||
| // Install the dummy plugin. | ||
| registry.installPlugin(plugins.dummy); | ||
| _; | ||
| } |
There was a problem hiding this comment.
🤖 OpenAI
The modifier whenPluginInstalledBefore is used to install the dummy plugin before executing the test case. However, the current implementation installs the plugin using registry.installPlugin(plugins.dummy) but doesn't uninstall it afterward. This means that subsequent test cases may have unexpected behavior due to the plugin being installed. To ensure a clean state for each test case, you should uninstall the plugin at the end of the modifier. Here's the updated code:
modifier whenPluginInstalledBefore() {
// Install the dummy plugin.
registry.installPlugin(plugins.dummy);
_;
// Uninstall the plugin.
registry.uninstallPlugin(plugins.dummy);
}
| } | ||
| } | ||
|
|
||
| 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); |
There was a problem hiding this comment.
🤖 OpenAI
The test case is emitting an UninstallPlugin event before calling registry.uninstallPlugin(plugins.dummy). However, the event emission is not necessary since the uninstallPlugin function itself emits the event. You can remove the unnecessary event emission. Here's the updated code:
function test_UninstallPlugin_Event() external whenCallerHasProxy whenPluginHasMethods whenPluginInstalledBefore {
vm.expectEmit({ emitter: address(registry) });
registry.uninstallPlugin(plugins.dummy);
// Rest of the code...
}
| 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 | ||
| ) | ||
| ); |
There was a problem hiding this comment.
🤖 OpenAI
The new code introduces a test case test_RevertWhen_MethodCollision that checks for method collisions when installing a plugin. It encodes the necessary parameters and expects a revert if a collision is detected. However, the implementation of this test case seems incorrect. The current implementation installs the sablier plugin, then attempts to check for a collision between sablier.onStreamCanceled and collider. However, it should be checking for a collision between sablier.onStreamCanceled and sablier. Additionally, the test case does not handle the scenario where there is no collision. Consider updating the test case to cover both scenarios: one with a collision and one without a collision.
| function test_InstallPlugin() external whenCallerHasProxy whenPluginListNotEmpty whenNoMethodCollision { | ||
| // Install a dummy plugin that has some methods. | ||
| registry.installPlugin(plugins.dummy); | ||
|
|
There was a problem hiding this comment.
🤖 OpenAI
The new code introduces a test case test_InstallPlugin that installs a dummy plugin and asserts that all its methods have been installed. However, the implementation of this test case seems incorrect. The current implementation installs the dummy plugin but does not perform any assertions or verifications. Consider adding assertions to verify that the plugin methods have been installed correctly.
| } | ||
| } | ||
|
|
||
| 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); |
There was a problem hiding this comment.
🤖 OpenAI
The new code introduces a test case test_InstallPlugin_Event that emits an InstallPlugin event and installs a dummy plugin. However, the implementation of this test case seems incorrect. The current implementation emits the event before installing the plugin, which may lead to confusion. Consider moving the emission of the event after installing the plugin to ensure the event accurately reflects the state change.
function test_RevertWhen_MethodCollision() external whenCallerHasProxy whenPluginListNotEmpty {
registry.installPlugin(plugins.sablier);
// Check for collision between sablier.onStreamCanceled and sablier.
vm.expectRevert(
abi.encodeWithSelector(
IPRBProxyRegistry.PRBProxyRegistry_PluginMethodCollision.selector,
plugins.sablier,
plugins.sablier,
plugins.sablier.onStreamCanceled.selector
)
);
// Check for collision between sablier.onStreamCanceled and collider.
vm.expectRevert(
abi.encodeWithSelector(
IPRBProxyRegistry.PRBProxyRegistry_PluginMethodCollision.selector,
plugins.sablier,
plugins.collider,
plugins.sablier.onStreamCanceled.selector
)
);
registry.installPlugin(plugins.collider);
}
function test_InstallPlugin() external whenCallerHasProxy whenPluginListNotEmpty whenNoMethodCollision {
// Install a dummy plugin that has some methods.
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_InstallPlugin_Event() external whenCallerHasProxy whenPluginListNotEmpty whenNoMethodCollision {
registry.installPlugin(plugins.dummy);
vm.expectEmit({ emitter: address(registry) });
emit InstallPlugin({ owner: users.alice, proxy: proxy, plugin: plugins.dummy });
}
Addresses https://github.com/cantinasec/review-sablier2/issues/1.
Summary by OpenAI
Release Notes: