-
Notifications
You must be signed in to change notification settings - Fork 0
feat: euler working poc #1
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
anxolin
left a comment
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.
Generally looks great!
Some general feedback:
- The PR is way too big. It adds too many things at once. We should seek smaller PRs to make sure reviews happen earlier
- I didn't get yet how this thing will be secure. If the transient storage is done in hook, how we make sure this is secure?
- In the description you reference the PoC. It would be good to give some pointer so readers have the right context.
- Added a few other nits and comments in the review
test/CowEvcWrapperTest.t.sol
Outdated
| import {CowEvcWrapper, GPv2Trade, GPv2Interaction} from "../src/CowEvcWrapper.sol"; | ||
| import {GPv2AllowListAuthentication} from "cow/GPv2AllowListAuthentication.sol"; | ||
| //import {GPv2Settlement} from "cow/GPv2Settlement.sol"; | ||
| //import {GPv2Interaction, GPv2Trade} from "../../src/vendor/interfaces/IGPv2Settlement.sol"; |
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.
dead code
| } | ||
|
|
||
| function test_batchWithSettle_Empty() external { | ||
| vm.skip(bytes(FORK_RPC_URL).length == 0); |
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.
Because so many dependencies, I guess its hard to not depend on forking right?
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.
couple things I have found WRT fork tests:
- If you lock a block number, fork tests can be just as performant and insightful as unit tests for many needs. And oftentimes easier to set up. That is the case here and its indeed been convenient
- As you say, setting up mocks for large, many dependency projects like this can be a challenge (though foundry has been finding ways to make this easier, so with the help of AI (cough cough) can probably crank these out)
in a PR following the security PR my priority will be to use Unit tests to cover any uncovered code as a next step.
| ); | ||
| } | ||
|
|
||
| function test_leverage_WithCoWOrder() external { |
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.
Probably this test should get a bit more of explanatation. Its a quite complex use case, so probably we should find a way to simplify it (so it has less lines of code) and also explain exactly what is doing (explain the use case, and why those EVC calls are needed).
Also, ideally nice to connect it more to how this could be used. The update of the transient data will be done in a hook (according to the comments). In general, would be great to see how this connects to how this will be used in reality.
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.
yea this should probably go in its own file actually. its the core of the whole PoC.
Will also link the diagram I sent yesterday for reference 👍
|
re @anxolin
whoops! yea my focus for this PR was just to take the work you did from the original euler repo and apply it to a fresh slate and minimize
thats because the security changes were put in a separate PR! #2
will do 👍 |
This reverts commit e56eac3.
best way to ensure the expected flow is followed exactly
src/CowEvcWrapper.sol
Outdated
|
|
||
| /// @title CowEvcWrapper | ||
| /// @notice A wrapper around the EVC that allows for settlement operations | ||
| contract CowEvcWrapper { |
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.
I don't mind either way but considering it's the first contract one sees in the explorer I think it makes sense it mentions the settlement contract. Maybe CowEvcSettlement for simplicity? I'd vote against anything like *Proxy though because the word proxy is usually interpreted as "delegatecall shim" but it isn't the case here.
| abstract contract EIP712 { | ||
|
|
||
| bytes32 internal constant _TYPE_HASH = | ||
| keccak256("EIP712Domain(string name,uint256 chainId,address verifyingContract)"); | ||
|
|
||
| bytes32 internal immutable _hashedName; | ||
| string private _name; | ||
| string private _nameFallback; | ||
|
|
||
| /** | ||
| * @dev Initializes the domain separator. | ||
| * | ||
| * The meaning of `name` is specified in | ||
| * https://eips.ethereum.org/EIPS/eip-712#definition-of-domainseparator[EIP 712]: | ||
| * | ||
| * - `name`: the user readable name of the signing domain, i.e. the name of the DApp or the protocol. | ||
| * | ||
| * NOTE: These parameters cannot be changed except through a xref:learn::upgrading-smart-contracts.adoc[smart | ||
| * contract upgrade]. | ||
| */ | ||
| constructor(string memory name) { | ||
| _name = name; | ||
| _hashedName = keccak256(bytes(name)); | ||
| } | ||
|
|
||
| /** | ||
| * @dev Returns the domain separator for the current chain. | ||
| */ | ||
| function _domainSeparatorV4() internal view returns (bytes32) { | ||
| return _buildDomainSeparator(); | ||
| } | ||
|
|
||
| function _buildDomainSeparator() internal view virtual returns (bytes32) { | ||
| return bytes32(0); | ||
| } | ||
|
|
||
| /** | ||
| * @dev Given an already https://eips.ethereum.org/EIPS/eip-712#definition-of-hashstruct[hashed struct], this | ||
| * function returns the hash of the fully encoded EIP712 message for this domain. | ||
| * | ||
| * This hash can be used together with {ECDSA-recover} to obtain the signer of a message. For example: | ||
| * | ||
| * ```solidity | ||
| * bytes32 digest = _hashTypedDataV4(keccak256(abi.encode( | ||
| * keccak256("Mail(address to,string contents)"), | ||
| * mailTo, | ||
| * keccak256(bytes(mailContents)) | ||
| * ))); | ||
| * address signer = ECDSA.recover(digest, signature); | ||
| * ``` | ||
| */ | ||
| function _hashTypedDataV4(bytes32 structHash) internal view virtual returns (bytes32) { | ||
| return ECDSA.toTypedDataHash(_domainSeparatorV4(), structHash); | ||
| } | ||
| } |
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.
Nit: to me, EIP712 just looks like an unneeded extra layer of indirection: it should just made part of SignerECDSA and all indirection involving the virtual _buildDomainSeparator and related function chain should be removed.
* check solver on internalSettle function * check only callable through EVC * prevent reentrancy
they shouldn't have been in the repository
3319241 to
ec9d67d
Compare
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
this PR is a division of previous PR #1 , focusing on just getting vendored dependencies isolated and added to the project
we dont need it because we have the actual cow repository
implements the bulk code expected to be required for an Ethereum Vault Connector integration with CoWSwap through a GPv2Wrapper integration. The code here is based on an original PoC developed by @anxolin https://github.com/anxolin/evk-periphery . Compared to the PoC, it has these major changes:
setEvcCalls, which sets transient data for use in the next callsettle()function (same as GPv2Settlement signature) on the EVC wraper contractinternalSettle()which calls the actualGPv2Settlementsettle()internalSettle)To be done in separate PRs: