diff --git a/foundry.toml b/foundry.toml index bcf99ad..250305d 100644 --- a/foundry.toml +++ b/foundry.toml @@ -6,7 +6,9 @@ cbor_metadata = false fs_permissions = [{ access = "read", path = "./optimized-out" }] gas_reports = ["PRBProxy", "PRBProxyFactory", "PRBProxyRegistry"] - ignored_error_codes = [5159] + ignored_error_codes = [ + 5159, # ignore selfdestruct warning + ] libs = ["lib"] optimizer = true optimizer_runs = 10_000 diff --git a/src/PRBProxy.sol b/src/PRBProxy.sol index 7b78ead..9f56893 100644 --- a/src/PRBProxy.sol +++ b/src/PRBProxy.sol @@ -123,10 +123,7 @@ contract PRBProxy is IPRBProxy { 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. if (owner != msg.sender) { - bytes4 selector; - assembly { - selector := calldataload(data.offset) - } + bytes4 selector = bytes4(data[:4]); if (!permissions[msg.sender][target][selector]) { revert PRBProxy_ExecutionUnauthorized({ owner: owner, diff --git a/test/helpers/targets/TargetDummyWithFallback.t.sol b/test/helpers/targets/TargetDummyWithFallback.t.sol new file mode 100644 index 0000000..96e9f8d --- /dev/null +++ b/test/helpers/targets/TargetDummyWithFallback.t.sol @@ -0,0 +1,20 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity >=0.8.18; + +contract TargetDummyWithFallback { + event LogFallback(); + + fallback() external payable { + emit LogFallback(); + } + + receive() external payable {} + + function foo() external pure returns (string memory) { + return "foo"; + } + + function bar() external pure returns (string memory) { + return "bar"; + } +} diff --git a/test/prb-proxy/PRBProxy.t.sol b/test/prb-proxy/PRBProxy.t.sol index 0822a68..46ab935 100644 --- a/test/prb-proxy/PRBProxy.t.sol +++ b/test/prb-proxy/PRBProxy.t.sol @@ -14,6 +14,7 @@ import { PluginReverter } from "../helpers/plugins/PluginReverter.t.sol"; import { PluginSelfDestructer } from "../helpers/plugins/PluginSelfDestructer.t.sol"; import { TargetChangeOwner } from "../helpers/targets/TargetChangeOwner.t.sol"; import { TargetDummy } from "../helpers/targets/TargetDummy.t.sol"; +import { TargetDummyWithFallback } from "../helpers/targets/TargetDummyWithFallback.t.sol"; import { TargetEcho } from "../helpers/targets/TargetEcho.t.sol"; import { TargetMinGasReserve } from "../helpers/targets/TargetMinGasReserve.t.sol"; import { TargetPanic } from "../helpers/targets/TargetPanic.t.sol"; @@ -38,6 +39,7 @@ contract PRBProxy_Test is Base_Test { struct Targets { TargetChangeOwner changeOwner; TargetDummy dummy; + TargetDummyWithFallback dummyWithFallback; TargetEcho echo; TargetMinGasReserve minGasReserve; TargetPanic panic; @@ -89,6 +91,7 @@ contract PRBProxy_Test is Base_Test { targets = Targets({ changeOwner: new TargetChangeOwner(), dummy: new TargetDummy(), + dummyWithFallback: new TargetDummyWithFallback(), echo: new TargetEcho(), minGasReserve: new TargetMinGasReserve(), panic: new TargetPanic(), diff --git a/test/prb-proxy/execute/execute.t.sol b/test/prb-proxy/execute/execute.t.sol index 8a0d9cf..a5b0d62 100644 --- a/test/prb-proxy/execute/execute.t.sol +++ b/test/prb-proxy/execute/execute.t.sol @@ -1,6 +1,7 @@ // SPDX-License-Identifier: UNLICENSED pragma solidity >=0.8.4 <=0.9.0; +import { console2 } from "forge-std/console2.sol"; import { stdError } from "forge-std/StdError.sol"; import { IPRBProxy } from "src/interfaces/IPRBProxy.sol"; @@ -36,7 +37,12 @@ contract Execute_Test is PRBProxy_Test { /// @dev it should revert. function test_RevertWhen_PermissionDifferentTarget() external callerUnauthorized callerHasPermission { - proxy.setPermission(users.envoy, address(targets.echo), targets.dummy.foo.selector, true); + proxy.setPermission({ + envoy: users.envoy, + target: address(targets.echo), + selector: targets.dummy.foo.selector, + permission: true + }); changePrank(users.envoy); bytes memory data = bytes.concat(targets.dummy.foo.selector); @@ -52,24 +58,70 @@ contract Execute_Test is PRBProxy_Test { proxy.execute(address(targets.dummy), data); } + modifier callerPermissionSameTarget() { + _; + } + /// @dev it should revert. - function test_RevertWhen_PermissionDifferentFunction() external callerUnauthorized callerHasPermission { - proxy.setPermission(users.envoy, address(targets.dummy), targets.dummy.bar.selector, true); + 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.foo.selector); + 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.foo.selector + 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() { _; } diff --git a/test/prb-proxy/execute/execute.tree b/test/prb-proxy/execute/execute.tree index 76812c5..5fcc1c8 100644 --- a/test/prb-proxy/execute/execute.tree +++ b/test/prb-proxy/execute/execute.tree @@ -6,7 +6,10 @@ execute.t.sol │ ├── 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 -│ └── it should revert +│ ├── 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 is authorized ├── when the target is not a contract │ └── it should revert