feat(wrapper): helpers contract

[kaze-cow]

Description

To ease integration of off-chain utilities, create a CowWrapperHelpers contract which contains functions useful for working with wrappers.

Context

During a previous call we had with the backend team, we found out that the implementation on the services repo is more complicated than we envisioned. Since there doesn't seem to be a great way to have both high gas efficiency and also to encode ABI parameters directly, I thought that perhaps an on-chain contract which builds the necessary contract.

In addition, while I was implementing this contract, I realized that most of the security checks that we have been concerned about (ex. solvers calling unapproved wrappers, solvers never calling the settlement contract, calldata being wrongly encoded, etc.) would actually be resolved by adding these checks to the contract.

Right now the helpers contract only implements one function:

• verifyAndBuildWrapperData -- assembles wrapperData needed to call wrappedSettle from raw arguments

Additionally, to have a check to verify the wrapperData supplied to each wrapper is valid, a new function was added to the CowWrapper interface, parseWrapperData. This function will parse the data that is needed for a wrapper, and if any additional data remains at the end, return it. This allows for us to simultaneously verify that the user is providing valid wrapperData, as well as that they havent supplied more data than is needed.

For detailed information about these functions, please read the descriptions offered in the actual code.

Out of Scope

The goal of this PR is to determine the value that having this contract can bring, and whether the value justifies adding a potentially complicated async call prior to auction encoding in the driver/solver implementations.

Testing Instructions

Follow the test command instructions in the README

We want to make sure:

• the CowWrapperHelpers correctly encodes needed calldata for CowWrapper
• any reasonable validations that can be taken by this contract are taken
• there is sufficient documentation to ensure that this contract can be easily integrated

[fedgiac]

I imagine this should be its own contract, maybe called WrapperRegistry and having different methods compared to the authenticator. It'd make sense to introduce it in a different PR, since we are convinced at this point that we need one and it's very much independent of everything else. Let's keep it like this in this PR for now.

[kaze-cow]

is there an abstract EIP or something for an "address registry"? we could use something like that

[fedgiac]

I don't know any, but I imagine there should be something by OpenZeppelin.

[kaze-cow]

btw I wasn't able to find any great libraries or established implementations.

I think it would be good to reuse the existing GPv2Authenticators because they are pretty much exactly what we need and the only thing thats kind of wierd about it is the terminology isSolver. Would rather avoid audits

[fedgiac]

Why this specific check? The settlement contract can't call itself anyway because of the reentrancy guard.

[kaze-cow]

yea this check may not be super important but it had to do with a concern I recall from last night where if the last call in the wrapper chain was not actually the settlement contrcat but a solver, and then it was actually a malicious contract... I forgot exactly what our thinking was there. maybe familiar.

but we dont allow users to set the settlementContract so the chances that this will ever become a legitimate check are very low.

[fedgiac]

I think it was about the user being able to specify the settlement contract as a wrapper, which in the old design was an issue because it would actually execute a settlement with that data and skip any other wrapper. So here it would be something like checking that each wrapper isn't the settlement contract. Not sure it's needed now because we call a specific function that isn't available in the settlement contract, so it should just revert. (And it can't be used to DoS solvers because it always reverts.)

[kaze-cow]

yea ok that makes sense. I will remove this check and now that we have the new interface this is less of an issue.

[fedgiac]

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.

[kaze-cow]

assuming you are referring to settlementContract as being the "last wrapper"... it has to not be a solver

[fedgiac]

No, I mean the last wrapper in the chain (so wrapperData = encodePacked(..., lastWrapperAddress, lastWrapperData, settlementAddress)), this is possibly the only one that needs to be a solver. (Well, right now all wrappers should be solvers, but in principle they could just check that the caller is a solver or a wrapper.)

[fedgiac]

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.

[kaze-cow]

yea true this is very malleable and view only

[kaze-cow]

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.

[fedgiac]

There's no indication of data lengths so far, which means that every wrapper is supposed to determine by itself what is amount of wrapperData it needs to take. If wrapperData has fixed size, this could be hardcoded in the contract itself, but if this length is variable then it needs to be encoded as part of the data. By design, individualWrapperData is controlled by the user. This means that the user can manipulate this data in a way that the resulting encoding affects the call sequence execution (e.g., calling an unexpected contract and stopping the wrapper chain), and this is definitely something that we want to avoid.

The fix would be making the length encoding part of helper and the generic wrapper. It could be just a few bytes at the start of the data that determine the length of the data that needs to be read (critically, the length of the data comes from the solver by reading the actual length of the data in the order and not directly from the user).

[kaze-cow]

This means that the user can manipulate this data in a way that the resulting encoding affects the call sequence execution

Yep, that was my concern as well

If we do have the call data length encoded in the official wrapperData call format then that still gives the user the ability to 1) supply invalid/undecodable data to the contract or 2) add their own useless calldata to the end of the call. To prevent both of these cases, a call to the actual wrapper contract is necessary and it happens here https://github.com/cowprotocol/euler-integration-contracts/pull/7/files#diff-86d649557c3f63b62c8097429b27a169c325a385d523699c9f5b003b9b385767R78 . The call will revert if the data is invalid, and if any superfluous data is supplied (aka injecting a bad wrapper address), that can be identified too by the remaining calldata after the function call. This also has kind of a nice effect of forcing wrapper implementors to think carefully about their wrapper's input data design.

I realize this is a significant design decision so I added it to the PR description.

(ofc there is also the minor gas usage of having the extra calldata but yea its pretty minimal)

[fedgiac]

I agree this can be addressed in the wrapper implementation, but I think this is super easy to miss when actually doing the coding.
For example, let's take some hypothetical wrapper that takes some data of constant size. The natural implementation is doing something like MyData data = abi.decode(wrapperData[:DATA_SIZE], (MyData)) and forwards wrapperData[DATA_SIZE:] to the next wrapper. I don't expect a third-party developer to realize that they just introduced a vulnerability and that they're instead expected to encode and validate the length themselves, no matter how many comments we add about it.
(Another interesting consequence is that all wrapper must take some data in this case, zero data means that the wrapper is vulnerable, another thing that isn't trivial.)
I think almost all wrappers need to do some data validation anyway, so I think it's anyway helpful that we do it for them in the generic wrapper and the price for this is quite small.

[kaze-cow]

I don't expect a third-party developer to realize that they just introduced a vulnerability and that they're instead expected to encode and validate the length themselves, no matter how many comments we add about it.

maybe this is the point you are trying to make, but I don't see the vulnerability 😆 in the case that a wrapper takes a constant input size, it takes the constant input size, sends the rest back as you say, and in the verify function if there is leftover, then its assumed that the user supplied unnecessary data at the end and that is not valid.

(Another interesting consequence is that all wrapper must take some data in this case, zero data means that the wrapper is vulnerable, another thing that isn't trivial.)

in the current impl a wrapper can take 0 data, at least according to the tests https://github.com/cowprotocol/euler-integration-contracts/pull/7/files#diff-8ffcefd1b174c494a961932dde9a124a719bd297b7bb7804a6ae1276e91c71d7R207

[claude]

Claude encountered an error —— View job

---

Code Review in Progress

Todo List

• Read changed files and understand implementation
• Review code quality and best practices
• Analyze security considerations
• Check performance implications
• Verify test coverage and quality
• Review documentation completeness
• Verify previous feedback has been addressed
• Post comprehensive review feedback

[claude]

Great fix! The uint16 overflow edge case from the previous review has been properly addressed with:

1. Clear error with diagnostic info (WrapperDataTooLong)
2. Validation before encoding (require(wrapperCalls[i].data.length < 65536, ...))
3. Comprehensive test coverage including edge cases at 65535 and 65536 bytes

This prevents silent truncation and makes debugging much easier.

[claude]

Security: Wrapper data validation design is sound

The parseWrapperData validation approach effectively prevents malicious data injection:

• Wrapper contracts must fully consume their data (line 111-112)
• Any unconsumed data is rejected (WrapperDataNotFullyConsumed)
• Invalid/malformed data causes WrapperDataMalformed revert (line 114-115)

This design forces wrapper authors to be explicit about their data format and prevents users from injecting unauthorized wrapper addresses into the chain.

[claude]

Excellent test coverage! The SettlementMismatch test case from the previous review feedback has been properly implemented. This verifies that all wrappers in a chain must use the same settlement contract, preventing chain inconsistencies.

[claude]

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 i==0 since it's comparing with itself.

[claude]

Excellent security design: The validation approach here is well thought out:

1. Forces wrapper authors to implement parseWrapperData explicitly
2. Catches malformed data through try-catch
3. Prevents injection attacks by ensuring all data is consumed

This design elegantly solves the security concern raised in earlier reviews about users potentially injecting unauthorized wrapper addresses into the chain.

[claude]

Documentation clarity: The NatSpec is excellent. One minor suggestion - consider adding an example of what a valid implementation might look like:

/// @dev Must be implemented by concrete wrapper contracts. Used for pre-execution validation.
///      The implementation should consume its wrapper-specific data and return the remainder.
///      Example implementation:
///      ```solidity
///      function parseWrapperData(bytes calldata wrapperData)
///          external pure returns (bytes calldata remainingWrapperData)
///      {
///          // Decode first 32 bytes as our data
///          require(wrapperData.length >= 32, "Insufficient data");
///          // myData = abi.decode(wrapperData[:32], (MyStruct));
///          return wrapperData[32:];
///      }
///      ```

Though given the complexity of different wrapper implementations, the current documentation may be sufficient.

[claude]

Excellent test coverage: The edge case tests around uint16 limits (65535 max, 65536 overflow) are comprehensive and well-designed. This addresses the previous review feedback perfectly.

[claude]

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 eth_call (staticcall) without actual deployment?

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.

[claude]

Security consideration: This check is valuable but worth noting its limitations. The settlement contract being a "solver" is checked via SOLVER_AUTHENTICATOR, which is a separate authenticator from WRAPPER_AUTHENTICATOR.

This assumes:

1. The two authenticators are properly managed
2. The settlement contract is never added to SOLVER_AUTHENTICATOR

Consider documenting this assumption in the contract-level NatSpec or constructor, especially since misconfiguration here could bypass wrapper logic entirely.

[claude]

Test coverage suggestion: Consider adding a test that verifies the behavior when wrapperCalls[0] itself has an invalid settlement (not just mismatches in subsequent wrappers). Currently line 97 fetches from the first wrapper without validating it's a valid contract with a SETTLEMENT() function.

function test_verifyAndBuildWrapperData_RevertsOnInvalidFirstWrapper() public {
    address notAContract = makeAddr("notAContract");
    wrapperAuth.setSolver(notAContract, true); // Pass authentication check
    
    CowWrapperHelpers.WrapperCall[] memory wrapperCalls = new CowWrapperHelpers.WrapperCall[](1);
    wrapperCalls[0] = CowWrapperHelpers.WrapperCall({target: notAContract, data: hex""});
    
    // Should revert when trying to call SETTLEMENT() on a non-contract
    vm.expectRevert();
    helpers.verifyAndBuildWrapperData(wrapperCalls);
}

This edge case could occur if the authenticator is misconfigured.