Refactor ERC4626-maxWithdraw to reflect other functions overrides

[Amxx]

@cairoeth WDYT?

PR Checklist

• Changeset entry (run npx changeset add)

[changeset-bot]

🦋 Changeset detected

Latest commit: 757a25f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
openzeppelin-solidity	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[Amxx]

This is the change I propose.

In the case of ERC4626Fees, the fees inclusion in previewRedeem woudl reflect here.
Additionally, if maxRedeem is ever overriden, it would reflect here (but the inverse is not true)

This is a breaking change

[ernestognw]

On second thought (thanks @cairoeth), I don't think the changelog entry is enough. It seems like there are multiple cases of ERC4626 vaults that override previewRedeem, which would affect the logic in maxWithdraw.

To me, it's not a huge deal because I can't see how it may affect the vault critically. If we continue with this change, we should discuss whether a changelog entry under the "Breaking Changes" section is good enough, or whether the change will cause an issue in some projects.

[cairoeth]

Following up on ^, the initial problem is that maxRedeem and maxWithdraw do not consider fees in the ERC4626Fees implementation -- I was thinking that a solution is just overriding maxRedeem in ERC4626Fees with something like this PR, instead of making changes in ERC4626. I mainly saw it as a problem in the ERC4626Fees implementation rather that in ERC4626. Based on the examples @ernestognw provided, I think this PR would make it easier for developers to mistakenly modify maxWithdraw by overriding previewRedeem (here is a good example), whereas before they weren't really connected as maxWithdraw just used _convertToAssets.

All in all, I think the changes in the PR are good but imo we can avoid a breaking change by applying them directly in ERC4626Fees

[ernestognw]

Opened #5135 as an alternative. It would make sense to include the maxWithdraw override in ERC4626 given that anyone adding fees would need to override maxWithdraw as well, and it's perhaps not obvious.

I'm not sure that's worth documenting. The "raw" ERC4626 shouldn't have any concerns about fees, and overriding functions are already assumed under the developer's risk.

[pcaversaccio]

So @cairoeth and myself quickly discussed this change, and if you read the maxWithdraw section of the EIP-4626 carefully, it states:

Now given the changeset for this PR, you could override previewRedeem with a function that reverts and thus it disables maxWithdraw and is hence not anymore EIP-4626 compliant. Note, that previewRedeem is allowed to revert:

MAY revert due to other conditions that would also cause redeem to revert.

Under these circumstances, I would not recommend implementing this change.

[Amxx]

@pcaversaccio

Now given the changeset for this PR, you could override previewRedeem with a function that reverts and thus it disables maxWithdraw and is hence not anymore EIP-4626 compliant.

My deep feeling is that withdraw and redeem are two ways of expressing the same operation. Its like saying "change 1$ into €" vs saying "change 0.91€ worth of $ into €". You give the number differently, but its essentially the same thing. I'd be curious why anyone would want to disable withdraw while keeping redeem functionnal. IMO it makes no sens.

This PR would increass the coupling between the two.

To me the consequence of this coupling are the following:

• Someone that want to change the behvarior of both functions only has one function to override, and both behavior would be consistent with one another. We've example of code where one of the override was missed, and you end up with undesired inconsistent behavior.
• Someone that wants to implement an inconsistency between the two function can still do it, but it may require 2 overrides instead of one.

It is my personal opinion that the first point (wanting consistency) is way more common (and desirable), and this is the one we should favor in our designs. We also identified instances of buggy vaults that would not have been buggy if this PR had been implemented.
I also think that the second point (implement inconsistent behavior between redeem and withdraw) is going to be an edge case that should be well documented. As such, I think asking the dev to override both side so that the inconsistency appears clearly is a good thing.

That is why I continue to support this change.

If you can find any example of real production code (not POCs) that this would break, I'd love to study them!

[Amxx]

This is likelly going to be being delayed, from 5.2 to 5.3.

So far:

1. We do have examples of production contracts where this change would have prevented issues/inconstency.
2. We don't have concrete example of contracts where this change would have caused issues.

We are delaying this because we are waiting for example of the second. At some point we need to stop delaying that more. If 2. doesn't exist, we should merge this PR to benefit from 1..

@frangio @ernestognw @arr00