From 8c3046c958d5e4d96b28906408cf519838b5a16d Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 30 Jul 2024 14:13:21 +0200 Subject: [PATCH 01/11] Refactor ERC4626-maxRedeem to reflect preview functions overrides --- contracts/token/ERC20/extensions/ERC4626.sol | 38 ++++++++++---------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/contracts/token/ERC20/extensions/ERC4626.sol b/contracts/token/ERC20/extensions/ERC4626.sol index c71b14ad48c..7b6adeff271 100644 --- a/contracts/token/ERC20/extensions/ERC4626.sol +++ b/contracts/token/ERC20/extensions/ERC4626.sol @@ -97,77 +97,77 @@ abstract contract ERC4626 is ERC20, IERC4626 { } /** + * @inheritdoc ERC20 + * * @dev Decimals are computed by adding the decimal offset on top of the underlying asset's decimals. This * "original" value is cached during construction of the vault contract. If this read operation fails (e.g., the * asset has not been created yet), a default of 18 is used to represent the underlying asset's decimals. - * - * See {IERC20Metadata-decimals}. */ function decimals() public view virtual override(IERC20Metadata, ERC20) returns (uint8) { return _underlyingDecimals + _decimalsOffset(); } - /** @dev See {IERC4626-asset}. */ + /// @inheritdoc IERC4626 function asset() public view virtual returns (address) { return address(_asset); } - /** @dev See {IERC4626-totalAssets}. */ + /// @inheritdoc IERC4626 function totalAssets() public view virtual returns (uint256) { return _asset.balanceOf(address(this)); } - /** @dev See {IERC4626-convertToShares}. */ + /// @inheritdoc IERC4626 function convertToShares(uint256 assets) public view virtual returns (uint256) { return _convertToShares(assets, Math.Rounding.Floor); } - /** @dev See {IERC4626-convertToAssets}. */ + /// @inheritdoc IERC4626 function convertToAssets(uint256 shares) public view virtual returns (uint256) { return _convertToAssets(shares, Math.Rounding.Floor); } - /** @dev See {IERC4626-maxDeposit}. */ + /// @inheritdoc IERC4626 function maxDeposit(address) public view virtual returns (uint256) { return type(uint256).max; } - /** @dev See {IERC4626-maxMint}. */ + /// @inheritdoc IERC4626 function maxMint(address) public view virtual returns (uint256) { return type(uint256).max; } - /** @dev See {IERC4626-maxWithdraw}. */ + /// @inheritdoc IERC4626 function maxWithdraw(address owner) public view virtual returns (uint256) { - return _convertToAssets(balanceOf(owner), Math.Rounding.Floor); + return previewRedeem(maxRedeem(owner)); } - /** @dev See {IERC4626-maxRedeem}. */ + /// @inheritdoc IERC4626 function maxRedeem(address owner) public view virtual returns (uint256) { return balanceOf(owner); } - /** @dev See {IERC4626-previewDeposit}. */ + /// @inheritdoc IERC4626 function previewDeposit(uint256 assets) public view virtual returns (uint256) { return _convertToShares(assets, Math.Rounding.Floor); } - /** @dev See {IERC4626-previewMint}. */ + /// @inheritdoc IERC4626 function previewMint(uint256 shares) public view virtual returns (uint256) { return _convertToAssets(shares, Math.Rounding.Ceil); } - /** @dev See {IERC4626-previewWithdraw}. */ + /// @inheritdoc IERC4626 function previewWithdraw(uint256 assets) public view virtual returns (uint256) { return _convertToShares(assets, Math.Rounding.Ceil); } - /** @dev See {IERC4626-previewRedeem}. */ + /// @inheritdoc IERC4626 function previewRedeem(uint256 shares) public view virtual returns (uint256) { return _convertToAssets(shares, Math.Rounding.Floor); } - /** @dev See {IERC4626-deposit}. */ + /// @inheritdoc IERC4626 function deposit(uint256 assets, address receiver) public virtual returns (uint256) { uint256 maxAssets = maxDeposit(receiver); if (assets > maxAssets) { @@ -180,7 +180,7 @@ abstract contract ERC4626 is ERC20, IERC4626 { return shares; } - /** @dev See {IERC4626-mint}. */ + /// @inheritdoc IERC4626 function mint(uint256 shares, address receiver) public virtual returns (uint256) { uint256 maxShares = maxMint(receiver); if (shares > maxShares) { @@ -193,7 +193,7 @@ abstract contract ERC4626 is ERC20, IERC4626 { return assets; } - /** @dev See {IERC4626-withdraw}. */ + /// @inheritdoc IERC4626 function withdraw(uint256 assets, address receiver, address owner) public virtual returns (uint256) { uint256 maxAssets = maxWithdraw(owner); if (assets > maxAssets) { @@ -206,7 +206,7 @@ abstract contract ERC4626 is ERC20, IERC4626 { return shares; } - /** @dev See {IERC4626-redeem}. */ + /// @inheritdoc IERC4626 function redeem(uint256 shares, address receiver, address owner) public virtual returns (uint256) { uint256 maxShares = maxRedeem(owner); if (shares > maxShares) { From b237dc19d9c7194d94e567fbff4714c2f584cfd9 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 30 Jul 2024 14:17:46 +0200 Subject: [PATCH 02/11] change inheritdoc --- contracts/token/ERC20/extensions/ERC4626.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/token/ERC20/extensions/ERC4626.sol b/contracts/token/ERC20/extensions/ERC4626.sol index 7b6adeff271..3ad8b30a492 100644 --- a/contracts/token/ERC20/extensions/ERC4626.sol +++ b/contracts/token/ERC20/extensions/ERC4626.sol @@ -97,7 +97,7 @@ abstract contract ERC4626 is ERC20, IERC4626 { } /** - * @inheritdoc ERC20 + * @inheritdoc IERC20Metadata * * @dev Decimals are computed by adding the decimal offset on top of the underlying asset's decimals. This * "original" value is cached during construction of the vault contract. If this read operation fails (e.g., the From bce690397d70f3e231a7f01246d0c6b1cefcc43b Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 30 Jul 2024 15:52:36 +0200 Subject: [PATCH 03/11] add changeset --- .changeset/shiny-dolphins-lick.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/shiny-dolphins-lick.md diff --git a/.changeset/shiny-dolphins-lick.md b/.changeset/shiny-dolphins-lick.md new file mode 100644 index 00000000000..508e7f4304c --- /dev/null +++ b/.changeset/shiny-dolphins-lick.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': minor +--- + +`ERC4626`: compute `maxWithdraw` using `maxRedeem` and `previewRedeem` so that changes to the preview functions affect the max functions. From 5af6775409964b6e801c0bcb027db701df1ed070 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 30 Jul 2024 17:08:57 +0200 Subject: [PATCH 04/11] Update contracts/token/ERC20/extensions/ERC4626.sol MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Ernesto García --- contracts/token/ERC20/extensions/ERC4626.sol | 2 ++ 1 file changed, 2 insertions(+) diff --git a/contracts/token/ERC20/extensions/ERC4626.sol b/contracts/token/ERC20/extensions/ERC4626.sol index 3ad8b30a492..a0c9d37ac47 100644 --- a/contracts/token/ERC20/extensions/ERC4626.sol +++ b/contracts/token/ERC20/extensions/ERC4626.sol @@ -102,6 +102,8 @@ abstract contract ERC4626 is ERC20, IERC4626 { * @dev Decimals are computed by adding the decimal offset on top of the underlying asset's decimals. This * "original" value is cached during construction of the vault contract. If this read operation fails (e.g., the * asset has not been created yet), a default of 18 is used to represent the underlying asset's decimals. + * + * See {IERC20Metadata-decimals}. */ function decimals() public view virtual override(IERC20Metadata, ERC20) returns (uint8) { return _underlyingDecimals + _decimalsOffset(); From 757a25fac827feed9c19cc867c01b26bd09aeb54 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 30 Jul 2024 17:11:11 +0200 Subject: [PATCH 05/11] Update ERC4626.sol --- contracts/token/ERC20/extensions/ERC4626.sol | 2 -- 1 file changed, 2 deletions(-) diff --git a/contracts/token/ERC20/extensions/ERC4626.sol b/contracts/token/ERC20/extensions/ERC4626.sol index a0c9d37ac47..0115a6b3a89 100644 --- a/contracts/token/ERC20/extensions/ERC4626.sol +++ b/contracts/token/ERC20/extensions/ERC4626.sol @@ -97,8 +97,6 @@ abstract contract ERC4626 is ERC20, IERC4626 { } /** - * @inheritdoc IERC20Metadata - * * @dev Decimals are computed by adding the decimal offset on top of the underlying asset's decimals. This * "original" value is cached during construction of the vault contract. If this read operation fails (e.g., the * asset has not been created yet), a default of 18 is used to represent the underlying asset's decimals. From e82261c007f17793a35b132cfa02106f92ac7cb6 Mon Sep 17 00:00:00 2001 From: Arr00 <13561405+arr00@users.noreply.github.com> Date: Tue, 9 Sep 2025 14:17:46 +0200 Subject: [PATCH 06/11] Fix typo in `IERC4626` --- contracts/interfaces/IERC4626.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/interfaces/IERC4626.sol b/contracts/interfaces/IERC4626.sol index 9a24507a7fd..dbd9bf403aa 100644 --- a/contracts/interfaces/IERC4626.sol +++ b/contracts/interfaces/IERC4626.sol @@ -198,7 +198,7 @@ interface IERC4626 is IERC20, IERC20Metadata { function maxRedeem(address owner) external view returns (uint256 maxShares); /** - * @dev Allows an on-chain or off-chain user to simulate the effects of their redeemption at the current block, + * @dev Allows an on-chain or off-chain user to simulate the effects of their redemption at the current block, * given current on-chain conditions. * * - MUST return as close to and no more than the exact amount of assets that would be withdrawn in a redeem call From fa67c3faf99d41325f34bba5e3a016e921624e83 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 9 Sep 2025 14:18:41 +0200 Subject: [PATCH 07/11] add overriding advices --- contracts/token/ERC20/extensions/ERC4626.sol | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/contracts/token/ERC20/extensions/ERC4626.sol b/contracts/token/ERC20/extensions/ERC4626.sol index 0115a6b3a89..28d6ca44735 100644 --- a/contracts/token/ERC20/extensions/ERC4626.sol +++ b/contracts/token/ERC20/extensions/ERC4626.sol @@ -44,6 +44,18 @@ import {Math} from "../../../utils/math/Math.sol"; * * To learn more, check out our xref:ROOT:erc4626.adoc[ERC-4626 guide]. * ==== + * + * Note: When overriding this contract, some elements have to be considered + * + * * When overriding the behavior of the deposit or withdraw mechanism, one should override the internal functions. + * Overriding {_deposit} automatically affects both {deposit} and {mint}. Similarly, overriding {_withdraw} + * automatically affects both {withdraw} and {redeem}. Having inconsistent behavior between the {deposit} and {mint} + * or between {withdraw} and {redeem} can lead to bugs. Overall it is not recommended to override the public facing + * functions. + * + * * {maxWithdraw} depends on {maxRedeem}. Therefore, overriding {maxRedeem} only is enough. On the other hand, + * overriding {maxWithdraw} only would have no effect on {maxRedeem}, and could create an inconsistency between the two + * functions. */ abstract contract ERC4626 is ERC20, IERC4626 { using Math for uint256; From e20baa8307cf66a1c422348566aad537f7fa0847 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 9 Sep 2025 14:30:29 +0200 Subject: [PATCH 08/11] up --- contracts/token/ERC20/extensions/ERC4626.sol | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/contracts/token/ERC20/extensions/ERC4626.sol b/contracts/token/ERC20/extensions/ERC4626.sol index 28d6ca44735..33d73f77a46 100644 --- a/contracts/token/ERC20/extensions/ERC4626.sol +++ b/contracts/token/ERC20/extensions/ERC4626.sol @@ -47,11 +47,11 @@ import {Math} from "../../../utils/math/Math.sol"; * * Note: When overriding this contract, some elements have to be considered * - * * When overriding the behavior of the deposit or withdraw mechanism, one should override the internal functions. - * Overriding {_deposit} automatically affects both {deposit} and {mint}. Similarly, overriding {_withdraw} - * automatically affects both {withdraw} and {redeem}. Having inconsistent behavior between the {deposit} and {mint} - * or between {withdraw} and {redeem} can lead to bugs. Overall it is not recommended to override the public facing - * functions. + * * When overriding the behavior of the deposit or withdraw mechanisms, it is recommended to override the internal + * functions. Overriding {_deposit} automatically affects both {deposit} and {mint}. Similarly, overriding {_withdraw} + * automatically affects both {withdraw} and {redeem}. Overall it is not recommended to override the public facing + * functions since that could lead to inconsistent behaviors between the {deposit} and {mint} or between {withdraw} and + * {redeem}, which is documented to have lead to loss of funds. * * * {maxWithdraw} depends on {maxRedeem}. Therefore, overriding {maxRedeem} only is enough. On the other hand, * overriding {maxWithdraw} only would have no effect on {maxRedeem}, and could create an inconsistency between the two From 2afc39c355fff6ac2f912aafaf33cd87eeca1385 Mon Sep 17 00:00:00 2001 From: Arr00 <13561405+arr00@users.noreply.github.com> Date: Tue, 9 Sep 2025 14:46:36 +0200 Subject: [PATCH 09/11] update docs --- contracts/token/ERC20/extensions/ERC4626.sol | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/contracts/token/ERC20/extensions/ERC4626.sol b/contracts/token/ERC20/extensions/ERC4626.sol index 33d73f77a46..742fd21cfbd 100644 --- a/contracts/token/ERC20/extensions/ERC4626.sol +++ b/contracts/token/ERC20/extensions/ERC4626.sol @@ -45,17 +45,25 @@ import {Math} from "../../../utils/math/Math.sol"; * To learn more, check out our xref:ROOT:erc4626.adoc[ERC-4626 guide]. * ==== * - * Note: When overriding this contract, some elements have to be considered + * [NOTE] + * ==== + * When overriding this contract, some elements must to be considered: * - * * When overriding the behavior of the deposit or withdraw mechanisms, it is recommended to override the internal + * * When overriding the behavior of the deposit or withdraw mechanism, it is recommended to override the internal * functions. Overriding {_deposit} automatically affects both {deposit} and {mint}. Similarly, overriding {_withdraw} * automatically affects both {withdraw} and {redeem}. Overall it is not recommended to override the public facing * functions since that could lead to inconsistent behaviors between the {deposit} and {mint} or between {withdraw} and * {redeem}, which is documented to have lead to loss of funds. * + * * Overrides to the deposit or withdraw mechanism must be reflected in the preview functions as well. + * * * {maxWithdraw} depends on {maxRedeem}. Therefore, overriding {maxRedeem} only is enough. On the other hand, * overriding {maxWithdraw} only would have no effect on {maxRedeem}, and could create an inconsistency between the two * functions. + * + * * If {previewRedeem} is overriden to revert, {maxWithdraw} must be overridden as necessary to ensure it + * always return successfully. + * ==== */ abstract contract ERC4626 is ERC20, IERC4626 { using Math for uint256; From 5b219022aea90f0df29ce71db827f79fc9c43b1f Mon Sep 17 00:00:00 2001 From: Arr00 <13561405+arr00@users.noreply.github.com> Date: Tue, 9 Sep 2025 14:49:16 +0200 Subject: [PATCH 10/11] Update contracts/token/ERC20/extensions/ERC4626.sol --- contracts/token/ERC20/extensions/ERC4626.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/token/ERC20/extensions/ERC4626.sol b/contracts/token/ERC20/extensions/ERC4626.sol index 742fd21cfbd..0e54e0ad0f0 100644 --- a/contracts/token/ERC20/extensions/ERC4626.sol +++ b/contracts/token/ERC20/extensions/ERC4626.sol @@ -49,7 +49,7 @@ import {Math} from "../../../utils/math/Math.sol"; * ==== * When overriding this contract, some elements must to be considered: * - * * When overriding the behavior of the deposit or withdraw mechanism, it is recommended to override the internal + * * When overriding the behavior of the deposit or withdraw mechanisms, it is recommended to override the internal * functions. Overriding {_deposit} automatically affects both {deposit} and {mint}. Similarly, overriding {_withdraw} * automatically affects both {withdraw} and {redeem}. Overall it is not recommended to override the public facing * functions since that could lead to inconsistent behaviors between the {deposit} and {mint} or between {withdraw} and From 778019f738c900df5b3eff5d27417a2f6a0b09d7 Mon Sep 17 00:00:00 2001 From: Arr00 <13561405+arr00@users.noreply.github.com> Date: Tue, 9 Sep 2025 14:53:37 +0200 Subject: [PATCH 11/11] Update contracts/token/ERC20/extensions/ERC4626.sol --- contracts/token/ERC20/extensions/ERC4626.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/token/ERC20/extensions/ERC4626.sol b/contracts/token/ERC20/extensions/ERC4626.sol index 0e54e0ad0f0..7e77c9d4440 100644 --- a/contracts/token/ERC20/extensions/ERC4626.sol +++ b/contracts/token/ERC20/extensions/ERC4626.sol @@ -61,7 +61,7 @@ import {Math} from "../../../utils/math/Math.sol"; * overriding {maxWithdraw} only would have no effect on {maxRedeem}, and could create an inconsistency between the two * functions. * - * * If {previewRedeem} is overriden to revert, {maxWithdraw} must be overridden as necessary to ensure it + * * If {previewRedeem} is overridden to revert, {maxWithdraw} must be overridden as necessary to ensure it * always return successfully. * ==== */