Merge pull request #145 from IaroslavMazur/iaro/test-changes-after-audit

Changes in test/ resulting from the audit

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.notPayable.selector;
 
         return methods;
     }

test/mocks/targets/TargetReverter.sol

@@ -20,7 +20,7 @@ contract TargetReverter {
         revert("You shall not pass");
     }
 
-    function dueToNoPayableModifier() external pure returns (uint256) {
+    function notPayable() external pure returns (uint256) {
         return 0;
     }
 }

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" });
     }
 }

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_NotPayable()
         external
         whenCallerAuthorized
         whenTargetContract
         whenDelegateCallReverts
     {
-        bytes memory data = bytes.concat(targets.reverter.dueToNoPayableModifier.selector);
+        bytes memory data = bytes.concat(targets.reverter.notPayable.selector);
         vm.expectRevert();
         proxy.execute{ value: 0.1 ether }(address(targets.reverter), data);
     }

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 modifier
+         │     └── 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

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 internal 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");
     }
 }

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, "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;
@@ -149,8 +150,10 @@ contract RunPlugin_Test is Proxy_Test {
         whenPluginDoesNotSelfDestruct
     {
         registry.installPlugin(plugins.basic);
-        (, bytes memory actualResponse) = address(proxy).call(abi.encodeWithSelector(plugins.basic.foo.selector));
+        (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");
     }
 

test/proxy/run-plugin/runPlugin.tree

@@ -17,9 +17,9 @@ 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

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");
     }

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");
     }

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)
         );

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);
     }
 

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,

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

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);
     }
 

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