feat(wrapper): helpers contract #7
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,136 @@ | ||
| // SPDX-License-Identifier: MIT OR Apache-2.0 | ||
| pragma solidity >=0.7.6 <0.9.0; | ||
| pragma abicoder v2; | ||
|
|
||
| import {ICowAuthentication, ICowWrapper} from "./CowWrapper.sol"; | ||
|
|
||
| /// @title CoW Protocol Wrapper Helpers | ||
| /// @notice Helper contract providing validation and encoding utilities for CoW Protocol wrapper chains | ||
| /// @dev This contract is not designed to be gas-efficient and is intended for off-chain use only. | ||
|
There was a problem hiding this comment. Documentation note: Since this contract is intended for off-chain use only, consider adding a note about deployment strategy. Should this be deployed on all networks where wrappers will be used? Or is it expected to be called via If it's meant for off-chain static calls only, you might also consider adding a comment explaining that the contract doesn't need to be deployed for its intended use case. |
||
| contract CowWrapperHelpers { | ||
| /// @notice Thrown when a provided address is not an authenticated wrapper | ||
| /// @param wrapperIndex The index of the invalid wrapper in the array | ||
| /// @param unauthorized The address that is not authenticated as a wrapper | ||
| /// @param authenticatorContract The authentication contract that rejected the wrapper | ||
| error NotAWrapper(uint256 wrapperIndex, address unauthorized, address authenticatorContract); | ||
|
|
||
| /// @notice Thrown when a wrapper's parseWrapperData doesn't fully consume its data | ||
| /// @param wrapperIndex The index of the wrapper that didn't consume all its data | ||
| /// @param remainingWrapperData The data that was not consumed by the wrapper | ||
| error WrapperDataNotFullyConsumed(uint256 wrapperIndex, bytes remainingWrapperData); | ||
|
|
||
| /// @notice Thrown when a wrapper's parseWrapperData reverts, which is assumed to be due to malformed data | ||
| /// @param wrapperIndex The index of the wrapper with malformed data | ||
| /// @param wrapperError The error returned by the wrapper's parseWrapperData | ||
| error WrapperDataMalformed(uint256 wrapperIndex, bytes wrapperError); | ||
|
|
||
| /// @notice Thrown when the data for the wrapper is too long. Its limited to 65535 bytes. | ||
| /// @param wrapperIndex The index of the wrapper with data that is too long | ||
| /// @param exceedingLength The observed length of the data | ||
| error WrapperDataTooLong(uint256 wrapperIndex, uint256 exceedingLength); | ||
|
|
||
| /// @notice Thrown when the settlement contract is authenticated as a solver | ||
| /// @dev The settlement contract should not be a solver to prevent direct settlement calls bypassing wrappers | ||
| /// @param settlementContract The settlement contract address | ||
| /// @param authenticatorContract The authentication contract that authenticated the settlement as a solver | ||
| error SettlementContractShouldNotBeSolver(address settlementContract, address authenticatorContract); | ||
|
|
||
| /// @notice Thrown when wrappers in the chain use different settlement contracts | ||
| /// @param wrapperIndex The index of the wrapper with a mismatched settlement | ||
| /// @param expectedSettlement The settlement contract used by the first wrapper | ||
| /// @param actualSettlement The settlement contract used by this wrapper | ||
| error SettlementMismatch(uint256 wrapperIndex, address expectedSettlement, address actualSettlement); | ||
|
|
||
| /// @notice A definition for a single call to a wrapper | ||
| /// @dev This corresponds to the `wrappers` item structure on the CoW Orderbook API | ||
| struct WrapperCall { | ||
| /// @notice The smart contract that will be receiving the call | ||
| address target; | ||
|
|
||
| /// @notice Any additional data which will be required to execute the wrapper call | ||
| bytes data; | ||
| } | ||
|
|
||
| /// @notice The authentication contract used to verify wrapper contracts | ||
| ICowAuthentication public immutable WRAPPER_AUTHENTICATOR; | ||
|
|
||
| /// @notice The authentication contract used to verify solvers | ||
| ICowAuthentication public immutable SOLVER_AUTHENTICATOR; | ||
|
|
||
| /// @notice Constructs a new CowWrapperHelpers contract | ||
| /// @param wrapperAuthenticator_ The ICowAuthentication contract used to verify wrapper contracts | ||
| /// @param solverAuthenticator_ The ICowAuthentication contract used to verify solvers | ||
| constructor(ICowAuthentication wrapperAuthenticator_, ICowAuthentication solverAuthenticator_) { | ||
| WRAPPER_AUTHENTICATOR = wrapperAuthenticator_; | ||
| SOLVER_AUTHENTICATOR = solverAuthenticator_; | ||
| } | ||
|
|
||
| /// @notice Validates a wrapper chain configuration and builds the properly formatted wrapper data | ||
| /// @dev Performs comprehensive validation of the wrapper chain before encoding: | ||
| /// 1. Verifies each wrapper is authenticated via WRAPPER_AUTHENTICATOR | ||
| /// 2. Verifies each wrapper's data is valid and fully consumed by calling parseWrapperData | ||
| /// 3. Verifies all wrappers use the same settlement contract (from first wrapper's SETTLEMENT) | ||
| /// 4. Verifies the settlement contract is not authenticated as a solver | ||
| /// The returned wrapper data format is: [data0][addr1][data1][addr2][data2]... | ||
| /// where data0 is for the first wrapper, addr1 is the second wrapper address, etc. | ||
| /// Note: No settlement address is appended as wrappers now use a static SETTLEMENT. | ||
| /// @param wrapperCalls Array of calls in execution order | ||
| /// @return wrapperData The encoded wrapper data ready to be passed to the first wrapper's wrappedSettle | ||
| function verifyAndBuildWrapperData(WrapperCall[] memory wrapperCalls) | ||
| external | ||
| view | ||
| returns (bytes memory wrapperData) | ||
| { | ||
| if (wrapperCalls.length == 0) { | ||
| return wrapperData; | ||
| } | ||
|
|
||
| // First pass: verify all wrappers are authenticated | ||
| for (uint256 i = 0; i < wrapperCalls.length; i++) { | ||
| require( | ||
| WRAPPER_AUTHENTICATOR.isSolver(wrapperCalls[i].target), | ||
| NotAWrapper(i, wrapperCalls[i].target, address(WRAPPER_AUTHENTICATOR)) | ||
| ); | ||
| } | ||
|
|
||
| // Get the expected settlement from the first wrapper | ||
|
There was a problem hiding this comment. Minor optimization opportunity: The settlement contract is fetched multiple times in the loop (once per wrapper). Consider fetching it once outside the loop and storing it, then just comparing inside the loop. // Get the expected settlement from the first wrapper (already done)
address expectedSettlement = address(ICowWrapper(wrapperCalls[0].target).SETTLEMENT());
for (uint256 i = 0; i < wrapperCalls.length; i++) {
// All wrappers must use the same settlement contract
address wrapperSettlement = address(ICowWrapper(wrapperCalls[i].target).SETTLEMENT());
require(
wrapperSettlement == expectedSettlement, SettlementMismatch(i, expectedSettlement, wrapperSettlement)
);
// ...
}This is already quite efficient - the external call is only made once per wrapper. If further optimization is desired, you could skip checking |
||
| address expectedSettlement = address(ICowWrapper(wrapperCalls[0].target).SETTLEMENT()); | ||
|
|
||
| for (uint256 i = 0; i < wrapperCalls.length; i++) { | ||
| // All wrappers must use the same settlement contract | ||
| address wrapperSettlement = address(ICowWrapper(wrapperCalls[i].target).SETTLEMENT()); | ||
|
|
||
| require( | ||
| wrapperSettlement == expectedSettlement, SettlementMismatch(i, expectedSettlement, wrapperSettlement) | ||
| ); | ||
|
|
||
| // The wrapper data must be parsable and fully consumed | ||
| try ICowWrapper(wrapperCalls[i].target).parseWrapperData(wrapperCalls[i].data) returns ( | ||
|
There was a problem hiding this comment. Security: Wrapper data validation design is sound The
This design forces wrapper authors to be explicit about their data format and prevents users from injecting unauthorized wrapper addresses into the chain. There was a problem hiding this comment. Excellent security design: The validation approach here is well thought out:
This design elegantly solves the security concern raised in earlier reviews about users potentially injecting unauthorized wrapper addresses into the chain. |
||
| bytes memory remainingWrapperData | ||
| ) { | ||
| if (remainingWrapperData.length > 0) { | ||
| revert WrapperDataNotFullyConsumed(i, remainingWrapperData); | ||
| } | ||
| } catch (bytes memory err) { | ||
| revert WrapperDataMalformed(i, err); | ||
| } | ||
| } | ||
|
|
||
| // The Settlement Contract should not be a solver | ||
|
There was a problem hiding this comment. Security consideration: This check is valuable but worth noting its limitations. The settlement contract being a "solver" is checked via This assumes:
Consider documenting this assumption in the contract-level NatSpec or constructor, especially since misconfiguration here could bypass wrapper logic entirely. |
||
| if (SOLVER_AUTHENTICATOR.isSolver(expectedSettlement)) { | ||
| revert SettlementContractShouldNotBeSolver(expectedSettlement, address(SOLVER_AUTHENTICATOR)); | ||
| } | ||
|
|
||
| // Build wrapper data | ||
| for (uint256 i = 0; i < wrapperCalls.length; i++) { | ||
| if (i > 0) { | ||
| wrapperData = abi.encodePacked(wrapperData, wrapperCalls[i].target); | ||
| } | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Should we also check that the last wrapper is a solver? It's needed to settle but it could be nice to skip this check when testing. Maybe @MartinquaXD can comment on this.
Collaborator
Author
There was a problem hiding this comment. assuming you are referring to
Contributor
There was a problem hiding this comment. No, I mean the last wrapper in the chain (so
Contributor
There was a problem hiding this comment. Anyway, the helper isn't as "final" as the wrapper design, we can always deploy a new one and use that if we need to. I'd say we should check that the wrappers can settle, but maybe that's unneeded.
Collaborator
Author
There was a problem hiding this comment. yea true this is very malleable and view only
Collaborator
Author
There was a problem hiding this comment. ok so I had a chance to slowly read this again and its making a bit more sense now. in my mind, I have always thought of all wrappers as being solvers (effectively, a "wrapper" is a subset of "solver"). this is because wrappers can be specified in any order or with/without other wrappers, so it doesn't really make sense to assume one wrapper could not be a solver. this helper contract I suppose makes that same assumption. |
||
| require(wrapperCalls[i].data.length < 65536, WrapperDataTooLong(i, wrapperCalls[i].data.length)); | ||
|
There was a problem hiding this comment. Great fix! The uint16 overflow edge case from the previous review has been properly addressed with:
This prevents silent truncation and makes debugging much easier. |
||
| wrapperData = abi.encodePacked(wrapperData, uint16(wrapperCalls[i].data.length), wrapperCalls[i].data); | ||
| } | ||
|
|
||
| return wrapperData; | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Documentation clarity: The NatSpec is excellent. One minor suggestion - consider adding an example of what a valid implementation might look like:
Though given the complexity of different wrapper implementations, the current documentation may be sufficient.