fix comments from federico

* add minor edge case checks (found a bug in max length!)
* better documentation
* clean up usages of `abi.encodeWithSelector` as appropriate
* fix licenses

Author: kaze-cow

src/CowWrapper.sol

@@ -1,11 +1,10 @@
 // SPDX-License-Identifier: MIT OR Apache-2.0
-pragma solidity >=0.7.6 <0.9.0;
-pragma abicoder v2;
+pragma solidity >=0.8.0 <0.9.0;
 
 /**
- * @title CoW Wrapper all-in-one integration file
- * @author CoW Protocol Developers
- * @notice This file is completely self-contained (ie no dependencies) and can be portably copied to whatever projects it is needed.
+ * CoW Wrapper all-in-one integration file
+ * CoW Protocol Developers
+ * This file is completely self-contained (ie no dependencies) and can be portably copied to whatever projects it is needed.
  * It contains:
  * * CowWrapper -- an abstract base contract which should be inherited by all wrappers
  * * ICowWrapper -- the required interface for all wrappers
@@ -41,7 +40,7 @@ interface ICowSettlement {
         bytes signature;
     }
 
-    /// @notice Interaction data structure for pre/intra/post-settlement hooks
+    /// @notice Interaction data structure for pre/intra/post-settlement actions which are supplied by the solver to complete the user request
     struct Interaction {
         address target;
         uint256 value;
@@ -89,7 +88,7 @@ interface ICowWrapper {
     ///      before calling the next wrapper or settlement contract in the chain.
     /// @param settleData ABI-encoded call to ICowSettlement.settle() containing trade data
     /// @param wrapperData Encoded data for this wrapper and the chain of next wrappers/settlement.
-    ///                    Format: [2-byte len][wrapper-specific-data][next-address]([2-byte len][wrapper-specific-data][next-address]...)
+    ///                    Format: [2-byte len][user-supplied wrapper specific data][next address]([2-byte len][user supplied wrapper specific data]...)
     function wrappedSettle(bytes calldata settleData, bytes calldata wrapperData) external;
 }
 
@@ -127,17 +126,17 @@ abstract contract CowWrapper is ICowWrapper {
     /// @notice Initiates a wrapped settlement call
     /// @dev Entry point for solvers to execute wrapped settlements. Verifies the caller is a solver,
     ///      validates wrapper data, then delegates to _wrap() for custom logic.
-    /// @param settleData ABI-encoded call to ICowSettlement.settle() containing trade data
+    /// @param settleData ABI-encoded call to ICowSettlement.settle() containing trade data. NOTE: wrappers may read this data, but it should not be trusted for anything of great importance (ex. destination of funds) because a malicious solver can modify this data later in the chain.
     /// @param wrapperData Encoded data for this wrapper and the chain of next wrappers/settlement.
-    ///                    Format: [2-byte len][wrapper-specific-data][next-address]([2-byte len][wrapper-specific-data][next-address]...)
+    ///                    Format: [2-byte len][user-supplied wrapper specific data][next address]([2-byte len][user supplied wrapper specific data]...)
     function wrappedSettle(bytes calldata settleData, bytes calldata wrapperData) external {
         // Revert if not a valid solver
         require(AUTHENTICATOR.isSolver(msg.sender), NotASolver(msg.sender));
 
         // Find out how long the next wrapper data is supposed to be
         // We use 2 bytes to decode the length of the wrapper data because it allows for up to 64KB of data for each wrapper.
         // This should be plenty of length for all identified use-cases of wrappers in the forseeable future.
-        uint16 nextWrapperDataLen = uint16(bytes2(wrapperData[0:2]));
+        uint256 nextWrapperDataLen = uint16(bytes2(wrapperData[0:2]));
 
         // Delegate to the wrapper's custom logic
         uint256 remainingWrapperDataStart = 2 + nextWrapperDataLen;

test/EmptyWrapper.sol

@@ -1,6 +1,5 @@
-// SPDX-License-Identifier: GPL-3.0-or-later
+// SPDX-License-Identifier: MIT OR Apache-2.0
 pragma solidity ^0.8;
-pragma abicoder v2;
 
 import {ICowSettlement, CowWrapper} from "../src/CowWrapper.sol";
 

test/unit/CowWrapper.t.sol

@@ -1,8 +1,8 @@
-// SPDX-License-Identifier: GPL-3.0-or-later
+// SPDX-License-Identifier: MIT OR Apache-2.0
 pragma solidity ^0.8;
 
 import {Test} from "forge-std/Test.sol";
-import {CowWrapper, ICowSettlement, ICowAuthentication} from "../../src/CowWrapper.sol";
+import {CowWrapper, ICowWrapper, ICowSettlement, ICowAuthentication} from "../../src/CowWrapper.sol";
 import {EmptyWrapper} from "../EmptyWrapper.sol";
 
 import {MockWrapper, MockCowSettlement, MockCowAuthentication} from "./mocks/MockCowProtocol.sol";
@@ -28,9 +28,11 @@ contract CowWrapperTest is Test {
         authenticator.setSolver(solver, true);
 
         // Create test wrapper and three EmptyWrapper instances with the settlement contract
-        wrapper1 = new MockWrapper(ICowSettlement(address(mockSettlement)), 65536);
-        wrapper2 = new MockWrapper(ICowSettlement(address(mockSettlement)), 65536);
-        wrapper3 = new MockWrapper(ICowSettlement(address(mockSettlement)), 65536);
+        // (use type(uint16).max because it will force consuming all the wrapper data, which is
+        // most useful for these tests)
+        wrapper1 = new MockWrapper(ICowSettlement(address(mockSettlement)), type(uint16).max);
+        wrapper2 = new MockWrapper(ICowSettlement(address(mockSettlement)), type(uint16).max);
+        wrapper3 = new MockWrapper(ICowSettlement(address(mockSettlement)), type(uint16).max);
 
         // Add all wrappers as solvers
         authenticator.setSolver(address(wrapper1), true);
@@ -51,27 +53,39 @@ contract CowWrapperTest is Test {
         address[] memory tokens = new address[](tokenCount);
         uint256[] memory clearingPrices = new uint256[](tokenCount);
         for (uint256 i = 0; i < tokenCount; i++) {
-            tokens[i] = makeAddr(string(abi.encodePacked("Settle Token #", vm.toString(i + 1))));
+            tokens[i] = makeAddr(string.concat("Settle Token #", vm.toString(i + 1)));
             clearingPrices[i] = 100 * (i + 1);
         }
-        return abi.encodeWithSelector(
-            ICowSettlement.settle.selector, tokens, clearingPrices, new ICowSettlement.Trade[](0), _emptyInteractions()
+        return abi.encodeCall(
+            ICowSettlement.settle, (tokens, clearingPrices, new ICowSettlement.Trade[](0), _emptyInteractions())
+        );
+    }
+
+    function test_verifyInitialState() public {
+        assertEq(
+            address(wrapper1.SETTLEMENT()),
+            address(mockSettlement),
+            "Settlement contract should be initialized correctly"
+        );
+        assertEq(
+            address(wrapper1.AUTHENTICATOR()),
+            address(authenticator),
+            "Authenticator contract should be initialized from the settlement contract"
         );
     }
 
     function test_next_CallsWrapperAndThenNextSettlement() public {
         bytes memory settleData = abi.encodePacked(_createSimpleSettleData(1), hex"123456");
-        bytes memory secondCallWrapperData = hex"0003098765";
-        bytes memory wrapperData = abi.encodePacked(hex"00021234", address(wrapper1), secondCallWrapperData);
+        // here we encode [2-byte len] followed by the actual wrapper data (which is 3 bytes, 6 chars hex)
+        bytes memory secondCallWrapperData = abi.encodePacked(uint16(3), hex"098765");
+        // here we encode [2-byte len] followed by the actual wrapper data (which is 2 bytes, 4 chars hex), and build the chain
+        bytes memory wrapperData = abi.encodePacked(uint16(2), hex"1234", address(wrapper1), secondCallWrapperData);
 
-        // the wrapper gets called exactly twice (once below and again inside the wrapper data calling self)
-        vm.expectCall(address(wrapper1), 0, abi.encodeWithSelector(wrapper1.wrappedSettle.selector), 2);
+        // verify the outside wrapper call data
+        vm.expectCall(address(wrapper1), abi.encodeCall(ICowWrapper.wrappedSettle, (settleData, wrapperData)));
 
         // verify the internal wrapper call data
-        vm.expectCall(
-            address(wrapper1),
-            abi.encodeWithSelector(wrapper1.wrappedSettle.selector, settleData, secondCallWrapperData)
-        );
+        vm.expectCall(address(wrapper1), abi.encodeCall(ICowWrapper.wrappedSettle, (settleData, secondCallWrapperData)));
 
         // the settlement contract gets called once after wrappers (including the surplus data at the end)
         vm.expectCall(address(mockSettlement), 0, settleData, 1);
@@ -108,20 +122,22 @@ contract CowWrapperTest is Test {
         settlement.clearingPrices[0] = 100;
         settlement.clearingPrices[1] = 200;
 
-        settlement.trades = new ICowSettlement.Trade[](1);
-        settlement.trades[0] = ICowSettlement.Trade({
-            sellTokenIndex: 0,
-            buyTokenIndex: 1,
-            receiver: address(0x123),
-            sellAmount: 1000,
-            buyAmount: 900,
-            validTo: uint32(block.timestamp + 1000),
-            appData: bytes32(uint256(1)),
-            feeAmount: 10,
-            flags: 0,
-            executedAmount: 0,
-            signature: hex"aabbccddee"
-        });
+        settlement.trades = new ICowSettlement.Trade[](10);
+        for (uint256 i = 0; i < 10; i++) {
+            settlement.trades[i] = ICowSettlement.Trade({
+                sellTokenIndex: 0,
+                buyTokenIndex: 1,
+                receiver: address(0x123),
+                sellAmount: 1000 * i,
+                buyAmount: 900 * i,
+                validTo: uint32(block.timestamp + 1000),
+                appData: bytes32(uint256(1)),
+                feeAmount: 10,
+                flags: 0,
+                executedAmount: 0,
+                signature: hex"aabbccddee"
+            });
+        }
 
         settlement.interactions = [
             new ICowSettlement.Interaction[](0),
@@ -144,10 +160,13 @@ contract CowWrapperTest is Test {
         (address target, bytes memory fullCalldata) =
             CowWrapperHelpers.encodeWrapperCall(wrappers, datas, address(mockSettlement), settlement);
 
-        // all the wrappers gets called, with wrapper 1 called twice
-        vm.expectCall(address(wrapper1), 0, abi.encodeWithSelector(wrapper1.wrappedSettle.selector), 2);
-        vm.expectCall(address(wrapper2), 0, abi.encodeWithSelector(wrapper1.wrappedSettle.selector), 1);
-        vm.expectCall(address(wrapper3), 0, abi.encodeWithSelector(wrapper1.wrappedSettle.selector), 1);
+        // all the wrappers get called, with wrapper 1 called twice
+
+        // we only want to verify that wrappedSettle was called. (not the specific data passed to wrappedSettle)
+        bytes memory wrappedSettleSelector = abi.encodePacked(ICowWrapper.wrappedSettle.selector);
+        vm.expectCall(address(wrapper1), 0, wrappedSettleSelector, 2);
+        vm.expectCall(address(wrapper2), 0, wrappedSettleSelector, 1);
+        vm.expectCall(address(wrapper3), 0, wrappedSettleSelector, 1);
 
         // the settlement gets called with the full data
         vm.expectCall(address(mockSettlement), new bytes(0));
@@ -157,4 +176,106 @@ contract CowWrapperTest is Test {
         (bool success,) = target.call(fullCalldata);
         assertTrue(success, "Chained wrapper call should succeed");
     }
+
+    function test_wrappedSettle_RevertsOnZeroLengthWrapperData() public {
+        bytes memory settleData = _createSimpleSettleData(0);
+        bytes memory wrapperData = hex""; // Completely empty wrapper data
+
+        vm.prank(solver);
+        vm.expectRevert(); // Should revert with out-of-bounds array access
+        wrapper1.wrappedSettle(settleData, wrapperData);
+    }
+
+    function test_wrappedSettle_RevertsOnOneByteWrapperData() public {
+        bytes memory settleData = _createSimpleSettleData(0);
+        bytes memory wrapperData = hex"01"; // Only 1 byte - not enough to read the 2-byte length
+
+        vm.prank(solver);
+        vm.expectRevert(); // Should revert with out-of-bounds array access
+        wrapper1.wrappedSettle(settleData, wrapperData);
+    }
+
+    function test_wrappedSettle_SucceedsWithZeroLengthIndicator() public {
+        bytes memory settleData = _createSimpleSettleData(0);
+        bytes memory wrapperData = hex"0000"; // 2 bytes indicating 0-length wrapper data
+
+        // Should call settlement directly with no wrapper-specific data
+        vm.expectCall(address(mockSettlement), 0, settleData, 1);
+
+        vm.prank(solver);
+        wrapper1.wrappedSettle(settleData, wrapperData);
+    }
+
+    function test_wrappedSettle_SucceedsWithMaximumLengthWrapperData() public {
+        bytes memory settleData = _createSimpleSettleData(0);
+
+        // Create maximum length wrapper data (65535 bytes)
+        // Format: [2-byte length = 0xFFFF][65535 bytes of data]
+        bytes memory maxData = new bytes(65535);
+        for (uint256 i = 0; i < 65535; i++) {
+            maxData[i] = bytes1(uint8(i % 256));
+        }
+
+        bytes memory wrapperData = abi.encodePacked(uint16(65535), maxData);
+
+        // Should successfully parse the maximum length data and call settlement
+        vm.expectCall(address(mockSettlement), 0, settleData, 1);
+
+        vm.prank(solver);
+        wrapper1.wrappedSettle(settleData, wrapperData);
+    }
+
+    function test_wrappedSettle_RevertsWhenDataShorterThanIndicated() public {
+        bytes memory settleData = _createSimpleSettleData(0);
+
+        // Wrapper data claims to be 100 bytes but only provides 50
+        bytes memory shortData = new bytes(50);
+        bytes memory wrapperData = abi.encodePacked(uint16(100), shortData);
+
+        vm.prank(solver);
+        vm.expectRevert(); // Should revert with out-of-bounds array access
+        wrapper1.wrappedSettle(settleData, wrapperData);
+    }
+
+    function test_wrappedSettle_SucceedsWithMaxLengthAndNextWrapper() public {
+        bytes memory settleData = _createSimpleSettleData(0);
+
+        // Create maximum length wrapper data followed by next wrapper address
+        bytes memory maxData = new bytes(65535);
+        for (uint256 i = 0; i < 65535; i++) {
+            maxData[i] = bytes1(uint8(i % 256));
+        }
+
+        // Format: [2-byte length = 0xFFFF][65535 bytes of data][20-byte next wrapper address][remaining data]
+        bytes memory nextWrapperData = hex"00030000FF"; // 3 bytes of data for next wrapper
+        bytes memory wrapperData = abi.encodePacked(type(uint16).max, maxData, address(wrapper2), nextWrapperData);
+
+        // Should call wrapper2 with the remaining data
+        vm.expectCall(address(wrapper2), 0, abi.encodePacked(ICowWrapper.wrappedSettle.selector), 1);
+
+        vm.prank(solver);
+        wrapper1.wrappedSettle(settleData, wrapperData);
+    }
+
+    function test_wrappedSettle_RevertsWithInsufficientLengthData() public {
+        bytes memory settleData = _createSimpleSettleData(0);
+
+        // Format: [1-byte length = 1 (insufficient)]
+        bytes memory wrapperData = hex"01";
+
+        vm.expectRevert(new bytes(0));
+        vm.prank(solver);
+        wrapper1.wrappedSettle(settleData, wrapperData);
+    }
+
+    function test_wrappedSettle_RevertsWithInsufficientCallData() public {
+        bytes memory settleData = _createSimpleSettleData(0);
+
+        // Format: [2-byte length = 0xa][9 bytes of data (insufficient)]
+        bytes memory wrapperData = hex"000A123412341234123412";
+
+        vm.expectRevert(new bytes(0));
+        vm.prank(solver);
+        wrapper1.wrappedSettle(settleData, wrapperData);
+    }
 }

test/unit/mocks/MockCowProtocol.sol

@@ -1,4 +1,4 @@
-// SPDX-License-Identifier: GPL-2.0-or-later
+// SPDX-License-Identifier: MIT OR Apache-2.0
 pragma solidity ^0.8;
 
 import {ICowSettlement, ICowAuthentication, CowWrapper} from "../../../src/CowWrapper.sol";