fix: fix selector read in "execute" function

chore: add comment for what is error code "5159" test: add "TargetDummyWithFallback" test target test: add test with fudged calldata sent to "execute" test: use named args in "execute" tests
PaulRBerg · Feb 17, 2023 · 7594489 · 7594489
1 parent 7daea25
commit 7594489
Show file tree

Hide file tree

Showing 6 changed files with 88 additions and 11 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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