feat: collateral swap wrapper

[kaze-cow]

Description

Implements the CowEvcCollateralSwapWrapper in order to satisfy a usecase for the Euler integration.

Context

Read up on notion

Considerations

• Overall this wrapper looks a lot like a condensed version of the CowEvcClosePositionWrapper. It has to enable the destination collateral, however.
• This flow is supposed to support both KIND_BUY and KIND_SELL from the COW side so that the user can select either an amount exact in or exact out, and swap it just like they would on a regular DEX.

Out of Scope

Every line of code in this PR should be considered in-scope.

Testing Instructions

Follow the test command instructions in the README

We want to make sure:

• the flow works as expected for the established cases of Euler finance.
• there are not security opportunities an arbitrary user on the open internet could take advantage of
• there are not security opportunities that a solver could take advantage that would give access to a significant amount of funds

fixes https://linear.app/cowswap/issue/COW-93/initial-merge-contract-of-cowevccollateralswapwrapper

[claude]

Claude encountered an error —— View job

---

Claude encountered an error —— View job

---

Code Review in Progress

Performing comprehensive code review focusing on:

• Read repository guidelines and previous feedback
• Examine current code state and recent changes
• Review code quality and best practices
• Analyze security considerations
• Evaluate performance implications
• Assess test coverage and quality
• Check documentation completeness
• Provide consolidated feedback

[claude]

High Priority: Potential precision loss in price calculation

The calculation params.swapAmount * toVaultPrice / fromVaultPrice can suffer from precision loss, especially when:

1. Tokens have different decimal places
2. Prices have significant magnitude differences
3. Division truncates instead of rounding up

Consider:

• Adding a small buffer/margin to ensure sufficient tokens are transferred
• Rounding up instead of down for transferAmount
• Documenting assumptions about token decimals and price precision

This could cause transactions to fail if the calculated transferAmount is slightly less than what's actually needed for the swap.

[claude]

Inconsistent with other wrappers: Should use onBehalfOfAccount: address(this) for consistency.

Both other wrappers use address(this):

• CowEvcOpenPositionWrapper.sol:206
• CowEvcClosePositionWrapper.sol:262

Using address(this) is the correct approach for maintaining proper EVC operation context.

[claude]

Missing comprehensive test coverage

This test suite only covers happy paths. Consider adding tests for:

Security:

• Unauthorized users attempting to swap another user's collateral
• Replay attacks with consumed pre-approved hashes

Edge Cases:

• Expired deadlines
• Insufficient collateral in account
• Invalid/non-existent vault addresses
• Zero swap amounts
• Maximum swap amounts

Health Factor:

• Swaps that would violate collateralization requirements
• Swaps during unhealthy account states

Order Types:

• KIND_BUY orders (all current tests use KIND_SELL)
• Mixed order scenarios

The current security tests (lines 351-392) are minimal and don't cover these important scenarios.

[claude]

Test uses hardcoded price instead of actual prices

The calculation borrowAmount * 2500e18 / 0.99e18 hardcodes:

1. WETH price as exactly 2500 sUSDS
2. Slippage as 0.99 (1%)

This should use milkSwap.prices(WETH) to match actual test conditions and prevent flakiness if prices change. The hardcoded values may not accurately reflect the vault share amounts needed.

[claude]

Suspicious borrow receiver

The borrowed WETH goes to address(this) (the test contract) instead of the account:

IBorrowing(EWETH).borrow(borrowAmount, address(this));

This seems incorrect for "setting up a leveraged position" for the user's account. The borrowed assets should either:

1. Go to the account
2. Be transferred to the account after borrowing
3. Have a comment explaining why the test contract receives them

This doesn't match the expected production flow where users borrow into their own accounts.

[claude]

Magic number without explanation

The * 1 ether / 0.98 ether multiplier applies 2% slippage but lacks explanation.

Consider:

1. Using a named constant: uint256 constant TEST_SLIPPAGE = 0.98 ether;
2. Adding a comment explaining why 2% slippage is applied
3. Testing different slippage values to ensure robustness

Same issue at line 148 with 0.999 ether (0.1%).

[claude]

Documentation doesn't match execution order

The documented order:

1. Enable new collateral vault
2. Transfer collateral from subaccount
3. Execute settlement

Actual execution order:

1. Enable vault (line 190)
2. Execute settlement (line 246-251)
3. Transfer happens during settlement (line 319-321)

Consider either:

• Updating the documentation to match actual execution order
• Adding a note that this describes the logical workflow, not execution order

[claude]

Critical: Potential precision loss in price calculation

transferAmount = params.swapAmount * toVaultPrice / fromVaultPrice;

This calculation can suffer from precision loss, especially when:

1. Tokens have different decimal places (e.g., WBTC has 8 decimals, sUSDS has 18)
2. Prices have significant magnitude differences
3. Division truncates instead of rounding up

Risk: The calculated transferAmount could be slightly less than what's actually needed for the swap to succeed, causing the transaction to revert.

Recommendation: Consider either:

• Adding a small buffer (e.g., transferAmount = (params.swapAmount * toVaultPrice / fromVaultPrice) + 1)
• Rounding up instead of down
• Documenting the precision assumptions and decimal requirements

[claude]

High Priority: Hardcoded price instead of actual prices

IERC4626(ESUSDS).deposit(collateralAmount + borrowAmount * 2500e18 / 0.99e18, account);

This hardcodes:

1. WETH price as exactly 2500 sUSDS
2. Slippage as 0.99 (1%)

Issue: This doesn't use milkSwap.prices(WETH) to match actual test conditions, which could lead to test flakiness if prices change or calculations don't match reality.

Recommendation:

uint256 wethPrice = milkSwap.prices(WETH);
uint256 susdsPrice = milkSwap.prices(SUSDS);
uint256 approximateSwapOutput = borrowAmount * wethPrice / susdsPrice * 99 / 100; // 1% slippage
IERC4626(ESUSDS).deposit(collateralAmount + approximateSwapOutput, account);

[claude]

High Priority: Suspicious borrow receiver

IBorrowing(EWETH).borrow(borrowAmount, address(this));

The borrowed WETH goes to address(this) (the test contract) instead of account.

Issue: This doesn't match the stated goal of "setting up a leveraged position" for the user's account. In a real scenario, users would borrow into their own accounts.

Recommendation: Either:

1. Borrow to account if that's the intended behavior
2. Transfer the borrowed assets to account after borrowing
3. Add a comment explaining why the test contract receives the borrowed assets

[fedgiac]

First round of comments. My only major concern is about the function _findRatePrices: the price that it computes can be easily manipulated by solvers and may cause things to break even if the solver isn't malicious.

[fedgiac]

Suggested change

	/// protection mixed in so that signed orders are only valid for specific
	/// this contract.
	/// protection mixed in so that signed orders are only valid for this
	/// contract at this address and on this one chain.

[fedgiac]

There are no further checks, meaning that if the EVC is in any way insecure or has some unexpected code path then someone would be able to execute arbitrary settlements. I don't think we want to risk that, the peace of mind we get for being able to sleep soundly if the EVC gets hacked is worth the extra checks.

We cam do this by hashing all input parameters and validate them here.
In the current code it wouldn't even be that hard: it's already writing abi.encodeCall(this.evcInternalSwap, (settleData, wrapperData, remainingWrapperData) to memory, so it's just about hashing that and comparing that with the hash of message.data as stored in transient storage.
It would need to account for the possibility of being called multiple times or out of order though.

[kaze-cow]

great suggestion! implemented 362e61a

its worth noting that since we are validating the calldata is correct and verifying its only called once (by clearing after the call is complete), we can now remove the onBehalfOfAccount check, which saves us a external contract call! We could hypothetically also remove the msg.sender == address(EVC) check as well but decided to leave it for now since its cheap.

[fedgiac]

Using balances is risky if users have any other order or logic changing the balance of their address. Will think a bit more about this tomorrow if there's another solution.

[kaze-cow]

yep that is a possibility, but it seems infitesimably rare and even when it were to happen--the worst case I can think of is maybe in the exact same transaction there is another order to add collateral to the user's account for a loan they have on their main/owner wallet, and then the collateral gets transferred to their subaccount instead of remaining the main account. the user would end up with funds in a place they are not expecting them, and perhaps they get liquidated?

[kaze-cow]

also yea I think the only way a user could change the balance in their own account is through some CoW means (either they have a separate order or they specify some other wrapper as part of this order)

in either case the user has to do this to themselves somehow

[fedgiac]

I missed it at first, but this isn't a valid struct hash for EIP712 because the type hash is missing (see here).

[fedgiac]

We should add a test that checks the hash matches the expected EIP712 hash as computed by Foundry. We could also consider using the dedicated OpenZeppelin library.

[kaze-cow]

in this case having a EIP712 for the hash used by approved hashes isnt strictly required because they wont be signed by a user wallet and will not have any relation to the EVC.permit signing flow. but it is indeed good for standardizability I suppose.