Refactor ERC4626-maxWithdraw to reflect other functions overrides

[Amxx]

@cairoeth WDYT?

PR Checklist

• [x] Changeset entry (run npx changeset add)

[changeset-bot[bot]]

🦋 Changeset detected

Latest commit: 778019f738c900df5b3eff5d27417a2f6a0b09d7

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

[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

[coderabbitai[bot]]

[!NOTE]

Reviews paused

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Walkthrough

• Updates changeset to bump openzeppelin-solidity minor version.
• IERC4626.sol: fixes a docstring typo in previewRedeem (“redeemption” → “redemption”); no signature changes.
• ERC4626.sol: adds a header note on overriding deposit/withdraw and clarifies maxWithdraw vs. maxRedeem relationship.
• Replaces dev comments with “/// @inheritdoc IERC4626” across public methods; no behavior change from documentation updates.
• Functional adjustment: maxWithdraw(owner) now uses previewRedeem(maxRedeem(owner)) instead of _convertToAssets(balanceOf(owner), Math.Rounding.Floor).
• No exported/public interfaces modified.

Pre-merge checks (2 passed, 1 inconclusive)

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Description Check	❓ Inconclusive	The description currently only addresses a reviewer and lists a checklist item without summarizing the pull request changes, making it too vague to convey meaningful information about the modifications.	Please expand the description to include a concise summary of the key changes, their purpose, and any relevant background or impact details.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title Check	✅ Passed	The title succinctly identifies the primary change of refactoring the ERC4626 maxWithdraw implementation to align with other function overrides and clearly communicates the main objective of the pull request.
Docstring Coverage	✅ Passed	No functions found in the changes. Docstring coverage check skipped.

[!TIP]

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

• Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
• Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Example:

reviews:
  pre_merge_checks:
    custom_checks:
      - name: "Undocumented Breaking Changes"
        mode: "warning"
        instructions: |
          Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).

Please share your feedback with us on this Discord post.

✨ Finishing Touches

🧪 Generate unit tests

• [ ] Create PR with unit tests
• [ ] Post copyable unit tests in a comment

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[Amxx]

@coderabbitai pause

[coderabbitai[bot]]

✅ Actions performed

Reviews paused.