Merge pull request #86 from Bunniapp/master

hooks improvements
flood-protocol · Nov 4, 2024 · d9311d4 · d9311d4
2 parents c989196 + 4164507
commit d9311d4
Show file tree

Hide file tree

Showing 12 changed files with 98 additions and 49 deletions.
diff --git a/foundry.toml b/foundry.toml
@@ -7,7 +7,7 @@
     "permit2/=lib/permit2/",
     "solady/=lib/solady/src/",
   ]
-  solc_version="0.8.23"
+  solc_version="0.8.25"
 
 [profile.ci]
   fuzz_runs=5000

diff --git a/src/EncodedCalls.sol b/src/EncodedCalls.sol
@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: MIT
-pragma solidity 0.8.23;
+pragma solidity ^0.8.23;
 
 // Inheritances
 import {IEncodedCalls} from "./interfaces/IEncodedCalls.sol";

diff --git a/src/FloodPlain.sol b/src/FloodPlain.sol
@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: MIT
-pragma solidity 0.8.23;
+pragma solidity ^0.8.23;
 
 // Inheritances
 import {IFloodPlain} from "./interfaces/IFloodPlain.sol";
@@ -61,7 +61,7 @@ contract FloodPlain is IFloodPlain, EncodedCalls, OnChainOrders, ReentrancyGuard
         if (order.zone != address(0)) if (!(IZone(order.zone).validate(order, msg.sender))) revert ZoneDenied();
 
         // Execute pre hooks.
-        order.preHooks.execute();
+        order.preHooks.execute(orderHash, address(PERMIT2));
 
         // Transfer each offer item to msg.sender using Permit2.
         _permitTransferOffer(order, package.signature, orderHash, msg.sender);
@@ -71,7 +71,7 @@ contract FloodPlain is IFloodPlain, EncodedCalls, OnChainOrders, ReentrancyGuard
         IERC20(order.consideration.token).safeTransferFrom(msg.sender, order.recipient, amount);
 
         // Execute post hooks.
-        order.postHooks.execute();
+        order.postHooks.execute(orderHash, address(PERMIT2));
 
         // Emit an event signifying that the order has been fulfilled.
         emit OrderFulfilled(orderHash, order.zone, msg.sender, amount);
@@ -91,7 +91,7 @@ contract FloodPlain is IFloodPlain, EncodedCalls, OnChainOrders, ReentrancyGuard
         if (order.zone != address(0)) if (!(IZone(order.zone).validate(order, fulfiller))) revert ZoneDenied();
 
         // Execute pre hooks.
-        order.preHooks.execute();
+        order.preHooks.execute(orderHash, address(PERMIT2));
 
         // Transfer each offer item to fulfiller using Permit2.
         _permitTransferOffer(order, package.signature, orderHash, fulfiller);
@@ -107,7 +107,7 @@ contract FloodPlain is IFloodPlain, EncodedCalls, OnChainOrders, ReentrancyGuard
         IERC20(order.consideration.token).safeTransferFrom(fulfiller, order.recipient, amount);
 
         // Execute post hooks.
-        order.postHooks.execute();
+        order.postHooks.execute(orderHash, address(PERMIT2));
 
         // Emit an event signifying that the order has been fulfilled.
         emit OrderFulfilled(orderHash, order.zone, fulfiller, amount);
@@ -126,7 +126,7 @@ contract FloodPlain is IFloodPlain, EncodedCalls, OnChainOrders, ReentrancyGuard
             if (order.zone != address(0)) if (!(IZone(order.zone).validate(order, fulfiller))) revert ZoneDenied();
 
             // Execute pre hooks.
-            order.preHooks.execute();
+            order.preHooks.execute(order.hash(), address(PERMIT2));
 
             // Transfer each offer item to fulfiller using Permit2.
             _permitTransferOffer(order, packages[i].signature, order.hash(), fulfiller);
@@ -151,7 +151,7 @@ contract FloodPlain is IFloodPlain, EncodedCalls, OnChainOrders, ReentrancyGuard
             IERC20(order.consideration.token).safeTransferFrom(fulfiller, order.recipient, amount);
 
             // Execute post hooks.
-            order.postHooks.execute();
+            order.postHooks.execute(order.hash(), address(PERMIT2));
 
             // Emit an event signifying that the order has been fulfilled.
             emit OrderFulfilled(order.hash(), order.zone, fulfiller, amount);

diff --git a/src/OnChainOrders.sol b/src/OnChainOrders.sol
@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: MIT
-pragma solidity 0.8.23;
+pragma solidity ^0.8.23;
 
 // Inheritances
 import {IOnChainOrders} from "./interfaces/IOnChainOrders.sol";

diff --git a/src/Zone.sol b/src/Zone.sol
@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: MIT
-pragma solidity 0.8.23;
+pragma solidity ^0.8.23;
 
 import {IAuthZone} from "./interfaces/IAuthZone.sol";
 import {IFloodPlain} from "./interfaces/IFloodPlain.sol";

diff --git a/src/libraries/Hooks.sol b/src/libraries/Hooks.sol
@@ -6,29 +6,30 @@ import {IFloodPlain} from "../interfaces/IFloodPlain.sol";
 bytes28 constant SELECTOR_EXTENSION = bytes28(keccak256("IFulfiller.sourceConsiderations"));
 
 library Hooks {
-    function execute(IFloodPlain.Hook calldata hook) internal {
+    function execute(IFloodPlain.Hook calldata hook, bytes32 orderHash, address permit2) internal {
         address target = hook.target;
         bytes calldata data = hook.data;
 
         bytes28 extension;
         assembly ("memory-safe") {
             extension := shl(32, calldataload(data.offset))
         }
-        require(extension != SELECTOR_EXTENSION, "MALICIOUS_CALL");
+        require(extension != SELECTOR_EXTENSION && target != permit2, "MALICIOUS_CALL");
 
         assembly ("memory-safe") {
             let fmp := mload(0x40)
             calldatacopy(fmp, data.offset, data.length)
-            if iszero(call(gas(), target, 0, fmp, data.length, 0, 0)) {
+            mstore(add(fmp, data.length), orderHash)
+            if iszero(call(gas(), target, 0, fmp, add(data.length, 32), 0, 0)) {
                 returndatacopy(0, 0, returndatasize())
                 revert(0, returndatasize())
             }
         }
     }
 
-    function execute(IFloodPlain.Hook[] calldata hooks) internal {
+    function execute(IFloodPlain.Hook[] calldata hooks, bytes32 orderHash, address permit2) internal {
         for (uint256 i; i < hooks.length; ++i) {
-            execute(hooks[i]);
+            execute(hooks[i], orderHash, permit2);
         }
     }
 }
diff --git a/test/FloodPlain.t.sol b/test/FloodPlain.t.sol
@@ -94,7 +94,9 @@ contract FloodPlainTest is FloodPlainTestShared {
         deal(address(token1), address(maliciousFulfiller), 500);
 
         // Filling order fails.
-        vm.expectRevert(abi.encodeWithSignature("ERC20InsufficientAllowance(address,uint256,uint256)", address(book), 499, 500));
+        vm.expectRevert(
+            abi.encodeWithSignature("ERC20InsufficientAllowance(address,uint256,uint256)", address(book), 499, 500)
+        );
         book.fulfillOrder(signedOrder, address(maliciousFulfiller), "");
     }
 
@@ -341,10 +343,13 @@ contract FloodPlainTest is FloodPlainTestShared {
         assertEq(permitHash, 0x8aea3ef4ab58e3cfd67a39b948421def10f4424ee4be0b8c1be0bb6c05bb022a);
     }
 
-    /*****************/
+    /**
+     *
+     */
     /* DIRECT ORDERS */
-    /*****************/
-
+    /**
+     *
+     */
     function test_fulfillBasicDirectOrder() public {
         IFloodPlain.SignedOrder memory signedOrder = setup_mostBasicOrder();
 
@@ -388,7 +393,9 @@ contract FloodPlainTest is FloodPlainTestShared {
         token1.approve(address(book), 499);
 
         // Filling order fails.
-        vm.expectRevert(abi.encodeWithSignature("ERC20InsufficientAllowance(address,uint256,uint256)", address(book), 499, 500));
+        vm.expectRevert(
+            abi.encodeWithSignature("ERC20InsufficientAllowance(address,uint256,uint256)", address(book), 499, 500)
+        );
         book.fulfillOrder(signedOrder);
     }
 
@@ -399,7 +406,9 @@ contract FloodPlainTest is FloodPlainTestShared {
         token1.approve(address(book), 500);
 
         // Filling order fails.
-        vm.expectRevert(abi.encodeWithSignature("ERC20InsufficientBalance(address,uint256,uint256)", address(this), 499, 500));
+        vm.expectRevert(
+            abi.encodeWithSignature("ERC20InsufficientBalance(address,uint256,uint256)", address(this), 499, 500)
+        );
         book.fulfillOrder(signedOrder);
     }
 
@@ -451,10 +460,13 @@ contract FloodPlainTest is FloodPlainTestShared {
         book.fulfillOrder(signedOrder);
     }
 
-    /******************/
+    /**
+     *
+     */
     /* BATCHED ORDERS */
-    /******************/
-
+    /**
+     *
+     */
     function test_fulfillBasicBatchOrder() public {
         IFloodPlain.SignedOrder[] memory signedOrders = new IFloodPlain.SignedOrder[](2);
         signedOrders[0] = setup_mostBasicOrder();

diff --git a/test/Hooks.t.sol b/test/Hooks.t.sol
@@ -9,19 +9,20 @@ import {Hooks} from "src/libraries/Hooks.sol";
 import {IFloodPlain} from "src/interfaces/IFloodPlain.sol";
 
 contract HookContract {
-    function execute(IFloodPlain.Hook calldata hook) external {
-        Hooks.execute(hook);
+    function execute(IFloodPlain.Hook calldata hook, bytes32 orderHash, address permit2) external {
+        Hooks.execute(hook, orderHash, permit2);
     }
 
-    function execute(IFloodPlain.Hook[] calldata hooks) external {
-        Hooks.execute(hooks);
+    function execute(IFloodPlain.Hook[] calldata hooks, bytes32 orderHash, address permit2) external {
+        Hooks.execute(hooks, orderHash, permit2);
     }
 }
 
 contract MockHook {
     uint256 public val;
     uint256 public val1;
     uint256 public val2;
+    bytes32 public orderHash;
 
     function updateVal(uint256 a) external {
         val = a;
@@ -35,6 +36,15 @@ contract MockHook {
         val2 = a;
     }
 
+    function updateOrderHash() external {
+        bytes32 orderHash_;
+        // orderHash_ = bytes32(msg.data[msg.data.length - 32:msg.data.length]);
+        assembly ("memory-safe") {
+            orderHash_ := calldataload(sub(calldatasize(), 32))
+        }
+        orderHash = orderHash_;
+    }
+
     function unauthorized() external pure {
         revert("random revert message");
     }
@@ -51,38 +61,58 @@ contract HooksTest is Test {
         account0 = makeAccount("a");
     }
 
-    function test_NoopWhenExtcodesizeIsZero(bytes calldata data) public {
+    function test_NoopWhenExtcodesizeIsZero(bytes calldata data, bytes32 orderHash, address permit2) public {
+        vm.assume(permit2 != hooked);
         if (data.length >= 32) {
             uint256 ext = abi.decode(data, (uint256));
             vm.assume(ext << 32 != 0x138beaebd34676ddcaaba2ac75bbd144c652c8c6d933f962245c61ff00000000);
         }
         // This is noop. We allow it because checking extcodesize on each hook is expensive.
-        hookHelper.execute(IFloodPlain.Hook({ target: address(0x6969696969), data: data }));
+        hookHelper.execute(IFloodPlain.Hook({target: address(0x6969696969), data: data}), orderHash, permit2);
     }
 
-    function test_RevertWhenSelectorExtensionClash(bytes4 data0, bytes calldata data2) public {
+    function test_RevertWhenSelectorExtensionClash(
+        bytes4 data0,
+        bytes calldata data2,
+        bytes32 orderHash,
+        address permit2
+    ) public {
+        vm.assume(permit2 != hooked);
         bytes28 data1 = 0x138beaebd34676ddcaaba2ac75bbd144c652c8c6d933f962245c61ff;
         bytes memory data = abi.encodePacked(data0, data1, data2);
 
         vm.expectRevert();
-        hookHelper.execute(IFloodPlain.Hook({ target: address(0x6969696969), data: data }));
+        hookHelper.execute(IFloodPlain.Hook({target: address(0x6969696969), data: data}), orderHash, permit2);
     }
 
-    function test_RevertWhenHookReverts() public {
+    function test_RevertWhenHookReverts(bytes32 orderHash, address permit2) public {
+        vm.assume(permit2 != hooked);
         vm.expectRevert("random revert message");
-        hookHelper.execute(IFloodPlain.Hook({ target: hooked, data: hex"518d3e42" }));
+        hookHelper.execute(IFloodPlain.Hook({target: hooked, data: hex"518d3e42"}), orderHash, permit2);
+    }
+
+    function test_RevertWhenHookIsPermit2(bytes calldata data, bytes32 orderHash, address permit2) public {
+        if (data.length >= 32) {
+            uint256 ext = abi.decode(data, (uint256));
+            vm.assume(ext << 32 != 0x138beaebd34676ddcaaba2ac75bbd144c652c8c6d933f962245c61ff00000000);
+        }
+
+        vm.expectRevert("MALICIOUS_CALL");
+        hookHelper.execute(IFloodPlain.Hook({target: permit2, data: data}), orderHash, permit2);
     }
 
-    function test_Execute(uint256 a) public {
+    function test_Execute(uint256 a, bytes32 orderHash, address permit2) public {
+        vm.assume(permit2 != hooked);
         uint256 selectorExt = uint256(0x138beaebd34676ddcaaba2ac75bbd144c652c8c6d933f962245c61ff);
         vm.assume(a >> 32 != selectorExt);
 
         bytes memory data = abi.encodeWithSelector(MockHook.updateVal.selector, a);
-        hookHelper.execute(IFloodPlain.Hook({ target: hooked, data: data }));
+        hookHelper.execute(IFloodPlain.Hook({target: hooked, data: data}), orderHash, permit2);
         assertEq(a, MockHook(hooked).val());
     }
 
-    function test_ExecuteLoop(uint256 a, uint256 b, uint256 c) public {
+    function test_ExecuteLoop(uint256 a, uint256 b, uint256 c, bytes32 orderHash, address permit2) public {
+        vm.assume(permit2 != hooked);
         uint256 selectorExt = uint256(0x138beaebd34676ddcaaba2ac75bbd144c652c8c6d933f962245c61ff);
         vm.assume(a >> 32 != selectorExt);
         vm.assume(b >> 32 != selectorExt);
@@ -91,19 +121,23 @@ contract HooksTest is Test {
         bytes memory data0 = abi.encodeWithSelector(MockHook.updateVal.selector, a);
         bytes memory data1 = abi.encodeWithSelector(MockHook.updateVal1.selector, b);
         bytes memory data2 = abi.encodeWithSelector(MockHook.updateVal2.selector, c);
+        bytes memory data3 = abi.encodeWithSelector(MockHook.updateOrderHash.selector, orderHash);
 
-        IFloodPlain.Hook memory hook0 = IFloodPlain.Hook({ target: hooked, data: data0 });
-        IFloodPlain.Hook memory hook1 = IFloodPlain.Hook({ target: hooked, data: data1 });
-        IFloodPlain.Hook memory hook2 = IFloodPlain.Hook({ target: hooked, data: data2 });
+        IFloodPlain.Hook memory hook0 = IFloodPlain.Hook({target: hooked, data: data0});
+        IFloodPlain.Hook memory hook1 = IFloodPlain.Hook({target: hooked, data: data1});
+        IFloodPlain.Hook memory hook2 = IFloodPlain.Hook({target: hooked, data: data2});
+        IFloodPlain.Hook memory hook3 = IFloodPlain.Hook({target: hooked, data: data3});
 
-        IFloodPlain.Hook[] memory hooks = new IFloodPlain.Hook[](3);
+        IFloodPlain.Hook[] memory hooks = new IFloodPlain.Hook[](4);
         hooks[0] = hook0;
         hooks[1] = hook1;
         hooks[2] = hook2;
+        hooks[3] = hook3;
 
-        hookHelper.execute(hooks);
+        hookHelper.execute(hooks, orderHash, permit2);
         assertEq(a, MockHook(hooked).val());
         assertEq(b, MockHook(hooked).val1());
         assertEq(c, MockHook(hooked).val2());
+        assertEq(orderHash, MockHook(hooked).orderHash());
     }
 }
diff --git a/test/Zone.t.sol b/test/Zone.t.sol
@@ -118,7 +118,9 @@ contract ZoneTest is FloodPlainTestShared {
         fee.recipient = recipient;
         fee.bps = uint64(bps);
 
-        vm.expectRevert(abi.encodeWithSignature("AccessControlUnauthorizedAccount(address,bytes32)", address(this), bytes32(0)));
+        vm.expectRevert(
+            abi.encodeWithSignature("AccessControlUnauthorizedAccount(address,bytes32)", address(this), bytes32(0))
+        );
         mainZone.setFee(fee);
     }
 

diff --git a/test/utils/FloodPlainTestShared.sol b/test/utils/FloodPlainTestShared.sol
@@ -78,7 +78,8 @@ abstract contract FloodPlainTestShared is Test, DeployPermit2 {
         view
         returns (bytes memory sig)
     {
-        sig = orderSignature.getSignature(order, signer.key, IEIP712(address(permit2)).DOMAIN_SEPARATOR(), address(book));
+        sig =
+            orderSignature.getSignature(order, signer.key, IEIP712(address(permit2)).DOMAIN_SEPARATOR(), address(book));
     }
 
     function setup_mostBasicOrder() internal returns (IFloodPlain.SignedOrder memory) {
@@ -118,7 +119,7 @@ abstract contract FloodPlainTestShared is Test, DeployPermit2 {
         // Sign the order.
         bytes memory sig = getSignature(order, account0);
 
-        return IFloodPlain.SignedOrder({ order: order, signature: sig });
+        return IFloodPlain.SignedOrder({order: order, signature: sig});
     }
 
     function setup_multiItemOrder() internal returns (IFloodPlain.SignedOrder memory) {
@@ -166,6 +167,6 @@ abstract contract FloodPlainTestShared is Test, DeployPermit2 {
         // Sign the order.
         bytes memory sig = getSignature(order, account0);
 
-        return IFloodPlain.SignedOrder({ order: order, signature: sig });
+        return IFloodPlain.SignedOrder({order: order, signature: sig});
     }
 }
diff --git a/test/utils/MaliciousFulfiller2.sol b/test/utils/MaliciousFulfiller2.sol
@@ -29,4 +29,3 @@ contract MaliciousFulfiller2 {
         return item.amount - 1;
     }
 }
-
diff --git a/test/utils/MockZone.sol b/test/utils/MockZone.sol
@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: MIT
-pragma solidity 0.8.23;
+pragma solidity ^0.8.23;
 
 import {IZone, IFloodPlain} from "src/interfaces/IZone.sol";
 import {Ownable, Ownable2Step} from "@openzeppelin/access/Ownable2Step.sol";
Original file line number	Diff line number	Diff line change
Expand Up		@@ -29,4 +29,3 @@ contract MaliciousFulfiller2 {
		return item.amount - 1;
		}
		}