feat: working security #2
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
There was a problem hiding this comment.
@kaze-cow if this needs review, you need to remember to add @cowprotocol/contracts for review
Also, you need to work a bit more on the description to really. We don't enforce a template for PRs, but if it helps we can, what I need to be able to do with reading the code is
- find a easy to read description of what is this PR doing
- Giving some additional context. The less you assume the reviewer knows the better. In your description. The description you added could introduce more to the problem and not take things for granted like what is the "skimming the euler vault"
- Making sure the scope is clear (what changes belong and what changes doesn't belong). You should be solving one thing only and solving it well
- Instructions how to test or review, including any commands or hints on what to pay attention to
need to account for the
Seems unfinished phrase
All PRs should explain what is it about. "working security" needs clarification.
Also, "test file CowEvcWrapper.t.sol to see the bulk of the expected process" is also not good intro for the PR. You want the revieewer to read this file to know what this PR is about? Also, this file existed and just have a few changes.
src/CowEvcWrapper.sol
Outdated
| data: abi.encodeCall(this.internalSettle, (tokens, clearingPrices, trades, interactions)) | ||
| }); | ||
|
|
||
| // immediately after processing the swap, we should be skimming the result to the user |
There was a problem hiding this comment.
Does this big commented code belong to this PR?
kaze-cow
force-pushed
the
feat/initial
branch
from
3319241 to
ec9d67d
Compare
feat: use new wrapper from upstream
look to the test file
CowEvcWrapper.t.solto see the bulk of the expected processadds the following fixes:
we still have the capability to surround the call with a wrapper that matches the call of the current, though recent discussions have pointed towards setting up these ewrappers so there can be multiple (in wihch case, the call signature would likely have to change...)
still a bit of a mess. cleanup will probalby be the next iteration.