From 22074864458cf2bc092b705c4bb5dd37341b7fef Mon Sep 17 00:00:00 2001 From: Paul Razvan Berg Date: Thu, 23 Feb 2023 15:06:57 +0200 Subject: [PATCH] refactor: simplify envoy system docs: say "provided" instead of "given" docs: document "TransferOwnership" event in NatSpec refactor: order events alphabetically closes #73 --- src/PRBProxy.sol | 33 ++----- src/PRBProxyStorage.sol | 3 +- src/interfaces/IPRBProxy.sol | 34 +++---- src/interfaces/IPRBProxyFactory.sol | 8 +- src/interfaces/IPRBProxyRegistry.sol | 12 +-- test/proxy/PRBProxy.t.sol | 2 +- test/proxy/execute/execute.t.sol | 93 +------------------ test/proxy/execute/execute.tree | 10 +- test/proxy/set-permission/setPermission.t.sol | 75 +++------------ 9 files changed, 57 insertions(+), 213 deletions(-) diff --git a/src/PRBProxy.sol b/src/PRBProxy.sol index d029199..4db6d2d 100644 --- a/src/PRBProxy.sol +++ b/src/PRBProxy.sol @@ -83,12 +83,8 @@ contract PRBProxy is IPRBProxy, PRBProxyStorage { //////////////////////////////////////////////////////////////////////////*/ /// @inheritdoc IPRBProxy - function getPermission( - address envoy, - address target, - bytes4 selector - ) external view override returns (bool permission) { - permission = permissions[envoy][target][selector]; + function getPermission(address envoy, address target) external view override returns (bool permission) { + permission = permissions[envoy][target]; } /// @inheritdoc IPRBProxy @@ -104,14 +100,8 @@ contract PRBProxy is IPRBProxy, PRBProxyStorage { function execute(address target, bytes calldata data) external payable override returns (bytes memory response) { // Check that the caller is either the owner or an envoy with permission. if (owner != msg.sender) { - bytes4 selector = bytes4(data[:4]); - if (!permissions[msg.sender][target][selector]) { - revert PRBProxy_ExecutionUnauthorized({ - owner: owner, - caller: msg.sender, - target: target, - selector: selector - }); + if (!permissions[msg.sender][target]) { + revert PRBProxy_ExecutionUnauthorized({ owner: owner, caller: msg.sender, target: target }); } } @@ -171,14 +161,9 @@ contract PRBProxy is IPRBProxy, PRBProxyStorage { } /// @inheritdoc IPRBProxy - function setPermission( - address envoy, - address target, - bytes4 selector, - bool permission - ) external override onlyOwner { - permissions[envoy][target][selector] = permission; - emit SetPermission(envoy, target, selector, permission); + function setPermission(address envoy, address target, bool permission) external override onlyOwner { + permissions[envoy][target] = permission; + emit SetPermission(envoy, target, permission); } /// @inheritdoc IPRBProxy @@ -225,7 +210,7 @@ contract PRBProxy is IPRBProxy, PRBProxyStorage { INTERNAL NON-CONSTANT FUNCTIONS //////////////////////////////////////////////////////////////////////////*/ - /// @notice Performs a DELEGATECALL to the given address with the given data. + /// @notice Performs a DELEGATECALL to the provided address with the provided data. /// @dev Shared logic between the {execute} and the {fallback} functions. function _safeDelegateCall(address to, bytes memory data) internal returns (bool success, bytes memory response) { // Save the owner address in memory. This variable cannot be modified during the DELEGATECALL. @@ -234,7 +219,7 @@ contract PRBProxy is IPRBProxy, PRBProxyStorage { // Reserve some gas to ensure that the function has enough to finish the execution. uint256 stipend = gasleft() - minGasReserve; - // Delegate call to the given contract. + // Delegate call to the provided contract. (success, response) = to.delegatecall{ gas: stipend }(data); // Check that the owner has not been changed. diff --git a/src/PRBProxyStorage.sol b/src/PRBProxyStorage.sol index b262c01..6a40ede 100644 --- a/src/PRBProxyStorage.sol +++ b/src/PRBProxyStorage.sol @@ -25,6 +25,5 @@ abstract contract PRBProxyStorage is IPRBProxy { mapping(bytes4 method => IPRBProxyPlugin plugin) internal plugins; /// @dev Maps envoys to target contracts to function selectors to boolean flags. - mapping(address envoy => mapping(address target => mapping(bytes4 selector => bool permission))) - internal permissions; + mapping(address envoy => mapping(address target => bool permission)) internal permissions; } diff --git a/src/interfaces/IPRBProxy.sol b/src/interfaces/IPRBProxy.sol index e12ccb3..c5c6636 100644 --- a/src/interfaces/IPRBProxy.sol +++ b/src/interfaces/IPRBProxy.sol @@ -15,7 +15,7 @@ interface IPRBProxy { error PRBProxy_ExecutionReverted(); /// @notice Emitted when the caller is not the owner. - error PRBProxy_ExecutionUnauthorized(address owner, address caller, address target, bytes4 selector); + error PRBProxy_ExecutionUnauthorized(address owner, address caller, address target); /// @notice Emitted when the caller is not the owner. error PRBProxy_NotOwner(address owner, address caller); @@ -45,42 +45,43 @@ interface IPRBProxy { /// @notice Emitted when a plugin is installed. event InstallPlugin(IPRBProxyPlugin indexed plugin); - /// @notice Emitted when a plugin is run for a given method. + /// @notice Emitted when a plugin is run for a provided method. event RunPlugin(IPRBProxyPlugin indexed plugin, bytes data, bytes response); /// @notice Emitted when the owner sets the permission for an envoy. - event SetPermission(address indexed envoy, address indexed target, bytes4 indexed selector, bool permission); + event SetPermission(address indexed envoy, address indexed target, bool permission); /// @notice Emitted when the owner changes the proxy's owner. event TransferOwnership(address indexed oldOwner, address indexed newOwner); + /// @notice Emitted when a plugin is uninstalled. event UninstallPlugin(IPRBProxyPlugin indexed plugin); /*////////////////////////////////////////////////////////////////////////// PUBLIC CONSTANT FUNCTIONS //////////////////////////////////////////////////////////////////////////*/ - /// @notice Returns a boolean flag that indicates whether the envoy has permission to call the given target - /// contract and function selector. - function getPermission(address envoy, address target, bytes4 selector) external view returns (bool permission); + /// @notice Returns a boolean flag that indicates whether the envoy has permission to call the provided target + /// contract. + function getPermission(address envoy, address target) external view returns (bool permission); - /// @notice Returns the address of the plugin installed for the the given method. + /// @notice Returns the address of the plugin installed for the the provided method. /// @dev Returns the zero address if no plugin is installed. /// @param method The signature of the method to make the query for. function getPluginForMethod(bytes4 method) external view returns (IPRBProxyPlugin plugin); - /// @notice The address of the owner account or contract. - function owner() external view returns (address); - /// @notice How much gas to reserve for running the remainder of the "execute" function after the DELEGATECALL. /// @dev This prevents the proxy from becoming unusable if EVM opcode gas costs change in the future. function minGasReserve() external view returns (uint256); + /// @notice The address of the owner account or contract. + function owner() external view returns (address); + /*////////////////////////////////////////////////////////////////////////// PUBLIC NON-CONSTANT FUNCTIONS //////////////////////////////////////////////////////////////////////////*/ - /// @notice Delegate calls to the given target contract by forwarding the data. It then returns the data it + /// @notice Delegate calls to the provided target contract by forwarding the data. It then returns the data it /// gets back, bubbling up any potential revert. /// /// @dev Emits an {Execute} event. @@ -112,23 +113,24 @@ interface IPRBProxy { /// @param plugin The address of the plugin to install. function installPlugin(IPRBProxyPlugin plugin) external; - /// @notice Gives or takes a permission from an envoy to call the given target contract and function selector + /// @notice Gives or takes a permission from an envoy to call the provided target contract and function selector /// on behalf of the owner. /// - /// @dev It is not an error to reset a permission on the same (envoy,target,selector) tuple multiple types. + /// @dev It is not an error to reset a permission on the same (envoy,target) tuple multiple types. /// /// Requirements: /// - The caller must be the owner. /// /// @param envoy The address of the envoy account. /// @param target The address of the target contract. - /// @param selector The 4 byte function selector on the target contract. /// @param permission The boolean permission to set. - function setPermission(address envoy, address target, bytes4 selector, bool permission) external; + function setPermission(address envoy, address target, bool permission) external; /// @notice Transfers the owner of the contract to a new account. /// - /// @dev Requirements: + /// @dev Emits a {TransferOwnership} event. + /// + /// Requirements: /// - The caller must be the owner. /// /// @param newOwner The address of the new owner account. diff --git a/src/interfaces/IPRBProxyFactory.sol b/src/interfaces/IPRBProxyFactory.sol index f01348f..c40451f 100644 --- a/src/interfaces/IPRBProxyFactory.sol +++ b/src/interfaces/IPRBProxyFactory.sol @@ -33,7 +33,7 @@ interface IPRBProxyFactory { /// @param eoa The externally owned account that will own the proxy. function getNextSeed(address eoa) external view returns (bytes32 result); - /// @notice Checks if the given address is a deployed proxy. + /// @notice Checks if the provided address is a deployed proxy. /// @param proxy The address of the proxy to make the query for. function isProxy(IPRBProxy proxy) external view returns (bool result); @@ -46,14 +46,14 @@ interface IPRBProxyFactory { /// @return proxy The address of the newly deployed proxy contract. function deploy() external returns (IPRBProxy proxy); - /// @notice Deploys a new proxy with CREATE2 for the given owner. + /// @notice Deploys a new proxy with CREATE2 for the provided owner. /// @dev Emits a {DeployProxy} event. /// @param owner The owner of the proxy. /// @return proxy The address of the newly deployed proxy contract. function deployFor(address owner) external returns (IPRBProxy proxy); /// @notice Deploys a new proxy with CREATE2 by setting the caller as the owner, and delegate calls to the - /// given target contract by forwarding the data. It returns the data it gets back, bubbling up any potential + /// provided target contract by forwarding the data. It returns the data it gets back, bubbling up any potential /// revert. /// /// @dev Emits a {DeployProxy} and an {Execute} event. @@ -70,7 +70,7 @@ interface IPRBProxyFactory { bytes calldata data ) external returns (IPRBProxy proxy, bytes memory response); - /// @notice Deploys a new proxy with CREATE2 for the given owner, and delegate calls to the given target + /// @notice Deploys a new proxy with CREATE2 for the provided 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. /// /// @dev Emits a {DeployProxy} and an {Execute} event. diff --git a/src/interfaces/IPRBProxyRegistry.sol b/src/interfaces/IPRBProxyRegistry.sol index 7cf9caa..3e23079 100644 --- a/src/interfaces/IPRBProxyRegistry.sol +++ b/src/interfaces/IPRBProxyRegistry.sol @@ -13,7 +13,7 @@ interface IPRBProxyRegistry { CUSTOM ERRORS //////////////////////////////////////////////////////////////////////////*/ - /// @notice Emitted when a proxy already exists for the given owner. + /// @notice Emitted when a proxy already exists for the provided owner. error PRBProxyRegistry_ProxyAlreadyExists(address owner); /*////////////////////////////////////////////////////////////////////////// @@ -23,7 +23,7 @@ interface IPRBProxyRegistry { /// @notice Address of the proxy factory contract. function factory() external view returns (IPRBProxyFactory proxyFactory); - /// @notice Gets the current proxy of the given owner. + /// @notice Gets the current proxy of the provided owner. /// @param owner The address of the owner of the current proxy. function getCurrentProxy(address owner) external view returns (IPRBProxy proxy); @@ -41,7 +41,7 @@ interface IPRBProxyRegistry { /// @return proxy The address of the newly deployed proxy contract. function deploy() external returns (IPRBProxy proxy); - /// @notice Deploys a new proxy via the proxy factory for the given owner. + /// @notice Deploys a new proxy via the proxy factory for the provided owner. /// /// @dev Emits a {DeployProxy} event. /// @@ -53,7 +53,7 @@ interface IPRBProxyRegistry { function deployFor(address owner) external returns (IPRBProxy proxy); /// @notice Deploys a new proxy via the proxy factory by setting the caller as the owner, and delegate calls to the - /// given target contract by forwarding the data. It returns the data it gets back, bubbling up any potential + /// provided target contract by forwarding the data. It returns the data it gets back, bubbling up any potential /// revert. /// /// @dev Emits a {DeployProxy} and an {Execute} event. @@ -70,8 +70,8 @@ interface IPRBProxyRegistry { bytes calldata data ) external returns (IPRBProxy proxy, bytes memory response); - /// @notice Deploys a new proxy via the proxy factor for the given owner, and delegate calls to the given target - /// contract by forwarding the data. It returns the data it gets back, bubbling up any potential revert. + /// @notice Deploys a new proxy via the proxy factor for the provided 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. /// /// @dev Emits a {DeployProxy} and an {Execute} event. /// diff --git a/test/proxy/PRBProxy.t.sol b/test/proxy/PRBProxy.t.sol index 2577bda..2330539 100644 --- a/test/proxy/PRBProxy.t.sol +++ b/test/proxy/PRBProxy.t.sol @@ -15,7 +15,7 @@ contract PRBProxy_Test is Base_Test { event RunPlugin(IPRBProxyPlugin indexed plugin, bytes data, bytes response); - event SetPermission(address indexed envoy, address indexed target, bytes4 indexed selector, bool permission); + event SetPermission(address indexed envoy, address indexed target, bool permission); event TransferOwnership(address indexed oldOwner, address indexed newOwner); diff --git a/test/proxy/execute/execute.t.sol b/test/proxy/execute/execute.t.sol index a662944..a1e7583 100644 --- a/test/proxy/execute/execute.t.sol +++ b/test/proxy/execute/execute.t.sol @@ -23,25 +23,15 @@ contract Execute_Test is PRBProxy_Test { IPRBProxy.PRBProxy_ExecutionUnauthorized.selector, owner, users.eve, - address(targets.dummy), - targets.dummy.foo.selector + address(targets.dummy) ) ); proxy.execute(address(targets.dummy), data); } - modifier callerHasPermission() { - _; - } - /// @dev it should revert. - function test_RevertWhen_PermissionDifferentTarget() external callerUnauthorized callerHasPermission { - proxy.setPermission({ - envoy: users.envoy, - target: address(targets.echo), - selector: targets.dummy.foo.selector, - permission: true - }); + function test_RevertWhen_PermissionDifferentTarget() external callerUnauthorized { + proxy.setPermission({ envoy: users.envoy, target: address(targets.echo), permission: true }); changePrank(users.envoy); bytes memory data = bytes.concat(targets.dummy.foo.selector); @@ -50,77 +40,12 @@ contract Execute_Test is PRBProxy_Test { IPRBProxy.PRBProxy_ExecutionUnauthorized.selector, owner, users.envoy, - address(targets.dummy), - targets.dummy.foo.selector + address(targets.dummy) ) ); proxy.execute(address(targets.dummy), data); } - modifier callerPermissionSameTarget() { - _; - } - - /// @dev it should revert. - function test_RevertWhen_TargetDoesNotHaveFallbackFunction() - external - callerUnauthorized - callerHasPermission - callerPermissionSameTarget - { - proxy.setPermission({ - envoy: users.envoy, - target: address(targets.dummy), - selector: targets.dummy.foo.selector, - permission: true - }); - changePrank(users.envoy); - - bytes memory data = bytes.concat(targets.dummy.bar.selector); - vm.expectRevert( - abi.encodeWithSelector( - IPRBProxy.PRBProxy_ExecutionUnauthorized.selector, - owner, - users.envoy, - address(targets.dummy), - targets.dummy.bar.selector - ) - ); - proxy.execute(address(targets.dummy), data); - } - - /// @dev it should revert. - function test_RevertWhen_TargetHasFallbackFunction() - external - callerUnauthorized - callerHasPermission - callerPermissionSameTarget - { - proxy.setPermission({ - envoy: users.envoy, - target: address(targets.dummyWithFallback), - selector: targets.dummyWithFallback.foo.selector, - permission: true - }); - changePrank(users.envoy); - - // Fudge the calldata such that `data` is empty, but there is additional calldata after it. This will - // attempt to bypass the usual selector checks, and call the fallback function on the target. - bytes memory usualCalldata = abi.encodeWithSelector( - proxy.execute.selector, - address(targets.dummyWithFallback), - new bytes(0) - ); - bytes memory data = abi.encodePacked(usualCalldata, targets.dummyWithFallback.foo.selector); - (bool success, bytes memory response) = address(proxy).call(data); - - // Assert that the call failed. - assertFalse(success); - - // Assert that the call reverted with no response. - assertEq(response.length, 0); - } - modifier callerAuthorized() { _; } @@ -357,15 +282,7 @@ contract Execute_Test is PRBProxy_Test { } modifier targetDoesNotSelfDestruct() { - proxy.setPermission(users.envoy, address(targets.echo), targets.echo.echoAddress.selector, true); - proxy.setPermission(users.envoy, address(targets.echo), targets.echo.echoBytesArray.selector, true); - proxy.setPermission(users.envoy, address(targets.echo), targets.echo.echoBytes32.selector, true); - proxy.setPermission(users.envoy, address(targets.echo), targets.echo.echoMsgValue.selector, true); - proxy.setPermission(users.envoy, address(targets.echo), targets.echo.echoString.selector, true); - proxy.setPermission(users.envoy, address(targets.echo), targets.echo.echoStruct.selector, true); - proxy.setPermission(users.envoy, address(targets.echo), targets.echo.echoUint8.selector, true); - proxy.setPermission(users.envoy, address(targets.echo), targets.echo.echoUint256.selector, true); - proxy.setPermission(users.envoy, address(targets.echo), targets.echo.echoUint256Array.selector, true); + proxy.setPermission({ envoy: users.envoy, target: address(targets.echo), permission: true }); _; } diff --git a/test/proxy/execute/execute.tree b/test/proxy/execute/execute.tree index 1fcced9..073adbb 100644 --- a/test/proxy/execute/execute.tree +++ b/test/proxy/execute/execute.tree @@ -2,14 +2,8 @@ execute.t.sol ├── when the caller is not authorized │ ├── when the caller does not have permission for anything │ │ └── it should revert -│ └── when the caller has permission for something -│ ├── when the caller has permission for a different target -│ │ └── it should revert -│ └── when the caller has permission for the same target but a different function -│ ├── when the target does not have a fallback function -│ │ └── it should revert -│ └── when the target has a fallback function -│ └── it should revert +│ └── when the caller has permission for another target +│ └── it should revert └── when the caller is authorized ├── when the target is not a contract │ └── it should revert diff --git a/test/proxy/set-permission/setPermission.t.sol b/test/proxy/set-permission/setPermission.t.sol index c2a084c..e4100dc 100644 --- a/test/proxy/set-permission/setPermission.t.sol +++ b/test/proxy/set-permission/setPermission.t.sol @@ -14,12 +14,7 @@ contract SetPermission_Test is PRBProxy_Test { // Run the test. vm.expectRevert(abi.encodeWithSelector(IPRBProxy.PRBProxy_NotOwner.selector, owner, caller)); - proxy.setPermission({ - envoy: caller, - target: address(targets.dummy), - selector: TargetDummy.foo.selector, - permission: true - }); + proxy.setPermission({ envoy: caller, target: address(targets.dummy), permission: true }); } modifier callerOwner() { @@ -28,87 +23,39 @@ contract SetPermission_Test is PRBProxy_Test { /// @dev it should set the permission. function test_SetPermission_PermissionNotSet() external callerOwner { - proxy.setPermission({ - envoy: users.envoy, - target: address(targets.dummy), - selector: TargetDummy.foo.selector, - permission: true - }); - bool permission = proxy.getPermission({ - envoy: users.envoy, - target: address(targets.dummy), - selector: TargetDummy.foo.selector - }); + proxy.setPermission({ envoy: users.envoy, target: address(targets.dummy), permission: true }); + bool permission = proxy.getPermission({ envoy: users.envoy, target: address(targets.dummy) }); assertTrue(permission); } modifier permissionSet() { - proxy.setPermission({ - envoy: users.envoy, - target: address(targets.dummy), - selector: TargetDummy.foo.selector, - permission: true - }); + proxy.setPermission({ envoy: users.envoy, target: address(targets.dummy), permission: true }); _; } /// @dev it should do nothing when re-setting the permission. function test_SetPermission_PermissionSet_ResetPermission() external callerOwner permissionSet { - proxy.setPermission({ - envoy: users.envoy, - target: address(targets.dummy), - selector: TargetDummy.foo.selector, - permission: true - }); - bool permission = proxy.getPermission({ - envoy: users.envoy, - target: address(targets.dummy), - selector: TargetDummy.foo.selector - }); + proxy.setPermission({ envoy: users.envoy, target: address(targets.dummy), permission: true }); + bool permission = proxy.getPermission({ envoy: users.envoy, target: address(targets.dummy) }); assertTrue(permission); } /// @dev it should emit a {SetPermission} event. function test_SetPermission_PermissionSet_ResetPermission_Event() external callerOwner permissionSet { expectEmit(); - emit SetPermission({ - envoy: users.envoy, - target: address(targets.dummy), - selector: TargetDummy.foo.selector, - permission: true - }); - proxy.setPermission({ - envoy: users.envoy, - target: address(targets.dummy), - selector: TargetDummy.foo.selector, - permission: true - }); + emit SetPermission({ envoy: users.envoy, target: address(targets.dummy), permission: true }); + proxy.setPermission({ envoy: users.envoy, target: address(targets.dummy), permission: true }); } /// @dev it should unset the permission. function test_SetPermission_PermissionSet_UnsetPermission() external callerOwner permissionSet { - proxy.setPermission({ - envoy: users.envoy, - target: address(targets.dummy), - selector: TargetDummy.foo.selector, - permission: false - }); + proxy.setPermission({ envoy: users.envoy, target: address(targets.dummy), permission: false }); } /// @dev it should emit a {SetPermission} event. function test_SetPermission_PermissionSet_UnsetPermission_Event() external callerOwner permissionSet { expectEmit(); - emit SetPermission({ - envoy: users.envoy, - target: address(targets.dummy), - selector: TargetDummy.foo.selector, - permission: false - }); - proxy.setPermission({ - envoy: users.envoy, - target: address(targets.dummy), - selector: TargetDummy.foo.selector, - permission: false - }); + emit SetPermission({ envoy: users.envoy, target: address(targets.dummy), permission: false }); + proxy.setPermission({ envoy: users.envoy, target: address(targets.dummy), permission: false }); } }