Add ERC20Vault and ERC7540 variants#6399
Add ERC20Vault and ERC7540 variants#6399ernestognw wants to merge 20 commits intoOpenZeppelin:masterfrom
ERC20Vault and ERC7540 variants#6399Conversation
🦋 Changeset detectedLatest commit: f3407fb The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
hieronx
left a comment
hieronx
left a comment
There was a problem hiding this comment.
Really like the overall setup of the contracts!
| /** | ||
| * @dev Performs a transfer in of shares. By default, it takes the shares from the owner. | ||
| * Used by {requestRedeem}. | ||
| * | ||
| * IMPORTANT: If overriding to burn shares immediately (instead of holding them in the vault), | ||
| * you must ALSO override {_fulfillRedeem} to use a snapshotted exchange rate, since the | ||
| * shares will no longer exist in `totalSupply()` at fulfillment time. Simply overriding | ||
| * {_completeSharesIn} to do nothing is not sufficient. | ||
| */ | ||
| function _lockSharesIn(uint256 shares, address owner) internal virtual { | ||
| _update(owner, address(this), shares); | ||
| } | ||
|
|
||
| /// @dev Performs a fulfillment of a redeem request. By default, it burns the shares. Used by {_fulfillRedeem}. | ||
| function _completeSharesIn(uint256 shares, address /* controller */) internal virtual { | ||
| _burn(address(this), shares); | ||
| } |
There was a problem hiding this comment.
This is neat, nice design @ernestognw !
| /// @inheritdoc IERC20Vault | ||
| function asset() public view virtual returns (address); | ||
|
|
There was a problem hiding this comment.
My suggestion would be to just add the share() function here.
It will instantly make all ERC-4626 vaults also ERC-7575 compatible.
| /// @inheritdoc IERC20Vault | |
| function asset() public view virtual returns (address); | |
| /// @inheritdoc IERC20Vault | |
| function asset() public view virtual returns (address); | |
| /// @inheritdoc IERC20Vault | |
| function share() public view virtual returns (address) { | |
| return address(this); | |
| } |
There was a problem hiding this comment.
Yes, this is a good idea. I'm still debating whether it makes sense because the function might suggest a developer that if they override it with another token address, then all the share transfer logic will be externalized, when in reality, they'd need to override other functions.
I'd prefer adding the share() function once we start with 7575
There was a problem hiding this comment.
I'd prefer adding the share() function once we start with 7575
We should already add the share() function because otherwise the vaults are not ERC7540 compliant. ERC7540 mandates that each vault MUST be ERC-7575 too.
It could be added inside the 7540 contracts rather than the ERC20Vault contract though, will leave that with you!
| * Users should only approve operators they fully trust with both their assets and shares. | ||
| * ==== | ||
| */ | ||
| abstract contract ERC7540Operator is ERC165, ERC20Vault, IERC7540Operator { |
There was a problem hiding this comment.
I wonder if you could have a abstract contract OperatorControl which implements setOperator, isOperator, and supportsInterface, which is then used by both ERC7540 as well as ERC6909. The inspiration for this feature set came from ERC6909 and it's almost a one to one match.
There was a problem hiding this comment.
I didn't mean adding full ERC-7741. Rather just moving setOperator, isOperator and supportsInterface to an abstract contract that is reused by ERC7540 and ERC6909 implementations.
ernestognw
changed the title
ERC20Vault, ERC7540Redeem, ERC7540Deposit and ERC7540Operator
ernestognw
changed the title
ERC20Vault, ERC7540Redeem, ERC7540Deposit and ERC7540OperatorERC20Vault and ERC7540 variants
|
I've been taking a look to Slither's issue: It seems a false positive caused by the refactoring splitting the code across two contracts in the inheritance hierarchy, and there are no public entry points within ERC20Vault itself that constrain from to Now we're just missing tests and we should be ready to go |
|
Ready for review @hieronx, if you want to take another look 😄 |
WalkthroughThis pull request introduces ERC-7540 (Asynchronous ERC-4626 Tokenized Vaults) support to the OpenZeppelin Solidity library. It extracts core vault mechanics from ERC4626 into a new base contract Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
contracts/token/ERC20/extensions/ERC7540Operator.sol (1)
38-55:_underlyingDecimalsis write-only in this contract.If decimals caching is intentionally needed here, wire it into an exposed/overridden decimals path; otherwise remove it (and related constructor work) to avoid misleading dead state.
♻️ Minimal cleanup option
import {IERC7540Operator} from "../../../interfaces/IERC7540.sol"; import {ERC165} from "../../../utils/introspection/ERC165.sol"; import {IERC20} from "../IERC20.sol"; -import {SafeERC20} from "../utils/SafeERC20.sol"; -import {Math} from "../../../utils/math/Math.sol"; @@ IERC20 private immutable _asset; - uint8 private immutable _underlyingDecimals; mapping(address => mapping(address => bool)) private _isOperator; @@ constructor(IERC20 asset_) { - (bool success, uint8 assetDecimals) = SafeERC20.tryGetDecimals(address(asset_)); - _underlyingDecimals = uint8(Math.ternary(success, assetDecimals, 18)); _asset = asset_; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/token/ERC20/extensions/ERC7540Operator.sol` around lines 38 - 55, The contract writes to _underlyingDecimals in the constructor but never reads or exposes it, creating dead state; either remove the field and the SafeERC20.tryGetDecimals logic from the constructor, or wire the cached value into an exposed/overridden decimals accessor (e.g., implement or override a decimals() function to return _underlyingDecimals). Locate the constructor and the _underlyingDecimals declaration and either delete both the field and the tryGetDecimals call (clean removal), or implement/override decimals() to return _underlyingDecimals so the cached value is used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Line 3: Change the "### Breaking changes" heading to the correct level (likely
"## Breaking changes") so the markdown header sequence is consistent (avoid
jumping from H1 to H3) and satisfies MD001 lint; update the header text in
CHANGELOG.md accordingly.
In `@contracts/token/ERC20/extensions/ERC7540Deposit.sol`:
- Around line 112-118: The code updates pending accounting (_setPendingDeposit,
pendingDepositRequest, and increments _totalPendingDepositAssets) before
performing the external token transfer via _transferIn, which allows reentrancy
to observe inconsistent totalAssets() and may underflow; fix by reordering: call
_transferIn(owner, assets) first, then compute requestId via
_depositRequestId(controller), call _setPendingDeposit(controller, assets +
pendingDepositRequest(requestId, controller)), increment
_totalPendingDepositAssets, and finally emit DepositRequest(controller, owner,
requestId, _msgSender(), assets) so pending state is recorded only after the
tokens have been successfully transferred.
In `@contracts/token/ERC20/extensions/ERC7540Operator.sol`:
- Around line 34-35: Update the error documentation for ERC7540InvalidOperator
to correctly reference the controller: change the NatSpec comment "The operator
is not the caller or an operator of the operator" to something like "The
operator is not the caller or an operator of the controller" so the comment
matches the error signature ERC7540InvalidOperator(address controller, address
operator) and the actual semantics.
In `@contracts/token/ERC20/extensions/ERC7540Redeem.sol`:
- Around line 194-205: The fulfill flow in _fulfillRedeem causes claimable
assets to remain included in totalAssets() so subsequent
_redeemPrice/convertToAssets overvalues shares; introduce a global
claimableRedeemAssets reserve (tracked and exposed via
claimableRedeemRequestAssets and updated by _setClaimableRedeem) and subtract it
from any totalAssets() used for pricing (or modify convertToAssets/_redeemPrice
to use totalAssets() - claimableRedeemAssets), update _fulfillRedeem to
increment the global reserve when making assets claimable (via
_setClaimableRedeem) and ensure burning in _completeSharesIn does not leave
assets double-counted, and update any other functions that calculate
pending/convert amounts to exclude the new claimableRedeemAssets reserve.
- Around line 98-108: The requestRedeem path currently only permits owner or
operator via the onlyOperatorOrController guard and never honors ERC‑20
allowance; modify requestRedeem so callers are allowed if they are the
controller, an owner-approved operator, or an ERC‑20 approved spender by
replacing the external-onlyOperatorOrController enforcement with an inline
check: if _msgSender() is not owner and not an operator, call
_spendAllowance(owner, _msgSender(), shares) to consume the allowance before
proceeding to _lockSharesIn; keep using _redeemRequestId, pendingRedeemRequest,
_setPendingRedeem and emit RedeemRequest as before.
---
Nitpick comments:
In `@contracts/token/ERC20/extensions/ERC7540Operator.sol`:
- Around line 38-55: The contract writes to _underlyingDecimals in the
constructor but never reads or exposes it, creating dead state; either remove
the field and the SafeERC20.tryGetDecimals logic from the constructor, or wire
the cached value into an exposed/overridden decimals accessor (e.g., implement
or override a decimals() function to return _underlyingDecimals). Locate the
constructor and the _underlyingDecimals declaration and either delete both the
field and the tryGetDecimals call (clean removal), or implement/override
decimals() to return _underlyingDecimals so the cached value is used.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 05ea2fef-227f-4055-b1e6-8e4db9a99250
📒 Files selected for processing (23)
.changeset/better-bananas-learn.md.changeset/chatty-jeans-sing.md.changeset/fast-beers-dream.md.changeset/moody-chicken-laugh.md.changeset/shiny-goats-dress.mdCHANGELOG.mdcontracts/interfaces/IERC4626.solcontracts/interfaces/IERC7540.solcontracts/mocks/token/ERC4626LimitsMock.solcontracts/token/ERC20/extensions/ERC20Vault.solcontracts/token/ERC20/extensions/ERC4626.solcontracts/token/ERC20/extensions/ERC7540.solcontracts/token/ERC20/extensions/ERC7540Deposit.solcontracts/token/ERC20/extensions/ERC7540Operator.solcontracts/token/ERC20/extensions/ERC7540Redeem.solcontracts/token/ERC20/extensions/IERC20Vault.solcontracts/token/ERC20/utils/SafeERC20.soltest/token/ERC20/extensions/ERC7540.behavior.jstest/token/ERC20/extensions/ERC7540.test.jstest/token/ERC20/extensions/ERC7540Deposit.test.jstest/token/ERC20/extensions/ERC7540Operator.test.jstest/token/ERC20/extensions/ERC7540Redeem.test.jstest/utils/introspection/SupportsInterface.behavior.js
| function requestRedeem( | ||
| uint256 shares, | ||
| address controller, | ||
| address owner | ||
| ) public virtual onlyOperatorOrController(owner, _msgSender()) returns (uint256) { | ||
| uint256 requestId = _redeemRequestId(controller); | ||
| _setPendingRedeem(controller, shares + pendingRedeemRequest(requestId, controller)); | ||
|
|
||
| // Must revert with ERC20InsufficientBalance if there's not enough balance. | ||
| _lockSharesIn(shares, owner); | ||
| emit RedeemRequest(controller, owner, requestId, _msgSender(), shares); |
There was a problem hiding this comment.
Honor ERC-20 allowance-based requestRedeem calls.
This path only accepts owner or an owner-approved operator and then moves shares with _update, so a third party that only has delegated share allowance can never open the request. ERC-7540 explicitly allows a non-owner requestRedeem to be authorized either by ERC-20 share approval or by owner-set operator approval. (eips.ethereum.org)
A small inline check that lets operators bypass allowance but falls back to _spendAllowance(owner, _msgSender(), shares) for ordinary approved spenders would cover the missing branch.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/token/ERC20/extensions/ERC7540Redeem.sol` around lines 98 - 108,
The requestRedeem path currently only permits owner or operator via the
onlyOperatorOrController guard and never honors ERC‑20 allowance; modify
requestRedeem so callers are allowed if they are the controller, an
owner-approved operator, or an ERC‑20 approved spender by replacing the
external-onlyOperatorOrController enforcement with an inline check: if
_msgSender() is not owner and not an operator, call _spendAllowance(owner,
_msgSender(), shares) to consume the allowance before proceeding to
_lockSharesIn; keep using _redeemRequestId, pendingRedeemRequest,
_setPendingRedeem and emit RedeemRequest as before.
✅ Confirmed as addressed by @ernestognw
There was a problem hiding this comment.
The current implementation intentionally uses onlyOperatorOrController for requestRedeem, which only allows owner or ERC-7540 operator authorization. However, you're right that the ERC-7540 spec requires support for ERC-20 share allowance as an alternative authorization path:
Redeem Request approval of
sharesfor amsg.senderNOT equal toownermay come either from ERC-20 approval over thesharesofowneror if theownerhas approved themsg.senderas an operator. This MUST be consistent with similar behaviour pointed out in ERC-6909 [...]
We've updated requestRedeem to replace the onlyOperatorOrController modifier with an inline three-way check: if the caller is not the owner and not an operator, _spendAllowance(owner, sender, shares) is called. This follows the ERC-6909 semantics referenced in the spec — operators bypass allowance, infinite approvals are not deducted, and finite approvals are consumed.
We also updated the IERC7540Redeem interface NatSpec to align with the spec language.
There was a problem hiding this comment.
@ernestognw, thanks for the detailed explanation and the fix. The three-way inline check (owner → operator → _spendAllowance fallback) correctly implements the ERC-7540 spec's authorization requirement and stays consistent with the ERC-6909 semantics referenced in the standard. Updating the IERC7540Redeem NatSpec to mirror the spec language is a nice touch as well.
✏️ Learnings added
Learnt from: ernestognw
Repo: OpenZeppelin/openzeppelin-contracts PR: 6399
File: contracts/token/ERC20/extensions/ERC7540Redeem.sol:0-0
Timestamp: 2026-04-01T13:36:48.338Z
Learning: In `contracts/token/ERC20/extensions/ERC7540Redeem.sol` (OpenZeppelin openzeppelin-contracts, Solidity), `requestRedeem` uses an inline three-way authorization check instead of the `onlyOperatorOrController` modifier: (1) caller is the owner — no allowance consumed; (2) caller is an ERC-7540 operator approved by the owner — no allowance consumed; (3) otherwise — `_spendAllowance(owner, sender, shares)` is called to consume ERC-20 share allowance. This follows ERC-6909 semantics as required by the ERC-7540 spec: operators bypass allowance, infinite approvals are not deducted, and finite approvals are consumed.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: ernestognw
Repo: OpenZeppelin/openzeppelin-contracts PR: 6399
File: contracts/token/ERC20/extensions/ERC7540Redeem.sol:200-212
Timestamp: 2026-04-01T10:12:10.174Z
Learning: In `contracts/token/ERC20/extensions/ERC7540Redeem.sol` (OpenZeppelin openzeppelin-contracts, Solidity), claimable redeem assets intentionally remain included in `totalAssets()` after `_fulfillRedeem` burns shares via `_completeSharesIn`. When shares are burned, `totalSupply()` decreases while `totalAssets()` is unchanged, which raises the assets-per-share rate — this is by design and consistent with standard ERC-4626 accounting. This is asymmetric with the deposit side, where pending assets *must* be excluded from `totalAssets()` to prevent existing shareholders from extracting value from depositors (assets arrive before shares are minted). The vault operator controls when `_fulfillRedeem` is called and can override `_redeemPrice` to use a snapshotted exchange rate for batch fulfillments.
Learnt from: ernestognw
Repo: OpenZeppelin/openzeppelin-contracts PR: 6399
File: contracts/token/ERC20/extensions/ERC7540Deposit.sol:122-128
Timestamp: 2026-04-01T10:28:11.839Z
Learning: In `contracts/token/ERC20/extensions/ERC7540Deposit.sol` (OpenZeppelin openzeppelin-contracts, Solidity), the `requestDeposit` function intentionally updates pending accounting (`_setPendingDeposit`, `_totalPendingDepositAssets`) BEFORE calling `_transferIn` — this is correct CEI (Checks-Effects-Interactions). Suggesting to reorder so that `_transferIn` comes first would break CEI and is strictly worse. The narrow read-only reentrancy window (ERC-777 hooks observing a temporarily understated `totalAssets()` mid-transfer) is addressed via documentation rather than code reordering.
Learnt from: ernestognw
Repo: OpenZeppelin/openzeppelin-contracts PR: 5904
File: contracts/crosschain/README.adoc:1-1
Timestamp: 2025-08-28T15:48:30.716Z
Learning: ernestognw prefers "Cross chain" without hyphenation rather than "Cross-chain" in documentation titles.
There was a problem hiding this comment.
@hieronx In your original ERC-7540 design proposal, requestRedeem used onlyOperatorOrController without an ERC-20 allowance path. The spec says authorization "may come either from ERC-20 approval over the shares of owner or if the owner has approved the msg.sender as an operator," and we're debating whether both paths should be supported or if operator-only is sufficient.
I went ahead and added the ERC-20 allowance fallback (with ERC-6909 semantics: operators bypass allowance, finite approvals are consumed) in 8dd0895. Would appreciate your thoughts on whether this aligns with the spec's intent or if operator-only was the intended design.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
I hadn't added this to the original proposal yet because of the increased complexity, but the intention of the ERC-7540 spec was exactly supporting the ERC-20 allowance fallback as well.
In hindsight I think we overcomplicated this and should have left it out, but it is how the spec is now written and finalized.
…ogic into ERC20Vault
|
This PR moves the To fix this, I've opened a PR on the transpiler adding support for a /// @custom:oz-transpile-namespace ERC4626
abstract contract ERC20Vault is ERC20, IERC20Vault {This will make the transpiler generate the storage struct at This PR is blocked on the transpiler update. |
| * | ||
| * Emits an {OperatorSet} event if the approval status changes. | ||
| */ | ||
| function _setOperator(address controller, address operator, bool approved) internal { |
There was a problem hiding this comment.
Small note, the ERC states that the setOperator function MUST emit the OperatorSet event. This implementation does not emit the event if the status was not modified. While skipping the event doesn't change observability of the operators, it is not exactly following the specs.
There was a problem hiding this comment.
True! The spec also says:
MAY be logged when the operator status is set to the same status it was before the current call.
I would prefer to follow this statement rather than always emit. It's true both overlap
| */ | ||
| function _setOperator(address controller, address operator, bool approved) internal { | ||
| if (_isOperator[controller][operator] != approved) { | ||
| _isOperator[controller][operator] = approved; |
There was a problem hiding this comment.
I'm not sure the check is really worth it.
- The (cold) sload costs 2100 gas.
- If we decide to not write (because the write is not necessary) we will end up paying 2100gas + the cost of the if.
- If we realize we do have to do the write, we have to pay the write (20k if false -> true, 2900 + 4800 refund if true -> false)
If we skip the check, and just write directly to storage:
- if
true -> trueorfalse -> false, the gas cost is 2200 (compared to 2100 + if logic) - if
false -> true: 22100, which is the same as previous, expect we don't have to pay for the if, so actually marginally cheaper - if
true -> false: 5000 (+ 4800 refund), which is the same as previous, expect we don't have to pay for the if, so actually marginally cheaper
So basically, the sload + allows the user saving 100 gas (minus the cost of the if) on no-ops, but costs the user the cost of the if on none-no-op operations (which I would argue are the one we should optimise).
If we follow the specs and always emit the event, I think we should not keep the if just for the sake of the sstore part.
| * See {IERC7540Deposit-deposit}. | ||
| */ | ||
| function deposit(uint256 assets, address receiver) public virtual returns (uint256 shares) { | ||
| return deposit(assets, receiver, receiver); |
There was a problem hiding this comment.
I believe that to comply with 4262, the token should be taken from the caller's "balance", and not from the receiver. So IMO this should probably be
| return deposit(assets, receiver, receiver); | |
| return deposit(assets, receiver, msg.sender); |
| _setClaimableDeposit( | ||
| controller, | ||
| requestAssets - assets, | ||
| Math.ternary(requestShares > sharesUp, requestShares - sharesUp, 0) |
There was a problem hiding this comment.
| Math.ternary(requestShares > sharesUp, requestShares - sharesUp, 0) | |
| Math.saturatingSub(requestShares, sharesUp) |
| address receiver, | ||
| address controller | ||
| ) public virtual onlyOperatorOrController(controller, _msgSender()) returns (uint256) { | ||
| uint256 requestId = _depositRequestId(controller); |
There was a problem hiding this comment.
What would happen here if _depositRequestId doesn't return the same thing as was computed in the requestDeposit ? _depositRequestId is view virtual, so we have to consider it could be anything.
Do you know how is the requestId is used in existing 7540 implementations ?
| } | ||
|
|
||
| /// @dev Returns the request ID for the given assets, controller, and owner | ||
| function _depositRequestId(address /* controller */) internal view virtual returns (uint256) { |
There was a problem hiding this comment.
the arguments don't match the natspec documentation
4 participants
Fixes #4761
PR Checklist
npx changeset add)