ERC4626: Allow overriding underlying assets transfer mechanisms

[Amxx]

Fixes #5960

PR Checklist

• Tests
• Documentation
• Changeset entry (run npx changeset add)

[changeset-bot]

🦋 Changeset detected

Latest commit: ea74661

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

[coderabbitai]

Walkthrough

• Adds a changeset entry noting a minor update and documenting an ERC4626 enhancement.
• In ERC4626.sol, introduces two new internal virtual hooks:
  • _transferIn(address from, uint256 assets)
  • _transferOut(address to, uint256 assets)
• Replaces direct SafeERC20 transfers with these hooks:
  • _deposit now calls _transferIn instead of SafeERC20.safeTransferFrom(...)
  • _withdraw now calls _transferOut instead of SafeERC20.safeTransfer(...)
• Deposit flow performs asset transfer before minting shares; withdraw flow burns shares before transferring assets.
• No external/public API signatures changed; additions are internal.

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title Check	✅ Passed	The title succinctly describes the primary purpose of the pull request by indicating that ERC4626 now allows overriding underlying asset transfer mechanisms, reflecting the addition of internal hooks for transfer customization.
Linked Issues Check	✅ Passed	The changes introduce the requested internal functions _transferIn and _transferOut and replace direct token transfers with these hooks in deposit and withdraw, directly satisfying the coding objectives of issue #5960 for overrideable asset transfers.
Out of Scope Changes Check	✅ Passed	All modifications are focused on introducing and integrating the new transfer hooks in ERC4626 and updating documentation, with no unrelated or out-of-scope alterations present.
Description Check	✅ Passed	The pull request description references the linked issue, lists relevant checklist items, and is directly related to the code changes implementing overrideable transfer logic, making it on-topic and sufficient for this lenient check.
Docstring Coverage	✅ Passed	No functions found in the changes. Docstring coverage check skipped.

✨ Finishing touches

🧪 Generate unit tests (beta)

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

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

contracts/token/ERC20/extensions/ERC4626.sol (1)

302-310: Well-designed hooks with clear default implementations.

The introduction of _transferIn and _transferOut as internal virtual functions elegantly solves the issue raised in #5960. Key strengths:

• The virtual modifier enables customization without forcing implementers to skip super._deposit/super._withdraw
• Default implementations maintain backward compatibility using SafeERC20
• Clear function names that indicate directionality
• Proper separation of concerns

Optional: Consider enhancing documentation.

While the current brief comments are adequate, you could optionally add more detailed NatSpec documentation for these hooks, such as:

-    /// @dev Performs a transfer in of underlying assets. The default implementation uses `SafeERC20`. Used by {_deposit}.
+    /**
+     * @dev Performs a transfer in of underlying assets from `from` to the vault.
+     *
+     * The default implementation uses `SafeERC20.safeTransferFrom`. Used by {_deposit}.
+     *
+     * IMPORTANT: When overriding this function:
+     * - Ensure assets are securely transferred or accounted for
+     * - Consider updating {totalAssets} if assets are held elsewhere (e.g., staking)
+     * - Maintain reentrancy safety by preserving transfer-before-mint order in {_deposit}
+     * - Update preview functions if the override affects conversion rates
+     */

Additionally, the contract header's override guidance (lines 52-68) could mention these hooks as another customization point alongside _deposit and _withdraw.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 765b288 and ea74661.

📒 Files selected for processing (2)

• .changeset/spotty-plums-brush.md (1 hunks)
• contracts/token/ERC20/extensions/ERC4626.sol (2 hunks)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)

• GitHub Check: slither
• GitHub Check: tests-upgradeable
• GitHub Check: coverage
• GitHub Check: tests-foundry
• GitHub Check: tests
• GitHub Check: Redirect rules - solidity-contracts
• GitHub Check: halmos

🔇 Additional comments (3)

contracts/token/ERC20/extensions/ERC4626.sol (2)

270-270: LGTM! Clean refactor enabling customization.

The replacement of the direct SafeERC20.safeTransferFrom call with _transferIn successfully achieves the PR objective of making asset transfers overridable. The order of operations (transfer before mint) is preserved, maintaining the reentrancy safety properties documented in the comment above.

---

297-297: LGTM! Clean refactor enabling customization.

The replacement of the direct SafeERC20.safeTransfer call with _transferOut successfully achieves the PR objective of making asset transfers overridable. The order of operations (burn before transfer) is preserved, maintaining the reentrancy safety properties documented in the comment above.

.changeset/spotty-plums-brush.md (1)

1-5: LGTM! Accurate changeset documentation.

The changeset correctly:

• Classifies this as a minor version bump, which is appropriate for adding new internal virtual functions while maintaining backward compatibility
• Provides a clear, concise description of the enhancement
• Accurately identifies the two new functions (_transferIn and _transferOut)