Conversation
fedgiac
left a comment
There was a problem hiding this comment.
-
#1Wrappers lack sufficient post-settlement validation
No changes, no fixes needed. -
#2(Withdrawn by auditor, no change needed.) -
#3evcInternalSettle() caller check could additionally verify onBehalfOfAccount
Discussed privately. This is relatively cheap, it isn't really needed for the current wrappers but it's a nice thing to have in case a future wrapper inadvertently needs this assumption.diff --git a/src/CowEvcBaseWrapper.sol b/src/CowEvcBaseWrapper.sol index e65ec56..9ad2c74 100644 --- a/src/CowEvcBaseWrapper.sol +++ b/src/CowEvcBaseWrapper.sol @@ -139,6 +139,8 @@ abstract contract CowEvcBaseWrapper is CowWrapper, PreApprovedHashes { bytes calldata remainingWrapperData ) external { require(msg.sender == address(EVC), Unauthorized(msg.sender)); + (address onBehalfOf,) = EVC.getCurrentOnBehalfOfAccount(address(0)); + require(onBehalfOf == address(this)); require(expectedEvcInternalSettleCallHash == keccak256(msg.data), InvalidCallback()); expectedEvcInternalSettleCallHash = bytes32(0);
Tests still need to be updated.
-
#4Misleading comments about remainingWrapperData in _evcInternalSettle()
Fixed, no code changes needed. -
#5Non-conformant EIP-712 struct hashing in Inbox.isValidSignature()
See comment about improving docs. No code change is really needed, neither for security not for proper 712 encoding; from the user's perspective this is a valid 712 signature.
If we want to make it easier for the integrator (Euler) to create valid signatures, we could create a dedicated struct that contains only the parameters that are actually useful (and excluding things like the balance source) and then we maually populate memory before hasing. Not needed thought, feel free to skip that. -
#6Close position wrapper repay amount uses existing balance instead of delta
Fixed, only comment change needed.
after testing its established that the wrapper would only consume 1-2k more gas with the additional security check in place.
After some thought, since we already have the integration coming down the pipe on the other team's side and this would be come a very much last minute thing increasing review overhead and possibly delaying the launch, decided to skip this for now. I agree its good to have, and we can save this as an improvement for the next version of the wrapper when the time comes 👍 |
Description
Fixes for the second round of audit. this mostly just comes down to some comments fixes/inaccuracies.
Testing Instructions
Read up on the proposed audit fix items and determine if they are sufficiently addressed.