Respond to initial audit feedback

[kaze-cow]

Description

We received feedback from the auditors. This PR contains commits to resolve any findings that they had.

Out of Scope

This is only covering changes to the code; any changes to the docs will be done as part of the #20 PR (separately committed, tbd)

Testing Instructions

Browse through the audit findings. I included commit hashes with every fix that was implemented. If anything seems amiss please feel free to comment either here (for specific feedback) or on the finding itself (for more general/overarching choices).

[fedgiac]

Addressed audit comments are ticked if they are satisfactorily addressed. Those not ticked have a comment.

• EIP-712 Type String Mismatch for CollateralSwapParams
  Let's add tests to make sure this doesn't happen again!
  We could use something like forge eip712 --json ./src/CowEvcCollateralSwapWrapper.sol and --ffi.
• Silent Successful Execution When Next Wrapper Is an EOA
• CowEvcCollateralSwapWrapper Never Disables the Old Collateral Vault
• Unrestored Memory Slot After In-Place EIP-712 Hash in Inbox.isValidSignature
• Events Emit User-Signed Minimum Amounts Instead of Actual Executed Amounts
• Pre-Approved Hash Flow Silently Revokes Operator Authorization Without Documentation
• ecrecover Return Value Not Checked
  We fixed this but we didn't test the fix.
• Stale Nonce Embedded in EVC Permit Calldata Breaks Wrappers That Require Permission in Both Phases
• InboxFactory Does Not Validate EVC Subaccount Relationship
• setPreApprovedHash(false) Transitions Unapproved Hashes Directly to CONSUMED
• Error Is Defined but Never Thrown
• Interest Accrual Between Signing and Execution Can Leave a Residual Debt After Close
• wrappedSettle does not bind signed wrapper parameters to settleData
• Unused constants
• Inbox hardcodes settlement domain separator
  I don't think this is addressed correctly. There's a good point that the domain separator could be different and we shouldn't just hardcode it if we can avoid it. And I think here we can: the inbox factory can read the domain separator, make it immutable, and then pass it to the inbox as a deployment parameter. Does it make sense?
• Unconditional max approval to the vault relayer
• toAmount parameter in CowEvcCollateralSwapWrapper is unused
• _evcInternalSettle reverts when asset equals collateralVault
  See suggestion in comment. The code addresses the issue but I'd like to avoid unnecessary validations if possible.
• Interface definitions located with implementation reduces discoverability for integrators
  Not addressed. I agree with not addressing. Will let discussion with auditor to you for making sure we didn't miss anything.
• Missing onchain signal when close results in a partial repay
  Don't have a strong opinion, I'll leave the decision on addressing this to you.
• Missing Euler vault validation in open/close wrappers
  Mmh. I like the idea of checks but I don't really see the benefit, as you mention in the comment. Your choice on what to do.
• _evcInternalSettle swap output check can pass without any settlement output
• _evcInternalSettle emits collateralAmount as input not collateral spent
• wrappedSettle does not bind signed wrapper parameters to settleData
  It was a documented design choice to ddo that. It has clear risks and drawbacks, but I don't think the audit surfaces new issues. I don't actually get their code change suggestion.
• verifyAndBuildWrapperData Silently Omits Two Promised Safety Checks
  Not addressed. Should be addressed.

[kaze-cow]

specific comments on the uncheckeds

• EIP-712 Type String Mismatch for CollateralSwapParams

 Let's add tests to make sure this doesn't happen again!
We could use something like `forge eip712 --json ./src/CowEvcCollateralSwapWrapper.sol` and `--ffi`.

Good idea d6f94d9

• ecrecover Return Value Not Checked

 We fixed this but we didn't test the fix.

added test 69d4842

• Inbox hardcodes settlement domain separator

 I don't think this is addressed correctly. There's a good point that the domain separator could be different and we shouldn't just hardcode it if we can avoid it. And I think here we can: the inbox factory can read the domain separator, make it immutable, and then pass it to the inbox as a deployment parameter. Does it make sense?

I'm mentally overloaded 244566b

• _evcInternalSettle reverts when asset equals collateralVault

 See suggestion in comment. The code addresses the issue but I'd like to avoid unnecessary validations if possible.

• Interface definitions located with implementation reduces discoverability for integrators

 Not addressed. I agree with not addressing. Will let discussion with auditor to you for making sure we didn't miss anything.

Auditor confirms they will mark it as "Acknowledged"

• Missing onchain signal when close results in a partial repay

 Don't have a strong opinion, I'll leave the decision on addressing this to you.

Auditor likes my suggestion, will mark it as "fixed" with the comments I added d094d3c

• Missing Euler vault validation in open/close wrappers

 Mmh. I like the idea of checks but I don't really see the benefit, as you mention in the comment. Your choice on what to do.

Asked darek about this, he confirms its probably best to leave it open as long as we are OK with the potential risks of griefing. The support for any EVC vault is very useful.

• wrappedSettle does not bind signed wrapper parameters to settleData

 It was a documented design choice to ddo that. It has clear risks and drawbacks, but I don't think the audit surfaces new issues. I don't actually get their code change suggestion.

I left a comment, improved the documentation, and the auditors marked it as fixed already. So seems good d2e453f

• verifyAndBuildWrapperData Silently Omits Two Promised Safety Checks

 Not addressed. Should be addressed.

There is sort of two parts to this 1) they noted the two safety checks should be removed (they were--or so I thought--I removed them now fc7fad3 ) and 2) check that the code is nonzero. I have been going back and forth on (2), and ultimately added it in because it serves as a way to protect against accidentally calling solvers (which are usually EOAs and therefore could concievably be missed by the checks)

all changes in fc7fad3

[kaze-cow]

_evcInternalSettle reverts when asset equals collateralVault

Reverted my original fix for this change, moved the source balance return to the end, but did not add any officially supported tests as we don't expect this case to be supported in practice.

[fedgiac]

• EIP-712 Type String Mismatch for CollateralSwapParams

  Nice job, the result looks much nicer than I thought!

• verifyAndBuildWrapperData Silently Omits Two Promised Safety Checks
  Still technically not addressed, note that for addressing it's enough to just remove the following two incorrect NatSpec lines.

  ///      3. Verifies all wrappers use the same settlement contract (from first wrapper's SETTLEMENT)
  ///      4. Verifies the settlement contract is not authenticated as a solver
  

All findings in the audit have been properly implemented or acknowledged without changes (except the above, which is a trivial change to fix).
Approving in advance since the few remaining changes are mostly inconsequential.

Good job addressing all the comment and passing the audit successfully!