-
Notifications
You must be signed in to change notification settings - Fork 0
feat: open position wrapper #14
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
base: chore/remove-stuff
Are you sure you want to change the base?
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
|
Claude encountered an error —— View job Code Review In Progress
|
| uint256 borrowAmount | ||
| ); | ||
|
|
||
| constructor(address _evc, ICowSettlement _settlement) CowWrapper(_settlement) { |
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: Missing zero address validation
The constructor doesn't validate that _evc is not the zero address. If deployed with address(0), all evc.batch() calls would fail, making the wrapper completely non-functional.
Consider adding:
require(_evc != address(0), "Invalid EVC address");| // - 32 bytes: signature length | ||
| // - N bytes: signature data (padded to 32-byte boundary) | ||
| // We can just math this out | ||
| uint256 consumed = 224 + 64 + ((signature.length + 31) & ~uint256(31)); |
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: Hardcoded magic number
The value 224 represents the size of OpenPositionParams (7 fields × 32 bytes). This is fragile and error-prone if the struct changes.
Consider using a constant or documenting this more clearly:
// OpenPositionParams size: 7 fields × 32 bytes = 224 bytes
uint256 constant OPEN_POSITION_PARAMS_SIZE = 224;
uint256 consumed = OPEN_POSITION_PARAMS_SIZE + 64 + ((signature.length + 31) & ~uint256(31));The same issue exists on line 145 in the assembly block.
| /// @notice Implementation of GPv2Wrapper._wrap - executes EVC operations to open a position | ||
| /// @param settleData Data which will be used for the parameters in a call to `CowSettlement.settle` | ||
| /// @param wrapperData Additional data containing OpenPositionParams | ||
| function _wrap(bytes calldata settleData, bytes calldata wrapperData, bytes calldata remainingWrapperData) |
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: No validation of critical parameters
The function doesn't validate that:
params.collateralVault != address(0)params.borrowVault != address(0)params.collateralVault != params.borrowVault(using same vault for both could cause issues)params.deadline >= block.timestampwhen using permit signature path (only checked in pre-approved path)
While some of these would fail later in EVC calls, explicit validation provides clearer error messages and fails faster.
| } | ||
|
|
||
| /// @notice Internal settlement function called by EVC | ||
| function evcInternalSettle(bytes calldata settleData, bytes calldata remainingWrapperData) external payable { |
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: Potential reentrancy vulnerability
While the EVC batch ensures atomicity, there's no reentrancy protection on this external function. An attacker could potentially call evcInternalSettle from within a malicious vault contract during the EVC batch execution.
The checks msg.sender == address(EVC) and onBehalfOfAccount == address(this) provide some protection, but consider whether this is sufficient given the complex call flow through EVC.
| /// @notice Abstract contract for managing pre-approved operation hashes | ||
| /// @dev Allows users to pre-approve specific operations without requiring signatures each time | ||
| abstract contract PreApprovedHashes { | ||
| /// @dev Marker value indicating a hash is pre-approved |
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: Missing documentation on security model
The contract uses marker values PRE_APPROVED and CONSUMED_PRE_APPROVED to prevent replay attacks, but this security mechanism isn't well-documented. Consider adding:
/// @dev Three states exist for each (owner, hash) pair:
/// - 0: Not approved
/// - PRE_APPROVED: User has approved this hash, can be consumed once
/// - CONSUMED_PRE_APPROVED: Hash was consumed and cannot be used again (prevents replay attacks)Refactor CowEvcOpenPositionWrapper test files to reduce code duplication and improve maintainability: Integration tests (CowEvcOpenPositionWrapper.t.sol): - Add helper functions for common test setup patterns - _createDefaultParams(): standardized param creation - _setupUserSusdsApproval(): SUSDS approval setup - _setupUserPreApprovedFlow(): pre-approved hash flow setup - _createPermitSignature(): permit signature creation - _encodeWrapperData(): wrapper data encoding - _executeWrappedSettlement(): settlement execution - _verifyPositionOpened(): position verification assertions - Add DEFAULT_BORROW_AMOUNT and DEFAULT_BUY_AMOUNT constants - Reduce test function size by ~50% while maintaining clarity Unit tests (CowEvcOpenPositionWrapper.unit.t.sol): - Add helper functions for common patterns - _getDefaultParams(): returns default params (tests modify as needed) - _getEmptySettleData(): creates empty settlement data - _encodeWrapperData(): wrapper data encoding - _setupPreApprovedHash(): pre-approved hash setup - Add DEFAULT_COLLATERAL_AMOUNT and DEFAULT_BORROW_AMOUNT constants - Remove 4 redundant tests with no coverage impact: - test_Constructor_SetsName (trivial getter) - test_GetApprovalHash_Consistency (deterministic function) - test_ParseWrapperData_WithSignature (duplicate code path) - test_GetSignedCalldata_ReturnsCorrectStructure (implicitly covered) - Reduce from 24 to 20 unit tests with no loss in coverage All tests pass: 6/6 integration tests, 20/20 unit tests 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…otocol/euler-integration-contracts into feat/open-position-wrapper
| // - 32 bytes: signature length | ||
| // - N bytes: signature data (padded to 32-byte boundary) | ||
| // We can just math this out | ||
| uint256 consumed = 224 + 64 + ((signature.length + 31) & ~uint256(31)); |
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.
Maintainability: Magic number should be a named constant
The value 224 represents OpenPositionParams size (7 fields × 32 bytes). This should be a named constant to prevent bugs if the struct changes:
| uint256 consumed = 224 + 64 + ((signature.length + 31) & ~uint256(31)); | |
| // OpenPositionParams size: 7 fields × 32 bytes = 224 bytes | |
| uint256 constant OPEN_POSITION_PARAMS_SIZE = 224; | |
| uint256 consumed = OPEN_POSITION_PARAMS_SIZE + 64 + ((signature.length + 31) & ~uint256(31)); |
The same constant should replace the hardcoded 224 on line 145.
| // Decode wrapper data into OpenPositionParams | ||
| OpenPositionParams memory params; | ||
| bytes memory signature; | ||
| (params, signature,) = _parseOpenPositionParams(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.
Code Quality: Missing parameter validation
Consider validating critical parameters before proceeding:
| (params, signature,) = _parseOpenPositionParams(wrapperData); | |
| (params, signature,) = _parseOpenPositionParams(wrapperData); | |
| // Validate critical parameters | |
| require(params.collateralVault != address(0), "Invalid collateral vault"); | |
| require(params.borrowVault != address(0), "Invalid borrow vault"); | |
| require(params.collateralVault != params.borrowVault, "Vaults cannot be the same"); |
While these would fail later in EVC calls, explicit validation provides clearer error messages and fails faster.
| // 1. There are two ways this contract can be executed: either the user approves this contract as | ||
| // and operator and supplies a pre-approved hash for the operation to take, or they submit a permit hash | ||
| // for this specific instance | ||
| if (!isPreApproved) { |
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: Inconsistent deadline validation
The pre-approved hash path validates params.deadline >= block.timestamp (line 207), but the permit signature path doesn't. While the permit call will handle this, consider adding explicit validation for consistency:
| if (!isPreApproved) { | |
| if (!isPreApproved) { | |
| require(params.deadline >= block.timestamp, OperationDeadlineExceeded(params.deadline, block.timestamp)); | |
| items[itemIndex++] = IEVC.BatchItem({ |
|
|
||
| /// @notice Storage indicating whether or not a signed calldata hash has been approved by an owner | ||
| /// @dev Maps owner -> hash(signedCalldata) -> approval status | ||
| mapping(address => mapping(bytes32 => uint256)) public preApprovedHashes; |
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: Expand explanation of three-state model
The three-state security model (0 / PRE_APPROVED / CONSUMED_PRE_APPROVED) is clever for preventing replay attacks, but could be better documented:
| mapping(address => mapping(bytes32 => uint256)) public preApprovedHashes; | |
| /// @notice Storage indicating whether or not a signed calldata hash has been approved by an owner | |
| /// @dev Maps owner -> hash(signedCalldata) -> approval status | |
| /// Three states exist for each (owner, hash) pair: | |
| /// - 0: Not approved (default state) | |
| /// - PRE_APPROVED: User has approved this hash, can be consumed once | |
| /// - CONSUMED_PRE_APPROVED: Hash was consumed and cannot be used again (prevents replay attacks) | |
| /// Once consumed, the hash cannot be re-approved or revoked (permanent consumption) | |
| mapping(address => mapping(bytes32 => uint256)) public preApprovedHashes; |

Description
Implements the
CowEvcOpenPositionWrapperin order to satisfy a usecase for the Euler integration.Context
Read up on notion
Considerations
collateralAmountto 0)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:
fixes https://linear.app/cowswap/issue/COW-65/initial-merge-contract-of-cowevcopenpositionwrapper