feat: add wrapper #6
Conversation
There was a problem hiding this comment.
I'm thinking about the security guarantees that a settlement wrapper should provide.
The current implementation works nicely for Euler because the settlement parameters don't need to be validated, it's enough that the settle call is executed in the context of the EVC and it's a solver feature.
It's designed to be (1) a tool used by solvers to enable fancy new features.
Another example of wrapper would be one that enforces hooks. This doesn't immediately work with the current wrapper implementation because each intermediate wrapper has no guarantee that the data passed in settle is correct: the enforced hook wrapper checks that the interactions contain the desired hook, but then the next wrapper is a "fake" wrapper that's controlled by the solver that rewrites the settlement data, invalidating any check based on the settle input. (This can be achieved, for example, by a solver enabling a fake wrapper interface with EIP7702 and routing the wrapper through it.)
The hook-enforcing wrapper is an example of (2) a tool used by users that want to make sure solvers only send out solutions that satisfy some checks.
So there are two use cases here and the current implementation covers the first.
The very nice thing about the current implementation: the code right now is optimal in terms of number of calls, any solution that enforces a specific settlement would need to have some indirection inside and would add gas overhead.
All that said: I think it's reasonable to prioritize gas efficiency here and go for the current implementation, but it's good to keep in mind its limits. This should be documented in the code so that it's clear what should be expected from it. Also, the design for wrappers type 2 is going to require a more elaborated construction (that I don't think we should do now).
|
@fedgiac thanks for your feedback!
...usecase (1) is more of a theoretical. Euler would also fall under case (2) because the frontend builds the data the user wants to executes and submits it as appdata. it is built into the wrapper design a way to ensure that its impossible to execute your order without supplying the wrapper.
how is a fake wrapper injected? every "real" wrapper in the call chain, and the settlement contract, verifies that the previous wrapper is authorized. so its not possible to introduce a wrapper in the callchain that isnt authorized, because at some point you have to return to a real wrapper or the settlement contract, and those will always verify the previous caller. now users modifying wrapperData for future wrappers is a potential concern and its something that wrappers are going to have to be able to protect somehow (probably through length monitoring)
I was expecting that auth would always be kept outside of the scope of wrappers behavior, and if something requires auth/verification/malleability prevention, that should be handled on a case by case basis for the wrapper. concrete examples:
as you can see all these hypothetical cases have their own unique authentication and data validity requirements. |
It verifies authorization by checking that the previous caller is a solver. What could happen is that the solver EIP7702-delegates a contract that's compatible with the wrapper interface and that rewrites the settlement (the call stack is delegated solver (origin) → official wrapper → delegated solver → settlement). The settlement sent to the official wrapper and the settlement executed at the end could be completely different.
If the wrapper just facilitates execution and doesn't need to validate the exact content of the settlement execution, then the current design is good, which is what I was thinking about when mentioning case 1. The discussed Aave flow is case 1 as well because it doesn't enforce specific settlement parameters.
This probably depends on the concrete design, I can see now how this could work as well.
Another interesting use case may be the flash-loan router. It does enforce settlement data, but this is because the data needs to go through possibly untrusted flash-loan contracts that could change the data outside of the solver's knowledge, while wrappers are expected to be under the control of the solvers. So maybe it's ok to move it to the current wrapper design and only authenticate the "local" flash-loan steps. Maybe the core of the question is: is it fine in the wrapper design if wrappers can't trust the content of |
fedgiac
left a comment
fedgiac
left a comment
There was a problem hiding this comment.
On the discussion above: we decided it's fine if the settle data isn't validated, I consider that resolved.
There was a problem hiding this comment.
Nothing of this is vendored so it should be moved to src.
I think each small interface could have its own file for ease of copypaste (ideally with a link to some URL where I can compare it with the full interface).
There was a problem hiding this comment.
i actually deliberately moved all the interfaces/dependencies for CowWrapper into the same file. The idea is that its a lot easier to just copy one self-contained file with no dependencies from project to project than a bunch of misc files/whole folder. I rezlide this as I was starting to implement the wrapper in the flash-loan-router repo.
There was a problem hiding this comment.
the reason I put CowWrapper into vendor is because its not directly related to the purposes/goal of this repository, though I understand you see it as an in-house product so it shouldn't be called "vendor".
How about moving it (and any related sources) to a new subdirectory src/wrapper instead?
There was a problem hiding this comment.
So, the issue is that the wrapper doesn't belong to this repo (Euler). I think we decided to have it here just to make it easy for you to iterate on the wrapper with this use case in mind. But I would think
- Either; Eventually will go to its own project (@cowprotocol/settlement-wrapper-contracts)
- Or, my prefered cause of simplicity. We rename this project to (@cowprotocol/settlement-wrapper-contracts) and put the main contracts in
srcand all wrappers insrc/wrappers
There was a problem hiding this comment.
I also think we should break a bit more this file into smaller ones with the interfaces and contracts separately
There was a problem hiding this comment.
though I understand you see it as an in-house product so it shouldn't be called "vendor".
Yeah, vendor sounds like this is just copied, but part of this code is new for this repo.
How about moving it (and any related sources) to a new subdirectory src/wrapper instead?
It makes sense to separate the wrapper code.
Overall, I'm ok with keeping ICoWSettlement and ICowAuthentication in the same file (and in vendored as well) but I'd still say that ICowWrapper should have its own file (in src/wrapper).
There was a problem hiding this comment.
I see the motivation for this was addresses in the inline comment, I think the idea is nice. I consider this comment resolved with no change needed.
- @notice This file is completely self-contained (ie no dependencies) and can be portably copied to whatever projects it is needed.
Co-authored-by: Federico Giacon <58218759+fedgiac@users.noreply.github.com>
…ler-integration-contracts into feat/wrapper-base
| @@ -0,0 +1,42 @@ | |||
| // SPDX-License-Identifier: GPL-3.0-or-later | |||
There was a problem hiding this comment.
NOTE: this file is replaced in the next PR (I just didnt want to dirtify this PR with the replacement). Not worth reviewing in this PR at all.
- Changed constructor to accept CowSettlement instead of CowAuthentication - Removed WrapperHasNoSettleTarget error and length check - Settlement contract is now stored as immutable SETTLEMENT - Authenticator is derived from SETTLEMENT.authenticator() - Updated _internalSettle to call static SETTLEMENT when wrapperData is empty - Updated tests to pass settlement contract to constructors - Removed settlement address from wrapperData in test helpers 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
useful for display in informational UIs
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| vm.expectRevert(abi.encodeWithSelector(CowWrapper.InvalidSettleData.selector, settleData)); | ||
| testWrapper.wrappedSettle(settleData, wrapperData); | ||
| } | ||
|
|
There was a problem hiding this comment.
Test Coverage: Missing edge cases
Consider adding tests for:
- Empty
wrapperData - Malformed length (claims more bytes than available)
- Incomplete length prefix (1 byte only)
- Missing next wrapper address
Example:
function test_wrappedSettle_RevertsOnEmptyWrapperData() public {
vm.prank(solver);
vm.expectRevert(/* InvalidWrapperData */);
testWrapper.wrappedSettle(_createSimpleSettleData(1), hex"");
}
src/CowWrapper.sol
Outdated
| // 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])); |
There was a problem hiding this comment.
Critical: Missing bounds validation
This line reads 2 bytes without validating wrapperData has sufficient length. Solidity calldata slicing returns zero-padding for out-of-bounds access rather than reverting, allowing malformed data to be processed.
Add validation:
require(wrapperData.length >= 2, InvalidWrapperData());
uint16 nextWrapperDataLen = uint16(bytes2(wrapperData[0:2]));
require(wrapperData.length >= 2 + nextWrapperDataLen, InvalidWrapperData());Define error: error InvalidWrapperData();
This has been flagged by multiple reviewers but remains unaddressed.
| vm.expectRevert(abi.encodeWithSelector(CowWrapper.InvalidSettleData.selector, settleData)); | ||
| testWrapper.wrappedSettle(settleData, wrapperData); | ||
| } | ||
|
|
There was a problem hiding this comment.
Test Coverage: Missing critical edge cases
Add tests for malformed wrapper data:
- Empty
wrapperData(should revert) - Length prefix exceeds available bytes
- Incomplete length prefix (1 byte only)
- Missing next wrapper address
Example:
function test_wrappedSettle_RevertsOnEmptyWrapperData() public {
vm.prank(solver);
vm.expectRevert(/* InvalidWrapperData */);
testWrapper.wrappedSettle(_createSimpleSettleData(1), hex"");
}
src/CowWrapper.sol
Outdated
| // 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])); |
There was a problem hiding this comment.
Critical: Missing bounds validation
This line reads 2 bytes without first validating that wrapperData has sufficient length. In Solidity, calldata slicing returns zero-padding for out-of-bounds access rather than reverting, which can lead to silent failures.
Recommendation:
require(wrapperData.length >= 2, InvalidWrapperData());
uint16 nextWrapperDataLen = uint16(bytes2(wrapperData[0:2]));
require(wrapperData.length >= 2 + nextWrapperDataLen, InvalidWrapperData());Add error definition:
error InvalidWrapperData();This prevents malformed wrapper data from causing unpredictable behavior in the wrapper chain.
There was a problem hiding this comment.
This makes sense, though the first require seems unneeded since if the check were to fail but be ignored then nextWrapperDataLen would be zero and the next require would cause a revert.
But maybe this isn't needed at all, do you know if Solidity calldata array access checks boundary conditions? In case it would be nice to have a test.
src/CowWrapper.sol
Outdated
| /// @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 wrapperData Encoded data for this wrapper and the chain of next wrappers/settlement. |
There was a problem hiding this comment.
Documentation: Wrapper data format needs precise specification
Per @fedgiac and @anxolin's discussion, the encoding format must be explicitly specified for wrappers to be composable. The current description should be more precise.
Suggested documentation:
/// @param wrapperData Encoded wrapper chain with the following format:
/// Structure: [uint16 len1][bytes data1][address wrapper2][uint16 len2][bytes data2][address wrapper3]...
///
/// Each wrapper in the chain consists of:
/// - 2 bytes: uint16 length of wrapper-specific data
/// - `length` bytes: wrapper-specific data for this wrapper
/// - 20 bytes: address of next wrapper (omitted for the final wrapper)
///
/// The final wrapper in the chain omits the next wrapper address and calls SETTLEMENT directly.
///
/// Example: [0x0005][0xAABBCCDDEE][0x1234...ABCD][0x0003][0x112233]
/// ↑len ↑data ↑next wrapper ↑len ↑data (final, no next address)
///
/// @dev SECURITY: `settleData` is NOT guaranteed to remain unchanged through the wrapper chain.
/// Intermediate wrappers could modify it before passing it along. Do not rely on
/// `settleData` validation for security-critical checks.There was a problem hiding this comment.
I agree it's good to be more verbose in the encoding definition. In particular it's easy to miss that one doesn't need to include an address in the wrapper data before the call.
There was a problem hiding this comment.
implemented ai suggestion verbatim daf68fc
Co-authored-by: Anxo Rodriguez <anxo@cow.fi>
fedgiac
left a comment
fedgiac
left a comment
There was a problem hiding this comment.
The implementation looks good, most comments are just about simplification, documentation, and clarity.
It would be better to also have unit tests that check edge cases in the encoding (zero length, max length, calldata too short, too long). Maybe also a basic test that checks the deployment parameters (SETTLEMENT and AUTHENTICATOR).
There was a problem hiding this comment.
I see the motivation for this was addresses in the inline comment, I think the idea is nice. I consider this comment resolved with no change needed.
- @notice This file is completely self-contained (ie no dependencies) and can be portably copied to whatever projects it is needed.
| /** | ||
| * @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. | ||
| * It contains: | ||
| * * CowWrapper -- an abstract base contract which should be inherited by all wrappers | ||
| * * ICowWrapper -- the required interface for all wrappers | ||
| * * ICowSettlement -- A minimized interface and base structures for CoW Protocol settlement contract. From https://github.com/cowprotocol/contracts/blob/main/src/contracts/GPv2Settlement.sol | ||
| * * ICowAuthentication -- The authentication interface used by ICowSettlement. From https://github.com/cowprotocol/contracts/blob/main/src/contracts/interfaces/GPv2Authentication.sol | ||
| */ |
There was a problem hiding this comment.
The tags here don't apply to the file but to the next entry in the file, so you'd get that ICowAuthentication is titled "CoW Wrapper all-in-one integration file" (you can check that by running forge docs and looking at the output in ./docs/).
You could just remove the @-tags and make this a standard comment instead of a docs comment.
src/CowWrapper.sol
Outdated
| // 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])); |
There was a problem hiding this comment.
This makes sense, though the first require seems unneeded since if the check were to fail but be ignored then nextWrapperDataLen would be zero and the next require would cause a revert.
But maybe this isn't needed at all, do you know if Solidity calldata array access checks boundary conditions? In case it would be nice to have a test.
| } | ||
|
|
||
| function test_integration_ThreeWrappersChained() public { | ||
| // Set up a more sophisticated settlement call to make sure it all gets through as expected. |
There was a problem hiding this comment.
Is it needed? I think the wrapper doesn't care about what the settle data is apart for the selector, so for a unit test all of this seems unnecessary and could be simplified. For an e2e test this would be reasonable though.
There was a problem hiding this comment.
yea I mostly wanted this test becuase it became an important feature of the wrappers, and I wanted a specific test that shows that this exact functionality works
There was a problem hiding this comment.
i would rather not create a new dedicated e2e testin file because everything we need is already in this file and there isnt a whole lot to e2e out.
| @@ -0,0 +1,157 @@ | |||
| # CLAUDE.md | |||
There was a problem hiding this comment.
It's ok to leave it like this in this PR and the following ones, but eventually it would be nice to extract useful information from here and move it to the readme.
these changes are ultimately requierd for use in the wrappers later, and they are generally good to have for any wrappers in the future.
src/CowWrapper.sol
Outdated
| // 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])); |
There was a problem hiding this comment.
Critical Security Issue: Missing bounds validation
This line reads 2 bytes from wrapperData without validating that the data has sufficient length. While Solidity calldata slicing returns zero-padding for out-of-bounds access rather than reverting, this can lead to silent failures:
- Empty or 1-byte
wrapperDatawould read as length 0 (or partial value) - Line 144 could then slice beyond available data
- Malformed data could pass through without appropriate validation
Recommendation:
require(wrapperData.length >= 2, InvalidWrapperData());
uint16 nextWrapperDataLen = uint16(bytes2(wrapperData[0:2]));
require(wrapperData.length >= 2 + nextWrapperDataLen, InvalidWrapperData());Add error definition:
error InvalidWrapperData();This was flagged by @fedgiac at line 140 but remains unaddressed. It's critical for preventing malformed wrapper data from causing unpredictable behavior.
| if (remainingWrapperData.length == 0) { | ||
| // No more wrapper data - we're calling the final settlement contract | ||
| // Verify the settle data has the correct function selector | ||
| require(bytes4(settleData[:4]) == ICowSettlement.settle.selector, InvalidSettleData(settleData)); |
There was a problem hiding this comment.
Performance Consideration: Error data allocation
As @fedgiac mentioned, when custom errors include large calldata parameters like settleData, the error may be allocated in memory even when the require condition is true. With large settlement data, this could lead to unnecessary gas costs from extra mstore operations.
Recommendation: Consider benchmarking this with large settleData to verify gas impact. If significant, could refactor to:
if (bytes4(settleData[:4]) != ICowSettlement.settle.selector) {
revert InvalidSettleData(settleData);
}Though given your use of require() style elsewhere per the coding guidelines, this may be acceptable if the gas impact is negligible.
| } | ||
| } else { | ||
| // Extract the next wrapper address from the first 20 bytes of wrapperData | ||
| address nextWrapper = address(bytes20(remainingWrapperData[:20])); |
There was a problem hiding this comment.
Enhancement: Consider zero address validation
While @carlos-cow and @kaze-cow discussed this, an explicit zero-address check provides clearer debugging output for malformed wrapper data:
address nextWrapper = address(bytes20(remainingWrapperData[:20]));
require(nextWrapper != address(0), InvalidWrapperAddress());However, this is lower priority if bounds validation is added at line 140, which would catch most malformation cases. This is optional but could improve the developer experience.
| vm.prank(solver); | ||
| vm.expectRevert(abi.encodeWithSelector(CowWrapper.InvalidSettleData.selector, settleData)); | ||
| wrapper1.wrappedSettle(settleData, wrapperData); | ||
| } |
There was a problem hiding this comment.
Test Coverage: Missing critical edge cases
The test suite lacks coverage for several important malformed data scenarios that could cause runtime failures. @fedgiac noted this in their review.
Missing test cases:
- Empty wrapperData - Should revert with clear error
- Insufficient wrapper data - Length prefix claims more bytes than available
- Partial length prefix - Only 1 byte provided
- Missing next wrapper address - Data indicates chaining but address is missing
Recommended additions:
function test_wrappedSettle_RevertsOnEmptyWrapperData() public {
bytes memory settleData = _createSimpleSettleData(1);
vm.prank(solver);
vm.expectRevert(); // Should revert with InvalidWrapperData once validation added
wrapper1.wrappedSettle(settleData, hex"");
}
function test_wrappedSettle_RevertsOnInsufficientData() public {
bytes memory settleData = _createSimpleSettleData(1);
// Claims 100 bytes but only provides 3
bytes memory wrapperData = hex"0064aabbcc";
vm.prank(solver);
vm.expectRevert(); // Should revert with InvalidWrapperData
wrapper1.wrappedSettle(settleData, wrapperData);
}
function test_wrappedSettle_RevertsOnPartialLengthPrefix() public {
bytes memory settleData = _createSimpleSettleData(1);
// Only 1 byte provided for 2-byte length prefix
bytes memory wrapperData = hex"00";
vm.prank(solver);
vm.expectRevert(); // Should revert with InvalidWrapperData
wrapper1.wrappedSettle(settleData, wrapperData);
}These tests will help ensure robust handling of malformed input.
| } | ||
|
|
||
| function test_next_CallsWrapperAndThenNextSettlement() public { | ||
| bytes memory settleData = abi.encodePacked(_createSimpleSettleData(1), hex"123456"); |
There was a problem hiding this comment.
Code Quality: Test could be simplified
As @fedgiac noted, for a unit test this level of settlement data construction isn't strictly necessary. The wrapper only cares about the function selector, not the actual trade parameters. Consider simplifying:
function test_next_CallsWrapperAndThenNextSettlement() public {
// For unit tests, can use minimal valid settle data
bytes memory settleData = abi.encodePacked(
ICowSettlement.settle.selector,
hex"123456" // arbitrary additional data
);
// ... rest of test
}This makes the test faster to read and understand, focusing on what's actually being tested (the wrapper chaining logic) rather than settlement data formatting.
test/unit/CowWrapper.t.sol
Outdated
|
|
||
| // 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); |
There was a problem hiding this comment.
Code Quality: Use correct selector source
It's confusing that the selector for the call to wrapper2 is taken from wrapper1. Should use the interface to get the selector:
| vm.expectCall(address(wrapper2), 0, abi.encodeWithSelector(wrapper1.wrappedSettle.selector), 1); | |
| vm.expectCall(address(wrapper2), 0, abi.encodeWithSelector(ICowWrapper.wrappedSettle.selector), 1); |
Same issue on line 150 for wrapper3. (@fedgiac noted this)
* add minor edge case checks (found a bug in max length!) * better documentation * clean up usages of `abi.encodeWithSelector` as appropriate * fix licenses
also switch to @inheritdoc
Description
Implementation of generalized wrappers on the smart contract side. For more info see the notion document
https://www.notion.so/cownation/Generalized-Wrapper-2798da5f04ca8095a2d4c56b9d17134e
How to Test/Review
forge test --match-path test/CowWrapper.t.solTo prevent this PR from being too unwieldy, its probably best not to look to detailed at the testing file at this time, as its mostly for POC purposes and is likely to change in the future. (up to you though).
Out of Scope
This PR should be reviewing the general implementation of the wrapper. Things related to euler can go in separate PR for the wrapper update forthecoming.
This PR is not intended to have 100% test coverage or be audit ready. it is intended to set a direction and make sure we have a strong base to work off of for the other PRs related to Euler.
Related Issues
contractsrepo feat: implement generic wrapper contracts#247