Add ERC-7535 Native Asset Vault as an ERC-4626 extension - #6624
Add ERC-7535 Native Asset Vault as an ERC-4626 extension#66240xAdriaTorralba wants to merge 5 commits into
Conversation
Implement ERC-7535 as a thin extension of ERC-4626. To enable it, make IERC4626/ERC4626 `deposit` and `mint` payable (function selectors and the ERC-165 interface id are unchanged) and route native-value validation through a new internal `_checkPayment` hook. The default hook reverts on any non-zero `msg.value`, so ERC-20 vaults keep rejecting native value exactly as before. ERC7535 overrides asset()/totalAssets() for the native-asset placeholder and balance, _transferIn/_transferOut for the msg.value / Address.sendValue paths, the conversion seam to price against the pre-msg.value balance, and _checkPayment to require msg.value to cover the deposit (any excess is kept as a donation). A virtual receive() rejects unsolicited plain transfers.
Hardhat and Foundry suites for ERC7535 (msg.value >= policy with overpayment donation, unsolicited-transfer rejection, the 77/78 decimals-offset boundary, force-fed-balance accounting, CEI reentrancy, and solvency / no-value-creation invariants), plus an ERC4626 test pinning that a plain ERC-20 vault still reverts on a non-zero msg.value to deposit/mint.
Add the ERC-7535 guide page, register it in the nav and the token/ERC20 API index, and note the new payable deposit/mint + _checkPayment seam in the ERC-4626 guide. Two changesets: the IERC4626/ERC4626 payable change and the ERC7535 addition.
- deposit() now prices shares off msg.value, ignoring the assets argument (EIP-7535 conformance); fixes the deposit{value:v}(0) zero-share trap
- Revert hand-stamped version headers (release tooling manages them)
- Add ERC4626 offset 77/78 conversion-overflow tests + IERC4626 selector/interfaceId lock test
- Note the payable-override breaking change in the changeset
… naming Parameterize the outer call in _assertReentrancyCEI by kind so Kind.Withdraw actually enters through withdraw() (previously both kinds redeemed, leaving the withdraw callback path uncovered), deriving payout and burned shares via the preview functions instead of assuming a full burn. Rename the reentrant receiver's reenterShares to reenterAmount and pass an assets amount on the withdraw path (it was reusing a share amount as the assets argument). In the Hardhat suite, rename a shares-holding variable that was named assets and pin the full Withdraw event payload in the zero-address documenting test.
🦋 Changeset detectedLatest commit: 9ae681b 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 |
WalkthroughThe changes make ERC-4626 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. 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. 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: 2
🧹 Nitpick comments (1)
test/token/ERC20/extensions/ERC7535.test.js (1)
163-171: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen the outbound-send-failure reverts with negative balance assertions.
These tests only assert the call reverts. Adding
to.not.changeEtherBalances(or atotalSupply/share-balance check like the Forge counterpart inERC7535.t.solat Lines 555-557) would prove the revertedwithdraw/redeemleft the vault and holder balances untouched, catching any partial-state leak.♻️ Example
it('withdraw to a receiver whose receive() reverts bubbles the failure', async function () { - await expect(this.vault.connect(this.holder).withdraw(ethers.parseEther('1'), this.rejector, this.holder)).to.be - .reverted; + const tx = this.vault.connect(this.holder).withdraw(ethers.parseEther('1'), this.rejector, this.holder); + await expect(tx).to.be.reverted; + await expect(tx).to.not.changeEtherBalances([this.vault, this.rejector], [0n, 0n]); });Based on learnings: in tests that exercise revert semantics, use negative balance assertions (e.g.,
to.not.changeEtherBalances) to verify that no ether balance changes occurred as a result of the reverted transaction.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/token/ERC20/extensions/ERC7535.test.js` around lines 163 - 171, Strengthen the revert assertions in the `withdraw` and `redeem` tests by also verifying that the failed transactions leave balances unchanged. Wrap each call with `to.not.changeEtherBalances` for the vault and holder, or assert unchanged `totalSupply` and holder share balance as in the Forge counterpart, while preserving the existing reverted expectation.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/modules/ROOT/pages/erc7535.adoc`:
- Line 63: Update the ERC7535 receive() behavior description to state that plain
.transfer reverts, while .send and call{value: ...}("") return failure unless
the caller checks and propagates the result. Preserve the explanation that the
failure is decodable and prevents unsolicited deposits.
- Line 5: Update the ERC7535 overview paragraph to avoid claiming that only
asset-movement seams differ from ERC4626; explicitly include payment-validation
and in-flight accounting/conversion behavior among the changed extension points
while preserving the listed inherited behavior.
---
Nitpick comments:
In `@test/token/ERC20/extensions/ERC7535.test.js`:
- Around line 163-171: Strengthen the revert assertions in the `withdraw` and
`redeem` tests by also verifying that the failed transactions leave balances
unchanged. Wrap each call with `to.not.changeEtherBalances` for the vault and
holder, or assert unchanged `totalSupply` and holder share balance as in the
Forge counterpart, while preserving the existing reverted expectation.
🪄 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: 34712e00-32d2-418f-89bc-a7a449ffa337
📒 Files selected for processing (13)
.changeset/native-harbors-drift.md.changeset/quiet-vaults-sail.mdcontracts/interfaces/IERC4626.solcontracts/mocks/token/ERC7535OffsetMock.solcontracts/token/ERC20/README.adoccontracts/token/ERC20/extensions/ERC4626.solcontracts/token/ERC20/extensions/ERC7535.soldocs/modules/ROOT/nav.adocdocs/modules/ROOT/pages/erc4626.adocdocs/modules/ROOT/pages/erc7535.adoctest/token/ERC20/extensions/ERC4626.test.jstest/token/ERC20/extensions/ERC7535.t.soltest/token/ERC20/extensions/ERC7535.test.js
|
|
||
| https://eips.ethereum.org/EIPS/eip-7535[ERC-7535] is an adaptation of xref:erc4626.adoc[ERC-4626] in which the underlying asset of the vault is the chain's native asset (e.g. Ether) instead of an ERC-20 token. It keeps the ERC-4626 share-accounting model, rounding rules, and interface, so the same mental model, security considerations, and customizations apply, with a few native-asset specific differences. | ||
|
|
||
| `ERC7535` is implemented as a thin extension of `ERC4626`: share accounting, rounding, the virtual-shares inflation mitigation, the preview functions, and the checks-effects-interactions withdraw path are all inherited. Only the asset-movement seams change. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Avoid understating the extension points.
The document says only asset-movement seams change, but ERC7535 also changes payment validation and in-flight accounting/conversion behavior. Replace this with wording that includes payment and accounting seams, or remove “only.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/modules/ROOT/pages/erc7535.adoc` at line 5, Update the ERC7535 overview
paragraph to avoid claiming that only asset-movement seams differ from ERC4626;
explicitly include payment-validation and in-flight accounting/conversion
behavior among the changed extension points while preserving the listed
inherited behavior.
|
|
||
| == Plain native-asset transfers | ||
|
|
||
| Value must enter the vault through the standardized `deposit` or `mint` entry points so it is matched with newly issued shares. To make accidental misuse fail loudly, `ERC7535` implements a `receive()` function that reverts with the named error `ERC7535UnsolicitedDeposit`: plain `.transfer`, `.send` or `call{value: x}("")` to the vault revert with a reason wallets and UIs can decode, instead of being silently absorbed as a donation that would skew the next deposit's exchange rate. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the .send and low-level call behavior description.
A reverting receive() causes .transfer to revert, but .send and call{value: ...}("") return failure to the caller unless that caller explicitly checks and propagates it. Clarify the statement so integrators do not assume those low-level operations automatically revert.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/modules/ROOT/pages/erc7535.adoc` at line 63, Update the ERC7535
receive() behavior description to state that plain .transfer reverts, while
.send and call{value: ...}("") return failure unless the caller checks and
propagates the result. Preserve the explanation that the failure is decodable
and prevents unsolicited deposits.
Fixes #5808
Implements ERC-7535 (Native Asset ERC-4626 Tokenized Vault) as a thin extension of
ERC4626, inheriting all share accounting instead of duplicating it.Approach
The only thing that prevented ERC-7535 from extending
ERC4626was thatIERC4626.deposit/mintarenonpayable(Solidity forbids apayableoverride of anonpayablefunction). This PR removes that blocker:IERC4626/ERC4626:depositandmintbecomepayable. This changes neither the function selectors nor the ERC-165 interface id, so it is ABI-compatible. A new internal hook_checkPayment(uint256 assets)validates the native value; the default reverts on any non-zeromsg.value, so existing ERC-20 vaults keep rejecting native value exactly as the previously non-payable entry points did.ERC4626already exposed_transferIn/_transferOutvirtual seams, so the asset-movement layer was already overridable.ERC7535then collapses to a small override set overERC4626:asset()→ ERC-7528 placeholder0xEeee…EEeE;totalAssets()→address(this).balance._checkPayment→ requiremsg.valueto cover the deposit._transferIn→ no-op (value arrived asmsg.value);_transferOut→Address.sendValue._convertToShares/_convertToAssets→ price against_pretotalAssets()(totalAssets() - msg.value) so a standalonepreviewDepositmatches the shares a subsequentdepositmints.receive()(virtual) → revertERC7535UnsolicitedDeposit.All share accounting, rounding, the virtual-offset inflation mitigation, the preview/max functions, and the CEI withdraw path are inherited unchanged — no duplication.
Design notes
msg.valuepolicy:deposit/mintrequiremsg.value >= assets(resp. the previewed cost); too little reverts withERC7535InsufficientNativeValue. Excess is kept as a donation rather than refunded — a refund would add an outbound native call inside the deposit flow, reopening the reentrancy / refund-griefing surface the inherited CEI ordering avoids.ERC4626; force-feeds (SELFDESTRUCT, block rewards) are tracked by the balance-basedtotalAssets()and handled by the offset math, not by thereceiveguard._decimalsOffsetceiling: documented asoffset <= 77—10 ** offsetoverflowsuint256beyond that (theuint8decimals()bound of 237 is not the binding constraint). Pinned with tests on both sides of the 77/78 boundary. (Applies toERC4626too.)Tests
[0, 6, 18]× empty/donated/populated states; the>=policy (underpay reverts, exact + overpay succeed, overpay donates); unsolicited-transfer rejection; outbound-send failure; the 77/78 boundary; a yield-accrual scenario. Plus a new ERC4626 test asserting a token vault reverts (ERC4626UnexpectedNativeValue) on a non-zeromsg.value.invariantSolvency/invariantNoValueCreation(5000 runs × 500 calls, 0 reverts).Local:
lint,test:inheritance,test:generationgreen; docs build resolves the{{ERC7535}}placeholder.Changeset classification
Both changesets are marked
minor. The selector + interface id are unchanged and no in-repo subclass overridesdeposit/mint, so this is non-breaking here — but an external subclass that overridesdeposit/mintasnonpayablewould need a recompile, so amajorclassification is defensible if maintainers prefer.PR Checklist
npx changeset add)