From 7fbf8cd21c03a541122d810b9cfedc606ad16950 Mon Sep 17 00:00:00 2001 From: Iaroslav Mazur Date: Mon, 3 Jul 2023 16:06:24 +0300 Subject: [PATCH 1/3] Changes in test/ resulting from the audit --- test/Base.t.sol | 6 ++-- test/mocks/plugins/PluginReverter.sol | 2 +- test/mocks/targets/TargetPayable.sol | 8 ----- test/mocks/targets/TargetReverter.sol | 2 +- test/proxy/Proxy.t.sol | 2 +- test/proxy/execute/execute.t.sol | 4 +-- test/proxy/execute/execute.tree | 2 +- test/proxy/receive/receive.t.sol | 7 ++-- test/proxy/run-plugin/runPlugin.t.sol | 34 +++++++------------ test/proxy/run-plugin/runPlugin.tree | 3 +- .../deployAndExecuteAndInstallPlugin.t.sol | 15 +++++--- .../deployAndInstallPlugin.t.sol | 14 +++++--- test/registry/deploy-for/deployFor.t.sol | 2 +- .../install-plugin/installPlugin.t.sol | 3 +- .../set-permission/setPermission.t.sol | 18 +++++----- .../set-permission/setPermission.tree | 8 ++--- .../uninstall-plugin/uninstallPlugin.t.sol | 20 +++++++---- .../uninstall-plugin/uninstallPlugin.tree | 4 +-- 18 files changed, 77 insertions(+), 77 deletions(-) delete mode 100644 test/mocks/targets/TargetPayable.sol diff --git a/test/Base.t.sol b/test/Base.t.sol index 73cdf3d..5cee8b5 100644 --- a/test/Base.t.sol +++ b/test/Base.t.sol @@ -127,7 +127,7 @@ abstract contract Base_Test is Assertions, Events, StdCheats, StdUtils { } /// @dev Generates an address by hashing the name, labels the address and funds it with test assets. - function createUser(string memory name) internal returns (address payable addr) { + function createUser(string memory name) private returns (address payable addr) { addr = payable(makeAddr(name)); vm.deal({ account: addr, newBalance: 100 ether }); } @@ -138,7 +138,7 @@ abstract contract Base_Test is Assertions, Events, StdCheats, StdUtils { } /// @dev Conditionally deploy the registry either normally or from a source precompiled with `--via-ir`. - function deployRegistryConditionally() internal { + function deployRegistryConditionally() private { if (!isTestOptimizedProfile()) { registry = new PRBProxyRegistry(); } else { @@ -149,7 +149,7 @@ abstract contract Base_Test is Assertions, Events, StdCheats, StdUtils { } /// @dev Reads the proxy bytecode either normally or from precompiled source. - function getProxyBytecode() internal returns (bytes memory) { + function getProxyBytecode() private returns (bytes memory) { if (!isTestOptimizedProfile()) { return type(PRBProxy).creationCode; } else { diff --git a/test/mocks/plugins/PluginReverter.sol b/test/mocks/plugins/PluginReverter.sol index a9d54bf..c9baa78 100644 --- a/test/mocks/plugins/PluginReverter.sol +++ b/test/mocks/plugins/PluginReverter.sol @@ -13,7 +13,7 @@ contract PluginReverter is IPRBProxyPlugin, TargetReverter { methods[1] = this.withCustomError.selector; methods[2] = this.withRequire.selector; methods[3] = this.withReasonString.selector; - methods[4] = this.dueToNoPayableModifier.selector; + methods[4] = this.dueToNoPayableFunction.selector; return methods; } diff --git a/test/mocks/targets/TargetPayable.sol b/test/mocks/targets/TargetPayable.sol deleted file mode 100644 index 4f63552..0000000 --- a/test/mocks/targets/TargetPayable.sol +++ /dev/null @@ -1,8 +0,0 @@ -// SPDX-License-Identifier: MIT -pragma solidity >=0.8.18 <0.9.0; - -contract TargetPayable { - function revertLackPayableModifier() external payable returns (uint256) { - return 0; - } -} diff --git a/test/mocks/targets/TargetReverter.sol b/test/mocks/targets/TargetReverter.sol index 295e7e7..06a9524 100644 --- a/test/mocks/targets/TargetReverter.sol +++ b/test/mocks/targets/TargetReverter.sol @@ -20,7 +20,7 @@ contract TargetReverter { revert("You shall not pass"); } - function dueToNoPayableModifier() external pure returns (uint256) { + function dueToNoPayableFunction() external pure returns (uint256) { return 0; } } diff --git a/test/proxy/Proxy.t.sol b/test/proxy/Proxy.t.sol index 05d8f1d..e6264ef 100644 --- a/test/proxy/Proxy.t.sol +++ b/test/proxy/Proxy.t.sol @@ -22,6 +22,6 @@ contract Proxy_Test is Base_Test { // Deploy and label the default proxy. proxy = registry.deployFor({ owner: users.alice }); - vm.label({ account: address(proxy), newLabel: "Alice Proxy" }); + vm.label({ account: address(proxy), newLabel: "Alice's Proxy" }); } } diff --git a/test/proxy/execute/execute.t.sol b/test/proxy/execute/execute.t.sol index 37489ab..909ae34 100644 --- a/test/proxy/execute/execute.t.sol +++ b/test/proxy/execute/execute.t.sol @@ -143,13 +143,13 @@ contract Execute_Test is Proxy_Test { proxy.execute(address(targets.reverter), data); } - function test_RevertWhen_Error_NoPayableModifier() + function test_RevertWhen_Error_NoPayableFunction() external whenCallerAuthorized whenTargetContract whenDelegateCallReverts { - bytes memory data = bytes.concat(targets.reverter.dueToNoPayableModifier.selector); + bytes memory data = bytes.concat(targets.reverter.dueToNoPayableFunction.selector); vm.expectRevert(); proxy.execute{ value: 0.1 ether }(address(targets.reverter), data); } diff --git a/test/proxy/execute/execute.tree b/test/proxy/execute/execute.tree index 02ff889..1ad67e3 100644 --- a/test/proxy/execute/execute.tree +++ b/test/proxy/execute/execute.tree @@ -22,7 +22,7 @@ execute.t.sol │ ├── it should revert with a custom error │ ├── it should revert with a require │ ├── it should revert with a reason string - │ └── it should revert due to no payable modifier + │ └── it should revert due to no payable function └── when the delegate call does not revert ├── when Ether is sent │ └── it should return the Ether amount diff --git a/test/proxy/receive/receive.t.sol b/test/proxy/receive/receive.t.sol index 2dc639d..7df595a 100644 --- a/test/proxy/receive/receive.t.sol +++ b/test/proxy/receive/receive.t.sol @@ -4,12 +4,13 @@ pragma solidity >=0.8.19 <=0.9.0; import { Proxy_Test } from "../Proxy.t.sol"; contract Receive_Test is Proxy_Test { + uint256 private value = 1 ether; + function setUp() public virtual override { Proxy_Test.setUp(); } function test_RevertWhen_CallDataNonEmpty() external { - uint256 value = 1 ether; bytes memory data = bytes.concat("0xcafe"); (bool condition,) = address(proxy).call{ value: value }(data); assertFalse(condition); @@ -20,12 +21,12 @@ contract Receive_Test is Proxy_Test { } function test_Receive() external whenCallDataEmpty { - uint256 value = 1 ether; + uint256 previousBalance = address(proxy).balance; (bool condition,) = address(proxy).call{ value: value }(""); assertTrue(condition); uint256 actualBalance = address(proxy).balance; - uint256 expectedBalance = value; + uint256 expectedBalance = previousBalance + value; assertEq(actualBalance, expectedBalance, "proxy balance mismatch"); } } diff --git a/test/proxy/run-plugin/runPlugin.t.sol b/test/proxy/run-plugin/runPlugin.t.sol index 523ce5f..5641738 100644 --- a/test/proxy/run-plugin/runPlugin.t.sol +++ b/test/proxy/run-plugin/runPlugin.t.sol @@ -48,14 +48,14 @@ contract RunPlugin_Test is Proxy_Test { function test_RevertWhen_Panic_DivisionByZero() external whenPluginInstalled whenDelegateCallReverts { registry.installPlugin(plugins.panic); - vm.expectRevert(stdError.arithmeticError); + vm.expectRevert(stdError.divisionError); (bool success,) = address(proxy).call(abi.encodeWithSelector(plugins.panic.divisionByZero.selector)); success; } function test_RevertWhen_Panic_IndexOOB() external whenPluginInstalled whenDelegateCallReverts { registry.installPlugin(plugins.panic); - vm.expectRevert(stdError.arithmeticError); + vm.expectRevert(stdError.indexOOBError); (bool success,) = address(proxy).call(abi.encodeWithSelector(plugins.panic.indexOOB.selector)); success; } @@ -76,7 +76,7 @@ contract RunPlugin_Test is Proxy_Test { function test_RevertWhen_Error_Require() external whenPluginInstalled whenDelegateCallReverts { registry.installPlugin(plugins.reverter); - vm.expectRevert(TargetReverter.SomeError.selector); + vm.expectRevert(); (bool success,) = address(proxy).call(abi.encodeWithSelector(plugins.reverter.withRequire.selector)); success; } @@ -92,7 +92,7 @@ contract RunPlugin_Test is Proxy_Test { _; } - function test_RunPlugin_EtherSent() + function test_RunPlugin_PluginReceivesEther() external whenPluginInstalled whenDelegateCallReverts @@ -126,9 +126,10 @@ contract RunPlugin_Test is Proxy_Test { // Install the plugin and run it. registry.installPlugin(plugins.selfDestructer); - (bool success,) = + (bool success, bytes memory response) = address(proxy).call(abi.encodeWithSelector(plugins.selfDestructer.destroyMe.selector, users.bob)); - success; + assertTrue(success); + assertEq(response.length, 0); // Assert that Bob's balance has increased by the contract's balance. uint256 actualBobBalance = users.bob.balance; @@ -147,20 +148,6 @@ contract RunPlugin_Test is Proxy_Test { whenDelegateCallDoesNotRevert whenNoEtherSent whenPluginDoesNotSelfDestruct - { - registry.installPlugin(plugins.basic); - (, bytes memory actualResponse) = address(proxy).call(abi.encodeWithSelector(plugins.basic.foo.selector)); - bytes memory expectedResponse = abi.encode(bytes("foo")); - assertEq(actualResponse, expectedResponse, "basic.foo response mismatch"); - } - - function test_RunPlugin_Event() - external - whenPluginInstalled - whenDelegateCallReverts - whenDelegateCallDoesNotRevert - whenNoEtherSent - whenPluginDoesNotSelfDestruct { registry.installPlugin(plugins.basic); vm.expectEmit({ emitter: address(proxy) }); @@ -169,7 +156,10 @@ contract RunPlugin_Test is Proxy_Test { data: abi.encodeWithSelector(TargetBasic.foo.selector), response: abi.encode(bytes("foo")) }); - (bool success,) = address(proxy).call(abi.encodeWithSelector(plugins.basic.foo.selector)); - success; + (bool success, bytes memory actualResponse) = + address(proxy).call(abi.encodeWithSelector(plugins.basic.foo.selector)); + assertTrue(success); + bytes memory expectedResponse = abi.encode(bytes("foo")); + assertEq(actualResponse, expectedResponse, "basic.foo response mismatch"); } } diff --git a/test/proxy/run-plugin/runPlugin.tree b/test/proxy/run-plugin/runPlugin.tree index 7945f11..0492bdd 100644 --- a/test/proxy/run-plugin/runPlugin.tree +++ b/test/proxy/run-plugin/runPlugin.tree @@ -23,6 +23,5 @@ runPlugin.t.sol ├── when the plugin self-destructs │ └── it should return an empty response and send the ETH to the SELFDESTRUCT recipient └── when the plugin does not self-destruct - ├── it should run the plugin - └── it should emit a {RunPlugin} event + └── it should run the plugin and emit a {RunPlugin} event diff --git a/test/registry/deploy-and-execute-and-install-plugin/deployAndExecuteAndInstallPlugin.t.sol b/test/registry/deploy-and-execute-and-install-plugin/deployAndExecuteAndInstallPlugin.t.sol index b891f01..5dbe35b 100644 --- a/test/registry/deploy-and-execute-and-install-plugin/deployAndExecuteAndInstallPlugin.t.sol +++ b/test/registry/deploy-and-execute-and-install-plugin/deployAndExecuteAndInstallPlugin.t.sol @@ -58,19 +58,26 @@ contract DeployAndExecuteAndInstallPlugin_Test is Registry_Test { assertEq(actualProxyAddress, expectedProxyAddress, "proxy address mismatch"); } - function testFuzz_DeployAndExecuteAndInstallPlugin_Plugin() external whenOwnerDoesNotHaveProxy { + function testFuzz_DeployAndExecuteAndInstallPlugin_Plugin(address owner) external whenOwnerDoesNotHaveProxy { + changePrank({ msgSender: owner }); + registry.deployAndExecuteAndInstallPlugin(target, data, plugins.basic); bytes4[] memory pluginMethods = plugins.basic.getMethods(); for (uint256 i = 0; i < pluginMethods.length; ++i) { - IPRBProxyPlugin actualPlugin = registry.getPluginByOwner({ owner: users.alice, method: pluginMethods[i] }); + IPRBProxyPlugin actualPlugin = registry.getPluginByOwner({ owner: owner, method: pluginMethods[i] }); IPRBProxyPlugin expectedPlugin = plugins.basic; assertEq(actualPlugin, expectedPlugin, "plugin method not installed"); } } - function testFuzz_DeployAndExecuteAndInstallPlugin_PluginReverseMapping() external whenOwnerDoesNotHaveProxy { + function testFuzz_DeployAndExecuteAndInstallPlugin_PluginReverseMapping(address owner) + external + whenOwnerDoesNotHaveProxy + { + changePrank({ msgSender: owner }); + registry.deployAndExecuteAndInstallPlugin(target, data, plugins.basic); - bytes4[] memory actualMethods = registry.getMethodsByOwner({ owner: users.alice, plugin: plugins.basic }); + bytes4[] memory actualMethods = registry.getMethodsByOwner({ owner: owner, plugin: plugins.basic }); bytes4[] memory expectedMethods = plugins.basic.getMethods(); assertEq(actualMethods, expectedMethods, "methods not saved in reverse mapping"); } diff --git a/test/registry/deploy-and-install-plugin/deployAndInstallPlugin.t.sol b/test/registry/deploy-and-install-plugin/deployAndInstallPlugin.t.sol index d9b7b7e..2758095 100644 --- a/test/registry/deploy-and-install-plugin/deployAndInstallPlugin.t.sol +++ b/test/registry/deploy-and-install-plugin/deployAndInstallPlugin.t.sol @@ -17,7 +17,7 @@ contract DeployAndInstallPlugin_Test is Registry_Test { vm.expectRevert( abi.encodeWithSelector(IPRBProxyRegistry.PRBProxyRegistry_OwnerHasProxy.selector, users.alice, proxy) ); - registry.deploy(); + registry.deployAndInstallPlugin(plugins.basic); } modifier whenOwnerDoesNotHaveProxy() { @@ -48,19 +48,23 @@ contract DeployAndInstallPlugin_Test is Registry_Test { assertEq(actualProxyAddress, expectedProxyAddress, "proxy address mismatch"); } - function testFuzz_DeployAndInstallPlugin_Plugin() external whenOwnerDoesNotHaveProxy { + function testFuzz_DeployAndInstallPlugin_Plugin(address owner) external whenOwnerDoesNotHaveProxy { + changePrank({ msgSender: owner }); registry.deployAndInstallPlugin(plugins.basic); + bytes4[] memory pluginMethods = plugins.basic.getMethods(); for (uint256 i = 0; i < pluginMethods.length; ++i) { - IPRBProxyPlugin actualPlugin = registry.getPluginByOwner({ owner: users.alice, method: pluginMethods[i] }); + IPRBProxyPlugin actualPlugin = registry.getPluginByOwner({ owner: owner, method: pluginMethods[i] }); IPRBProxyPlugin expectedPlugin = plugins.basic; assertEq(actualPlugin, expectedPlugin, "plugin method not installed"); } } - function testFuzz_DeployAndInstallPlugin_PluginReverseMapping() external whenOwnerDoesNotHaveProxy { + function testFuzz_DeployAndInstallPlugin_PluginReverseMapping(address owner) external whenOwnerDoesNotHaveProxy { + changePrank({ msgSender: owner }); registry.deployAndInstallPlugin(plugins.basic); - bytes4[] memory actualMethods = registry.getMethodsByOwner({ owner: users.alice, plugin: plugins.basic }); + + bytes4[] memory actualMethods = registry.getMethodsByOwner({ owner: owner, plugin: plugins.basic }); bytes4[] memory expectedMethods = plugins.basic.getMethods(); assertEq(actualMethods, expectedMethods, "methods not saved in reverse mapping"); } diff --git a/test/registry/deploy-for/deployFor.t.sol b/test/registry/deploy-for/deployFor.t.sol index b59373d..dc45185 100644 --- a/test/registry/deploy-for/deployFor.t.sol +++ b/test/registry/deploy-for/deployFor.t.sol @@ -12,7 +12,7 @@ contract DeployFor_Test is Registry_Test { } function test_RevertWhen_OwnerHasProxy() external { - IPRBProxy proxy = registry.deployFor({ owner: users.alice }); + IPRBProxy proxy = registry.deploy(); vm.expectRevert( abi.encodeWithSelector(IPRBProxyRegistry.PRBProxyRegistry_OwnerHasProxy.selector, users.alice, proxy) ); diff --git a/test/registry/install-plugin/installPlugin.t.sol b/test/registry/install-plugin/installPlugin.t.sol index 714802b..c0d67c8 100644 --- a/test/registry/install-plugin/installPlugin.t.sol +++ b/test/registry/install-plugin/installPlugin.t.sol @@ -9,9 +9,8 @@ import { Registry_Test } from "../Registry.t.sol"; contract InstallPlugin_Test is Registry_Test { function test_RevertWhen_CallerDoesNotHaveProxy() external { vm.expectRevert( - abi.encodeWithSelector(IPRBProxyRegistry.PRBProxyRegistry_CallerDoesNotHaveProxy.selector, users.bob) + abi.encodeWithSelector(IPRBProxyRegistry.PRBProxyRegistry_CallerDoesNotHaveProxy.selector, users.alice) ); - changePrank({ msgSender: users.bob }); registry.installPlugin(plugins.empty); } diff --git a/test/registry/set-permission/setPermission.t.sol b/test/registry/set-permission/setPermission.t.sol index 8d10134..b64e34d 100644 --- a/test/registry/set-permission/setPermission.t.sol +++ b/test/registry/set-permission/setPermission.t.sol @@ -8,9 +8,8 @@ import { Registry_Test } from "../Registry.t.sol"; contract SetPermission_Test is Registry_Test { function test_RevertWhen_CallerDoesNotHaveProxy() external { vm.expectRevert( - abi.encodeWithSelector(IPRBProxyRegistry.PRBProxyRegistry_CallerDoesNotHaveProxy.selector, users.bob) + abi.encodeWithSelector(IPRBProxyRegistry.PRBProxyRegistry_CallerDoesNotHaveProxy.selector, users.alice) ); - changePrank({ msgSender: users.bob }); registry.setPermission({ envoy: users.envoy, target: address(targets.basic), permission: true }); } @@ -19,26 +18,26 @@ contract SetPermission_Test is Registry_Test { _; } - function test_SetPermission_PermissionNotSet() external whenCallerHasProxy { + function test_SetPermission_PermissionNotSetPreviously() external whenCallerHasProxy { registry.setPermission({ envoy: users.envoy, target: address(targets.basic), permission: true }); bool permission = registry.getPermissionByOwner({ owner: users.alice, envoy: users.envoy, target: address(targets.basic) }); assertTrue(permission, "permission mismatch"); } - modifier whenPermissionSet() { + modifier whenPermissionSetPreviously() { registry.setPermission({ envoy: users.envoy, target: address(targets.basic), permission: true }); _; } - function test_SetPermission_ResetPermission() external whenCallerHasProxy whenPermissionSet { + function test_SetPermission_ResetPermission() external whenCallerHasProxy whenPermissionSetPreviously { registry.setPermission({ envoy: users.envoy, target: address(targets.basic), permission: true }); bool permission = registry.getPermissionByOwner({ owner: users.alice, envoy: users.envoy, target: address(targets.basic) }); assertTrue(permission, "permission mismatch"); } - function test_SetPermission_ResetPermission_Event() external whenCallerHasProxy whenPermissionSet { + function test_SetPermission_ResetPermission_Event() external whenCallerHasProxy whenPermissionSetPreviously { vm.expectEmit({ emitter: address(registry) }); emit SetPermission({ owner: users.alice, @@ -50,11 +49,14 @@ contract SetPermission_Test is Registry_Test { registry.setPermission({ envoy: users.envoy, target: address(targets.basic), permission: true }); } - function test_SetPermission_UnsetPermission() external whenCallerHasProxy whenPermissionSet { + function test_SetPermission_UnsetPermission() external whenCallerHasProxy whenPermissionSetPreviously { registry.setPermission({ envoy: users.envoy, target: address(targets.basic), permission: false }); + bool permission = + registry.getPermissionByOwner({ owner: users.alice, envoy: users.envoy, target: address(targets.basic) }); + assertFalse(permission, "permission mismatch"); } - function test_SetPermission_UnsetPermission_Event() external whenCallerHasProxy whenPermissionSet { + function test_SetPermission_UnsetPermission_Event() external whenCallerHasProxy whenPermissionSetPreviously { vm.expectEmit({ emitter: address(registry) }); emit SetPermission({ owner: users.alice, diff --git a/test/registry/set-permission/setPermission.tree b/test/registry/set-permission/setPermission.tree index aceaf34..d0feecb 100644 --- a/test/registry/set-permission/setPermission.tree +++ b/test/registry/set-permission/setPermission.tree @@ -1,10 +1,10 @@ setPermission.t.sol -├── when the caller is not the owner +├── when the caller doesn't have a proxy │ └── it should revert -└── when the caller is the owner - ├── when the permission has not been set +└── when the caller has a proxy + ├── when the permission has not been set previously │ └── it should set the permission - └── when the permission has been set + └── when the permission has been set previously ├── when resetting the permission │ ├── it should reset the permission │ └── it should emit a {SetPermission} event diff --git a/test/registry/uninstall-plugin/uninstallPlugin.t.sol b/test/registry/uninstall-plugin/uninstallPlugin.t.sol index 090cbde..e12a349 100644 --- a/test/registry/uninstall-plugin/uninstallPlugin.t.sol +++ b/test/registry/uninstall-plugin/uninstallPlugin.t.sol @@ -9,9 +9,8 @@ import { Registry_Test } from "../Registry.t.sol"; contract UninstallPlugin_Test is Registry_Test { function test_RevertWhen_CallerDoesNotHaveProxy() external { vm.expectRevert( - abi.encodeWithSelector(IPRBProxyRegistry.PRBProxyRegistry_CallerDoesNotHaveProxy.selector, users.bob) + abi.encodeWithSelector(IPRBProxyRegistry.PRBProxyRegistry_CallerDoesNotHaveProxy.selector, users.alice) ); - changePrank({ msgSender: users.bob }); registry.uninstallPlugin(plugins.empty); } @@ -21,10 +20,13 @@ contract UninstallPlugin_Test is Registry_Test { } function test_RevertWhen_PluginUnknown() external whenCallerHasProxy { + // Assert that every plugin method is uninstalled. + checkThatPluginMethodsArentInstalled(users.alice, plugins.basic); + vm.expectRevert( - abi.encodeWithSelector(IPRBProxyRegistry.PRBProxyRegistry_PluginUnknown.selector, plugins.empty) + abi.encodeWithSelector(IPRBProxyRegistry.PRBProxyRegistry_PluginUnknown.selector, plugins.basic) ); - registry.uninstallPlugin(plugins.empty); + registry.uninstallPlugin(plugins.basic); } modifier whenPluginKnown() { @@ -37,11 +39,15 @@ contract UninstallPlugin_Test is Registry_Test { registry.uninstallPlugin(plugins.basic); // Assert that every plugin method has been uninstalled. - bytes4[] memory pluginMethods = plugins.basic.getMethods(); + checkThatPluginMethodsArentInstalled(users.alice, plugins.basic); + } + + function checkThatPluginMethodsArentInstalled(address owner, IPRBProxyPlugin plugin) private { + bytes4[] memory pluginMethods = plugin.getMethods(); for (uint256 i = 0; i < pluginMethods.length; ++i) { - IPRBProxyPlugin actualPlugin = registry.getPluginByOwner({ owner: users.alice, method: pluginMethods[i] }); + IPRBProxyPlugin actualPlugin = registry.getPluginByOwner({ owner: owner, method: pluginMethods[i] }); IPRBProxyPlugin expectedPlugin = IPRBProxyPlugin(address(0)); - assertEq(actualPlugin, expectedPlugin, "plugin method still installed"); + assertEq(actualPlugin, expectedPlugin, "plugin method installed"); } } diff --git a/test/registry/uninstall-plugin/uninstallPlugin.tree b/test/registry/uninstall-plugin/uninstallPlugin.tree index 00d6242..8a76da3 100644 --- a/test/registry/uninstall-plugin/uninstallPlugin.tree +++ b/test/registry/uninstall-plugin/uninstallPlugin.tree @@ -1,7 +1,7 @@ uninstallPlugin.t.sol -├── when the caller does not have a proxy +├── when the caller doesn't own a proxy │ └── it should revert -└── when the caller has a proxy +└── when the caller owns a proxy ├── when the plugin is unknown │ └── it should revert └── when the plugin is known From 71aa9e42bf42ea20e66de7799eadb9c24f101799 Mon Sep 17 00:00:00 2001 From: Paul Razvan Berg Date: Wed, 5 Jul 2023 14:59:50 +0300 Subject: [PATCH 2/3] test: polish Iaro's changes --- test/Base.t.sol | 6 ++--- test/mocks/plugins/PluginReverter.sol | 2 +- test/mocks/targets/TargetReverter.sol | 2 +- test/proxy/execute/execute.t.sol | 4 +-- test/proxy/execute/execute.tree | 6 ++--- test/proxy/receive/receive.t.sol | 2 +- test/proxy/run-plugin/runPlugin.t.sol | 27 ++++++++++++++----- test/proxy/run-plugin/runPlugin.tree | 9 ++++--- .../install-plugin/installPlugin.t.sol | 3 ++- .../set-permission/setPermission.t.sol | 3 ++- .../uninstall-plugin/uninstallPlugin.t.sol | 20 +++++--------- 11 files changed, 47 insertions(+), 37 deletions(-) diff --git a/test/Base.t.sol b/test/Base.t.sol index 5cee8b5..73cdf3d 100644 --- a/test/Base.t.sol +++ b/test/Base.t.sol @@ -127,7 +127,7 @@ abstract contract Base_Test is Assertions, Events, StdCheats, StdUtils { } /// @dev Generates an address by hashing the name, labels the address and funds it with test assets. - function createUser(string memory name) private returns (address payable addr) { + function createUser(string memory name) internal returns (address payable addr) { addr = payable(makeAddr(name)); vm.deal({ account: addr, newBalance: 100 ether }); } @@ -138,7 +138,7 @@ abstract contract Base_Test is Assertions, Events, StdCheats, StdUtils { } /// @dev Conditionally deploy the registry either normally or from a source precompiled with `--via-ir`. - function deployRegistryConditionally() private { + function deployRegistryConditionally() internal { if (!isTestOptimizedProfile()) { registry = new PRBProxyRegistry(); } else { @@ -149,7 +149,7 @@ abstract contract Base_Test is Assertions, Events, StdCheats, StdUtils { } /// @dev Reads the proxy bytecode either normally or from precompiled source. - function getProxyBytecode() private returns (bytes memory) { + function getProxyBytecode() internal returns (bytes memory) { if (!isTestOptimizedProfile()) { return type(PRBProxy).creationCode; } else { diff --git a/test/mocks/plugins/PluginReverter.sol b/test/mocks/plugins/PluginReverter.sol index c9baa78..0b4adcf 100644 --- a/test/mocks/plugins/PluginReverter.sol +++ b/test/mocks/plugins/PluginReverter.sol @@ -13,7 +13,7 @@ contract PluginReverter is IPRBProxyPlugin, TargetReverter { methods[1] = this.withCustomError.selector; methods[2] = this.withRequire.selector; methods[3] = this.withReasonString.selector; - methods[4] = this.dueToNoPayableFunction.selector; + methods[4] = this.notPayable.selector; return methods; } diff --git a/test/mocks/targets/TargetReverter.sol b/test/mocks/targets/TargetReverter.sol index 06a9524..3ff7a4a 100644 --- a/test/mocks/targets/TargetReverter.sol +++ b/test/mocks/targets/TargetReverter.sol @@ -20,7 +20,7 @@ contract TargetReverter { revert("You shall not pass"); } - function dueToNoPayableFunction() external pure returns (uint256) { + function notPayable() external pure returns (uint256) { return 0; } } diff --git a/test/proxy/execute/execute.t.sol b/test/proxy/execute/execute.t.sol index 909ae34..caee1e8 100644 --- a/test/proxy/execute/execute.t.sol +++ b/test/proxy/execute/execute.t.sol @@ -143,13 +143,13 @@ contract Execute_Test is Proxy_Test { proxy.execute(address(targets.reverter), data); } - function test_RevertWhen_Error_NoPayableFunction() + function test_RevertWhen_Error_NotPayable() external whenCallerAuthorized whenTargetContract whenDelegateCallReverts { - bytes memory data = bytes.concat(targets.reverter.dueToNoPayableFunction.selector); + bytes memory data = bytes.concat(targets.reverter.notPayable.selector); vm.expectRevert(); proxy.execute{ value: 0.1 ether }(address(targets.reverter), data); } diff --git a/test/proxy/execute/execute.tree b/test/proxy/execute/execute.tree index 1ad67e3..ce60302 100644 --- a/test/proxy/execute/execute.tree +++ b/test/proxy/execute/execute.tree @@ -22,11 +22,11 @@ execute.t.sol │ ├── it should revert with a custom error │ ├── it should revert with a require │ ├── it should revert with a reason string - │ └── it should revert due to no payable function + │ └── it should revert when sending ETH to a non-payable function └── when the delegate call does not revert - ├── when Ether is sent + ├── when ETH is sent │ └── it should return the Ether amount - └── when no Ether is sent + └── when no ETH is sent ├── when the target self-destructs │ └── it should return an empty response and send the ETH to the SELFDESTRUCT recipient └── when the target does not self-destruct diff --git a/test/proxy/receive/receive.t.sol b/test/proxy/receive/receive.t.sol index 7df595a..f93e014 100644 --- a/test/proxy/receive/receive.t.sol +++ b/test/proxy/receive/receive.t.sol @@ -4,7 +4,7 @@ pragma solidity >=0.8.19 <=0.9.0; import { Proxy_Test } from "../Proxy.t.sol"; contract Receive_Test is Proxy_Test { - uint256 private value = 1 ether; + uint256 internal value = 1 ether; function setUp() public virtual override { Proxy_Test.setUp(); diff --git a/test/proxy/run-plugin/runPlugin.t.sol b/test/proxy/run-plugin/runPlugin.t.sol index 5641738..80e64bd 100644 --- a/test/proxy/run-plugin/runPlugin.t.sol +++ b/test/proxy/run-plugin/runPlugin.t.sol @@ -128,8 +128,8 @@ contract RunPlugin_Test is Proxy_Test { registry.installPlugin(plugins.selfDestructer); (bool success, bytes memory response) = address(proxy).call(abi.encodeWithSelector(plugins.selfDestructer.destroyMe.selector, users.bob)); - assertTrue(success); - assertEq(response.length, 0); + assertTrue(success, "selfDestructer.destroyMe failed"); + assertEq(response.length, 0, "selfDestructer.destroyMe response length mismatch"); // Assert that Bob's balance has increased by the contract's balance. uint256 actualBobBalance = users.bob.balance; @@ -148,6 +148,22 @@ contract RunPlugin_Test is Proxy_Test { whenDelegateCallDoesNotRevert whenNoEtherSent whenPluginDoesNotSelfDestruct + { + registry.installPlugin(plugins.basic); + (bool success, bytes memory actualResponse) = + address(proxy).call(abi.encodeWithSelector(plugins.basic.foo.selector)); + bytes memory expectedResponse = abi.encode(bytes("foo")); + assertTrue(success, "basic.foo failed"); + assertEq(actualResponse, expectedResponse, "basic.foo response mismatch"); + } + + function test_RunPlugin_Event() + external + whenPluginInstalled + whenDelegateCallReverts + whenDelegateCallDoesNotRevert + whenNoEtherSent + whenPluginDoesNotSelfDestruct { registry.installPlugin(plugins.basic); vm.expectEmit({ emitter: address(proxy) }); @@ -156,10 +172,7 @@ contract RunPlugin_Test is Proxy_Test { data: abi.encodeWithSelector(TargetBasic.foo.selector), response: abi.encode(bytes("foo")) }); - (bool success, bytes memory actualResponse) = - address(proxy).call(abi.encodeWithSelector(plugins.basic.foo.selector)); - assertTrue(success); - bytes memory expectedResponse = abi.encode(bytes("foo")); - assertEq(actualResponse, expectedResponse, "basic.foo response mismatch"); + (bool success,) = address(proxy).call(abi.encodeWithSelector(plugins.basic.foo.selector)); + success; } } diff --git a/test/proxy/run-plugin/runPlugin.tree b/test/proxy/run-plugin/runPlugin.tree index 0492bdd..690dc11 100644 --- a/test/proxy/run-plugin/runPlugin.tree +++ b/test/proxy/run-plugin/runPlugin.tree @@ -17,11 +17,12 @@ runPlugin.t.sol │ ├── it should revert with a require │ └── it should revert with a reason string └── when the delegate call does not revert - ├── when Ether is sent - │ └── it should return the Ether amount - └── when no Ether is sent + ├── when the plugin receives ETH + │ └── it should return the ETH amount + └── when the plugin does not receive ETH ├── when the plugin self-destructs │ └── it should return an empty response and send the ETH to the SELFDESTRUCT recipient └── when the plugin does not self-destruct - └── it should run the plugin and emit a {RunPlugin} event + ├── it should run the plugin + └── it should emit a {RunPlugin} event diff --git a/test/registry/install-plugin/installPlugin.t.sol b/test/registry/install-plugin/installPlugin.t.sol index c0d67c8..714802b 100644 --- a/test/registry/install-plugin/installPlugin.t.sol +++ b/test/registry/install-plugin/installPlugin.t.sol @@ -9,8 +9,9 @@ import { Registry_Test } from "../Registry.t.sol"; contract InstallPlugin_Test is Registry_Test { function test_RevertWhen_CallerDoesNotHaveProxy() external { vm.expectRevert( - abi.encodeWithSelector(IPRBProxyRegistry.PRBProxyRegistry_CallerDoesNotHaveProxy.selector, users.alice) + abi.encodeWithSelector(IPRBProxyRegistry.PRBProxyRegistry_CallerDoesNotHaveProxy.selector, users.bob) ); + changePrank({ msgSender: users.bob }); registry.installPlugin(plugins.empty); } diff --git a/test/registry/set-permission/setPermission.t.sol b/test/registry/set-permission/setPermission.t.sol index b64e34d..df80db5 100644 --- a/test/registry/set-permission/setPermission.t.sol +++ b/test/registry/set-permission/setPermission.t.sol @@ -8,8 +8,9 @@ import { Registry_Test } from "../Registry.t.sol"; contract SetPermission_Test is Registry_Test { function test_RevertWhen_CallerDoesNotHaveProxy() external { vm.expectRevert( - abi.encodeWithSelector(IPRBProxyRegistry.PRBProxyRegistry_CallerDoesNotHaveProxy.selector, users.alice) + abi.encodeWithSelector(IPRBProxyRegistry.PRBProxyRegistry_CallerDoesNotHaveProxy.selector, users.bob) ); + changePrank({ msgSender: users.bob }); registry.setPermission({ envoy: users.envoy, target: address(targets.basic), permission: true }); } diff --git a/test/registry/uninstall-plugin/uninstallPlugin.t.sol b/test/registry/uninstall-plugin/uninstallPlugin.t.sol index e12a349..090cbde 100644 --- a/test/registry/uninstall-plugin/uninstallPlugin.t.sol +++ b/test/registry/uninstall-plugin/uninstallPlugin.t.sol @@ -9,8 +9,9 @@ import { Registry_Test } from "../Registry.t.sol"; contract UninstallPlugin_Test is Registry_Test { function test_RevertWhen_CallerDoesNotHaveProxy() external { vm.expectRevert( - abi.encodeWithSelector(IPRBProxyRegistry.PRBProxyRegistry_CallerDoesNotHaveProxy.selector, users.alice) + abi.encodeWithSelector(IPRBProxyRegistry.PRBProxyRegistry_CallerDoesNotHaveProxy.selector, users.bob) ); + changePrank({ msgSender: users.bob }); registry.uninstallPlugin(plugins.empty); } @@ -20,13 +21,10 @@ contract UninstallPlugin_Test is Registry_Test { } function test_RevertWhen_PluginUnknown() external whenCallerHasProxy { - // Assert that every plugin method is uninstalled. - checkThatPluginMethodsArentInstalled(users.alice, plugins.basic); - vm.expectRevert( - abi.encodeWithSelector(IPRBProxyRegistry.PRBProxyRegistry_PluginUnknown.selector, plugins.basic) + abi.encodeWithSelector(IPRBProxyRegistry.PRBProxyRegistry_PluginUnknown.selector, plugins.empty) ); - registry.uninstallPlugin(plugins.basic); + registry.uninstallPlugin(plugins.empty); } modifier whenPluginKnown() { @@ -39,15 +37,11 @@ contract UninstallPlugin_Test is Registry_Test { registry.uninstallPlugin(plugins.basic); // Assert that every plugin method has been uninstalled. - checkThatPluginMethodsArentInstalled(users.alice, plugins.basic); - } - - function checkThatPluginMethodsArentInstalled(address owner, IPRBProxyPlugin plugin) private { - bytes4[] memory pluginMethods = plugin.getMethods(); + bytes4[] memory pluginMethods = plugins.basic.getMethods(); for (uint256 i = 0; i < pluginMethods.length; ++i) { - IPRBProxyPlugin actualPlugin = registry.getPluginByOwner({ owner: owner, method: pluginMethods[i] }); + IPRBProxyPlugin actualPlugin = registry.getPluginByOwner({ owner: users.alice, method: pluginMethods[i] }); IPRBProxyPlugin expectedPlugin = IPRBProxyPlugin(address(0)); - assertEq(actualPlugin, expectedPlugin, "plugin method installed"); + assertEq(actualPlugin, expectedPlugin, "plugin method still installed"); } } From 2d678c459627522af635cd404476c01208d4c4a8 Mon Sep 17 00:00:00 2001 From: Paul Razvan Berg Date: Wed, 5 Jul 2023 15:25:26 +0300 Subject: [PATCH 3/3] test: remove ignored pranks --- test/registry/install-plugin/installPlugin.t.sol | 3 +-- test/registry/set-permission/setPermission.t.sol | 3 +-- test/registry/uninstall-plugin/uninstallPlugin.t.sol | 3 +-- 3 files changed, 3 insertions(+), 6 deletions(-) diff --git a/test/registry/install-plugin/installPlugin.t.sol b/test/registry/install-plugin/installPlugin.t.sol index 714802b..c0d67c8 100644 --- a/test/registry/install-plugin/installPlugin.t.sol +++ b/test/registry/install-plugin/installPlugin.t.sol @@ -9,9 +9,8 @@ import { Registry_Test } from "../Registry.t.sol"; contract InstallPlugin_Test is Registry_Test { function test_RevertWhen_CallerDoesNotHaveProxy() external { vm.expectRevert( - abi.encodeWithSelector(IPRBProxyRegistry.PRBProxyRegistry_CallerDoesNotHaveProxy.selector, users.bob) + abi.encodeWithSelector(IPRBProxyRegistry.PRBProxyRegistry_CallerDoesNotHaveProxy.selector, users.alice) ); - changePrank({ msgSender: users.bob }); registry.installPlugin(plugins.empty); } diff --git a/test/registry/set-permission/setPermission.t.sol b/test/registry/set-permission/setPermission.t.sol index df80db5..b64e34d 100644 --- a/test/registry/set-permission/setPermission.t.sol +++ b/test/registry/set-permission/setPermission.t.sol @@ -8,9 +8,8 @@ import { Registry_Test } from "../Registry.t.sol"; contract SetPermission_Test is Registry_Test { function test_RevertWhen_CallerDoesNotHaveProxy() external { vm.expectRevert( - abi.encodeWithSelector(IPRBProxyRegistry.PRBProxyRegistry_CallerDoesNotHaveProxy.selector, users.bob) + abi.encodeWithSelector(IPRBProxyRegistry.PRBProxyRegistry_CallerDoesNotHaveProxy.selector, users.alice) ); - changePrank({ msgSender: users.bob }); registry.setPermission({ envoy: users.envoy, target: address(targets.basic), permission: true }); } diff --git a/test/registry/uninstall-plugin/uninstallPlugin.t.sol b/test/registry/uninstall-plugin/uninstallPlugin.t.sol index 090cbde..5fa200d 100644 --- a/test/registry/uninstall-plugin/uninstallPlugin.t.sol +++ b/test/registry/uninstall-plugin/uninstallPlugin.t.sol @@ -9,9 +9,8 @@ import { Registry_Test } from "../Registry.t.sol"; contract UninstallPlugin_Test is Registry_Test { function test_RevertWhen_CallerDoesNotHaveProxy() external { vm.expectRevert( - abi.encodeWithSelector(IPRBProxyRegistry.PRBProxyRegistry_CallerDoesNotHaveProxy.selector, users.bob) + abi.encodeWithSelector(IPRBProxyRegistry.PRBProxyRegistry_CallerDoesNotHaveProxy.selector, users.alice) ); - changePrank({ msgSender: users.bob }); registry.uninstallPlugin(plugins.empty); }