-
Notifications
You must be signed in to change notification settings - Fork 0
feat: specialized wrappers #8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
* check solver on internalSettle function * check only callable through EVC * prevent reentrancy
make settlement the gatekeeper why did I not do this before
This reverts commit e56eac3.
best way to ensure the expected flow is followed exactly
* check solver on internalSettle function * check only callable through EVC * prevent reentrancy
they shouldn't have been in the repository
feat: use new wrapper from upstream
feat: working security
we use the settlement contract, so it shouldn't be needed anymore also soljson.latest is still here
src/CowEvcClosePositionWrapper.sol
Outdated
| { | ||
| address borrowAsset = IERC4626(borrowVault).asset(); | ||
| (address[] memory tokens, uint256[] memory clearingPrices,,) = abi.decode( | ||
| settleData[4:], (address[], uint256[], CowSettlement.CowTradeData[], CowSettlement.CowInteractionData[][3]) |
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.
Gas Optimization & Validation in _findRatePrices
Issue: Missing input validation and gas optimization opportunities
Impact: Medium - Gas efficiency and robustness
Current Issues:
- No validation that
tokens.length > 0 - No validation that arrays have matching lengths
- Loop continues after both prices are found (wastes gas)
Recommendation:
function _findRatePrices(bytes calldata settleData, address collateralVault, address borrowVault)
internal
view
returns (uint256 collateralVaultPrice, uint256 borrowPrice)
{
address borrowAsset = IERC4626(borrowVault).asset();
(address[] memory tokens, uint256[] memory clearingPrices,,) = abi.decode(
settleData[4:], (address[], uint256[], CowSettlement.CowTradeData[], CowSettlement.CowInteractionData[][3])
);
require(tokens.length > 0 && tokens.length == clearingPrices.length, PricesNotFoundInSettlement(collateralVault, borrowAsset));
// Early exit optimization - stop once both prices found
for (uint256 i = 0; i < tokens.length && (collateralVaultPrice == 0 || borrowPrice == 0); i++) {
if (tokens[i] == collateralVault) {
collateralVaultPrice = clearingPrices[i];
} else if (tokens[i] == borrowAsset) {
borrowPrice = clearingPrices[i];
}
}
require(collateralVaultPrice != 0 && borrowPrice != 0, PricesNotFoundInSettlement(collateralVault, borrowAsset));
}| if (repayAmount > debtAmount) { | ||
| // the user intends to repay all their debt. we will revert if their balance is not sufficient. | ||
| repayAmount = debtAmount; | ||
| } |
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.
Unclear repayAll Logic
Issue: The helperRepay function has unclear logic regarding repayment amounts
Impact: Medium - Code clarity and potential confusion
Current Behavior:
- Always tries to repay based on the owner's balance
- If
repayAmount > debtAmount, caps atdebtAmount - This makes the function generic but disconnects it from the settlement flow expectations
Questions:
- Should this function validate that the owner has received the expected repayment assets from the settlement?
- What happens if the settlement sent less than expected?
Recommendation: Add documentation explaining:
- The function repays using whatever balance the owner has (from the settlement)
- If the balance is insufficient for full repayment, it will revert when trying to transfer
- The settlement is responsible for ensuring sufficient tokens reach the owner
/// @notice Called by the EVC after a CoW swap is completed to repay the user's debt.
/// @dev This function repays debt using the owner's balance of the vault's underlying asset.
/// The CoW settlement must have transferred sufficient repayment assets to the owner's wallet.
/// If the owner's balance is less than their debt, only that amount is repaid (partial repayment).
/// If the balance exceeds the debt, only the debt amount is repaid (full repayment with dust).| /** | ||
| * @dev | ||
| */ | ||
| uint256 collateralAmount; |
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.
Missing Documentation in ClosePositionParams
Issue: The collateralAmount field in ClosePositionParams has empty documentation.
Impact: Low - Documentation quality
Recommendation: Add proper documentation explaining that for KIND_SELL orders, this is the exact sell amount, while for KIND_BUY orders, this represents the maximum collateral to sell with the actual amount calculated based on repayAmount and clearing prices.
| (address onBehalfOfAccount,) = EVC.getCurrentOnBehalfOfAccount(address(0)); | ||
| require(onBehalfOfAccount == account, Unauthorized(onBehalfOfAccount)); | ||
|
|
||
| IERC20 asset = IERC20(IERC4626(vault).asset()); |
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.
Excellent Security Design - Access Control Pattern
Positive Finding: The access control pattern here is well-designed and secure.
Why This Is Good:
- Checks msg.sender == EVC (line 220)
- Checks onBehalfOfAccount == owner (line 222)
- This prevents unauthorized calls while maintaining flexibility
Per CLAUDE.md: "anyone can call the EVC.batch() function" so just checking EVC is insufficient. The onBehalfOfAccount check ensures only the authorized owner can execute the operation.
This addresses previous review concerns about access control.
| // Additionally, we don't transfer this collateral directly to the settlement contract because the settlement contract | ||
| // requires receiving of funds from the user's wallet, and cannot be put in the contract in advance. | ||
| if (params.owner != params.account) { | ||
| require( |
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.
Subaccount Flow Documentation Needed
Issue: Complex subaccount logic lacks comprehensive documentation
Impact: Medium - Code maintainability and auditability
Current Situation:
The code handles subaccount scenarios where params.owner != params.account, transferring collateral from subaccount to owner before settlement. However, the rationale and security considerations are not fully documented.
What Should Be Documented:
- WHY: Subaccounts cannot sign transactions (no private key exists)
- PROBLEM: Settlement contract requires funds from transaction signer
- SOLUTION: Transfer collateral subaccount -> owner wallet before settlement
- SECURITY: Transfer uses solver-controlled clearingPrices for KIND_BUY (but funds stay within user's own addresses)
- VALIDATION: Subaccount ownership check at lines 360-363 ensures owner controls the subaccount
Note: While clearingPrices are solver-controlled, the transfer is between the user's own addresses (account -> owner), so even manipulated prices cannot result in theft (as clarified by the author in previous comments).
|
|
||
| /// @dev Indicates that the close order cannot be executed becuase the necessary pricing data is not present in the `tokens`/`clearingPrices` variable | ||
| error PricesNotFoundInSettlement(address collateralVaultToken, address borrowToken); | ||
|
|
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.
Typo in Error Documentation
"becuase" should be "because"
| // requires receiving of funds from the user's wallet, and cannot be put in the contract in advance. | ||
| if (params.owner != params.account) { | ||
| require( | ||
| bytes19(bytes20(params.owner)) == bytes19(bytes20(params.account)), |
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.
Typo in Comment
"becuase" should be "because"
src/CowEvcClosePositionWrapper.sol
Outdated
| { | ||
| address borrowAsset = IERC4626(borrowVault).asset(); | ||
| (address[] memory tokens, uint256[] memory clearingPrices,,) = abi.decode( | ||
| settleData[4:], (address[], uint256[], CowSettlement.CowTradeData[], CowSettlement.CowInteractionData[][3]) |
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.
Gas Optimization & Validation in _findRatePrices
Missing validations:
- No check that
tokens.length > 0 - No validation that arrays have matching lengths
- Loop continues after both prices are found (wastes gas)
Suggested improvements:
require(tokens.length > 0 && tokens.length == clearingPrices.length, PricesNotFoundInSettlement(collateralVault, borrowAsset));
// Early exit optimization - stop once both prices found
for (uint256 i = 0; i < tokens.length && (collateralVaultPrice == 0 || borrowPrice == 0); i++) {| if (repayAmount > debtAmount) { | ||
| // the user intends to repay all their debt. we will revert if their balance is not sufficient. | ||
| repayAmount = debtAmount; | ||
| } |
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.
Unclear repayAll Logic
The helperRepay function logic could be clearer:
Current behavior:
- Always tries to repay based on owner's balance
- If
repayAmount > debtAmount, caps atdebtAmount
Questions:
- Should this validate that the owner received expected repayment assets from settlement?
- What happens if settlement sent less than expected?
Recommendation: Add documentation explaining:
- Function repays using whatever balance the owner has (from settlement)
- If balance is insufficient for full repayment, it will revert when trying to transfer
- Settlement is responsible for ensuring sufficient tokens reach the owner
| /** | ||
| * @dev | ||
| */ | ||
| uint256 collateralAmount; |
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.
Missing Documentation in ClosePositionParams
The collateralAmount field documentation is empty. This is a critical parameter.
Suggested documentation:
/**
* @dev The amount of collateral vault shares to sell. For KIND_SELL orders, this is the exact sell amount.
* For KIND_BUY orders with subaccounts, this parameter is used as the maximum collateral to sell,
* with the actual amount calculated based on repayAmount and clearing prices from the settlement.
*/| (address onBehalfOfAccount,) = EVC.getCurrentOnBehalfOfAccount(address(0)); | ||
| require(onBehalfOfAccount == account, Unauthorized(onBehalfOfAccount)); | ||
|
|
||
| IERC20 asset = IERC20(IERC4626(vault).asset()); |
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.
Excellent Security Design - Access Control Pattern
✅ Positive Finding: The access control pattern here is well-designed and secure:
- Checks
msg.sender == EVC(line 220) - Checks
onBehalfOfAccount == owner(line 222)
Per CLAUDE.md: "anyone can call the EVC.batch() function" so just checking EVC is insufficient. The onBehalfOfAccount check ensures only the authorized owner can execute the operation.
This properly addresses access control concerns.
| // Additionally, we don't transfer this collateral directly to the settlement contract because the settlement contract | ||
| // requires receiving of funds from the user's wallet, and cannot be put in the contract in advance. | ||
| if (params.owner != params.account) { | ||
| require( |
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.
Subaccount Flow Documentation Needed
The subaccount logic is complex but lacks comprehensive documentation.
What should be documented:
- WHY: Subaccounts cannot sign transactions (no private key exists)
- PROBLEM: Settlement contract requires funds from transaction signer
- SOLUTION: Transfer collateral subaccount → owner wallet before settlement
- SECURITY: Transfer uses solver-controlled
clearingPricesfor KIND_BUY (but funds stay within user's own addresses) - VALIDATION: Subaccount ownership check at lines 360-363 ensures owner controls the subaccount
Note: While clearingPrices are solver-controlled, the transfer is between the user's own addresses (account → owner), so even manipulated prices cannot result in theft (as clarified by the author in previous comments).
|
|
||
| /// @dev Indicates that the close order cannot be executed becuase the necessary pricing data is not present in the `tokens`/`clearingPrices` variable | ||
| error PricesNotFoundInSettlement(address collateralVaultToken, address borrowToken); | ||
|
|
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.
Typo: "becuase" should be "because"
| /// @dev Indicates that the close order cannot be executed because the necessary pricing data is not present in the `tokens`/`clearingPrices` variable |
| ClosePositionParams memory params | ||
| ) internal { | ||
| // If a subaccount is being used, we need to transfer the required amount of collateral for the trade into the owner's wallet. | ||
| // This is required becuase the settlement contract can only pull funds from the wallet that signed the transaction. |
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.
Typo: "becuase" should be "because"
| // This is required becuase the settlement contract can only pull funds from the wallet that signed the transaction. | |
| // This is required because the settlement contract can only pull funds from the wallet that signed the transaction. |
| /** | ||
| * @dev | ||
| */ | ||
| uint256 collateralAmount; |
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.
Missing Documentation: The collateralAmount field lacks proper documentation. Based on code analysis:
For KIND_SELL orders, this is the exact amount of collateral vault shares to sell. For KIND_BUY orders with subaccounts (lines 381-385), this value is used when params.kind == KIND_SELL and a calculated transferAmount is used when params.kind == KIND_BUY.
| uint256 collateralAmount; | |
| /** | |
| * @dev The amount of collateral vault shares to sell or transfer. | |
| * For KIND_SELL orders, this is the exact sell amount. | |
| * For KIND_BUY orders with subaccounts, this is ignored and transfer amount is calculated from repayAmount. | |
| */ |
| (address[] memory tokens, uint256[] memory clearingPrices,,) = abi.decode( | ||
| settleData[4:], (address[], uint256[], CowSettlement.CowTradeData[], CowSettlement.CowInteractionData[][3]) | ||
| ); | ||
| for (uint256 i = 0; i < tokens.length; i++) { |
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.
Gas Optimization & Missing Validation: This function should include input validation and optimize gas by early-exiting the loop:
| for (uint256 i = 0; i < tokens.length; i++) { | |
| for (uint256 i = 0; i < tokens.length && (collateralVaultPrice == 0 || borrowPrice == 0); i++) { |
Additionally, consider adding validation before the loop:
require(tokens.length > 0 && tokens.length == clearingPrices.length, PricesNotFoundInSettlement(collateralVault, borrowAsset));This prevents potential issues with empty arrays and mismatched array lengths.
| // Additionally, we don't transfer this collateral directly to the settlement contract because the settlement contract | ||
| // requires receiving of funds from the user's wallet, and cannot be put in the contract in advance. | ||
| if (params.owner != params.account) { | ||
| require( |
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 Needed: The subaccount flow is complex and could benefit from more comprehensive inline documentation explaining:
- Why: Subaccounts cannot sign transactions (no private key exists)
- Problem: Settlement contract requires funds from transaction signer
- Solution: Transfer collateral from subaccount → owner wallet before settlement
- Security: Subaccount ownership validated at lines 374-377
- Note: Per author's clarification, while
clearingPricesare solver-controlled, the transfer is between user's own addresses (account → owner), so price manipulation cannot result in theft
Consider adding a comprehensive comment block here to aid future auditors and developers.
| uint256 debtAmount = IBorrowing(vault).debtOf(account); | ||
|
|
||
| // repay as much debt as we can | ||
| uint256 repayAmount = asset.balanceOf(owner); |
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.
Code Quality: The repayment logic could be clearer. Consider adding documentation:
/// @dev This function repays debt using the owner's balance of the vault's underlying asset.
/// The CoW settlement must have transferred sufficient repayment assets to the owner's wallet.
/// If the owner's balance exceeds the debt, only the debt amount is repaid (full repayment with dust).
/// Any excess after repayment is left in the owner's wallet.
uint256 repayAmount = asset.balanceOf(owner);This clarifies that the settlement is responsible for ensuring sufficient tokens reach the owner.
| onBehalfOfAccount: params.account, | ||
| targetContract: params.borrowVault, | ||
| value: 0, | ||
| data: abi.encodeCall(IBorrowing.borrow, (params.borrowAmount, params.owner)) |
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.
Security Consideration - Settlement Atomicity: The borrowed funds are sent to params.owner wallet before the CoW settlement executes (settlement happens in the next batch item at lines 215-220).
Current Protections:
✅ EVC batch atomicity - entire batch reverts if any step fails
✅ Account health check at batch end (line 222-224 comment)
✅ Trust that solver configures settlement correctly
Recommendation: Consider adding NatSpec documentation explaining:
- This flow relies on EVC batch atomicity
- If settlement fails or is misconfigured, the ENTIRE batch reverts (no partial execution)
- Account health check prevents undercollateralization
- The settlement MUST swap the borrowed assets back to collateral as expected
This is especially important if the loan leaves the position close to liquidation threshold.
| (address onBehalfOfAccount,) = EVC.getCurrentOnBehalfOfAccount(address(0)); | ||
| require(onBehalfOfAccount == account, Unauthorized(onBehalfOfAccount)); | ||
|
|
||
| IERC20 asset = IERC20(IERC4626(vault).asset()); |
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.
Excellent Access Control Pattern: This dual-check security pattern is well-designed:
- Line 224: Checks
msg.sender == EVC - Line 226: Checks
onBehalfOfAccount == account
Per CLAUDE.md: "anyone can call the EVC.batch() function" so just checking EVC is insufficient. The onBehalfOfAccount check ensures only the authorized owner can execute the operation through the proper EVC batch context.
This properly addresses access control concerns raised in earlier reviews.
| SafeERC20Lib.safeTransferFrom(asset, owner, address(this), repayAmount, address(0)); | ||
|
|
||
| // repay what was requested on the vault | ||
| asset.approve(vault, repayAmount); |
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.
Code Quality - Approval Pattern: The approval here is set to repayAmount (limited approval) which is good security practice, unlike some vault patterns that use infinite approval.
However, consider documenting why this is safe:
// Limited approval: only approve the exact amount needed for this specific repayment
// This minimizes risk if the vault contract has unexpected behavior
asset.approve(vault, repayAmount);| /// from collateralVault -> IERC20(borrowVault.asset()). The recipient of the | ||
| /// swap should *THIS* contract so that it can repay on behalf of the owner. Furthermore, | ||
| /// the order should be of type GPv2Order.KIND_BUY to prevent excess from being sent to the contract. | ||
| /// If a full close is being performed, leave a small buffer for intrest accumultation, and the dust will |
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.
Typo: "intrest" should be "interest"
| /// If a full close is being performed, leave a small buffer for intrest accumultation, and the dust will | |
| /// If a full close is being performed, leave a small buffer for interest accumulation, and the dust will |
| /// 2. Repay debt and return remaining assets to user | ||
| /// @dev The settle call by this order should be performing the necessary swap | ||
| /// from collateralVault -> IERC20(borrowVault.asset()). The recipient of the | ||
| /// swap should *THIS* contract so that it can repay on behalf of the owner. Furthermore, |
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.
Grammar: "should THIS contract" should be "should be THIS contract"
| /// swap should *THIS* contract so that it can repay on behalf of the owner. Furthermore, | |
| /// swap should be *THIS* contract so that it can repay on behalf of the owner. Furthermore, |
Description
We collapse the original
CowEvcWrappercontract--a highly generalized approach--to very narrow purpose-built wrapper contracts in order to improve security and gas efficiency.Context
After a lot of time investigating the and reviewing the possible use cases for the
CowEvcWrapper, it was identified that our actual needed intial usecases are fairly narrow. A user only needs to be able to open and close a leveraged position, so we provide the EVC flows just for that and the frontend can build these flows by using the helper functions.General expected frontend flow (example for OpenPosition flow--ClosePosition flow is mostly the same):
1 User selects the parameters for their long position and clicks "submit"
2. The frontend computes the
OpenPositionParamsbased on the user's input and callsgetSignedCalldata()to get theEVC.permit()call to sign3. User signs the EVC permit using their wallet as usual
4. The wrapper data is created with
abi.encode(params, signature)with the data from step 2 and 35. A second signature is requested for the user to execute the CoW
settleorder6. any approvals that may be required are accumulated
7. CoW solver processes the order with wrapper.
(a replica of this flow can be seen in the
test_OpenPositionWrapper_Successortest_ClosePositionWrapper_Successe2e testConsiderations
ClosePositionwrapper will leave the user's remaining collateral in their accountClosePositionwrapper, if doing a full repayment (fully closing a position), there may be an amount of the debt token left over after repayment. If this happens, we currently send this amount back to the user's wallet. However, maybe there is a better way to handle this situation? Or is it ok to not worry so much about it?Out of Scope
The wrapper itself is not for this PR (even if the wrappers appear to be added because they were copied over). For that, see #6 and #7
Testing Instructions
Follow the test command instructions in the README
We want to make sure: