Add ERC-7535 Native Asset Tokenized Vault#245
Open
0xAdriaTorralba wants to merge 3 commits into
Open
Conversation
WalkthroughThis PR adds ERC-7535, a native-asset ERC-4626 tokenized vault pattern. It introduces the IERC7535 interface, an ERC7535 implementation with payable deposit/mint, exact msg.value validation, virtual-offset inflation mitigation, and reentrancy-safe CEI ordering, plus Foundry/Hardhat test suites and comprehensive documentation. ChangesERC-7535 Native Asset Vault
Sequence DiagramsequenceDiagram
participant User
participant Vault as ERC7535 Vault
participant Balance as Native Balance
User->>Vault: deposit(assets, receiver)<br/>{value: assets}
activate Vault
Vault->>Vault: _pretotalAssets()<br/>(excludes msg.value)
Vault->>Vault: convertToShares(assets)<br/>(floor rounding)
Vault->>Vault: _deposit(shares, receiver)
Vault->>Vault: _transferIn()<br/>(default: no-op)
Vault->>Vault: mint(shares, receiver)
Vault->>Vault: emit Deposit
deactivate Vault
User->>Vault: withdraw(assets, receiver, owner)
activate Vault
Vault->>Vault: previewWithdraw(assets)<br/>(ceil to shares)
Vault->>Vault: _withdraw(shares, receiver, owner)
Vault->>Vault: _approve burn allowance
Vault->>Vault: burn(shares)
Vault->>Balance: _transferOut (sendValue)
Vault->>Vault: emit Withdraw
deactivate Vault
User->>Vault: receive() {value}
Vault-->>User: revert ERC7535UnsolicitedDeposit
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Pull request overview
This PR introduces ERC-7535 support to the codebase by adding a native-asset (e.g., ETH) tokenized vault interface and an ERC-4626-like base implementation, plus accompanying documentation and test suites (Hardhat + Foundry).
Changes:
- Added
IERC7535(ERC-7535 interface surface withpayabledeposit/mint) andERC7535(native-asset vault base implementation with pre-msg.valueaccounting). - Added mocks and test coverage (Hardhat behavioral suite + Foundry unit/fuzz + invariants).
- Added docs, API doc registrations, navigation entry, and a changelog entry.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| test/token/ERC20/extensions/ERC7535.test.js | Hardhat test suite for ERC-7535 behavior (msg.value enforcement, offset bounds, send failure, etc.). |
| test/token/ERC20/extensions/ERC7535.t.sol | Foundry unit/fuzz + invariant tests for ERC-7535 (preview equivalence, CEI reentrancy, solvency invariants). |
| docs/modules/ROOT/pages/erc7535.adoc | New documentation guide describing ERC-7535 semantics and differences from ERC-4626. |
| docs/modules/ROOT/nav.adoc | Adds ERC-7535 to the docs navigation under Tokens. |
| contracts/token/README.adoc | Registers ERC7535 for token docs and API placeholder expansion. |
| contracts/token/ERC20/extensions/ERC7535.sol | Core ERC-7535 abstract implementation (native-asset vault logic + virtual-offset math + CEI withdrawals). |
| contracts/mocks/token/ERC7535OffsetMock.sol | Mock vault with configurable decimals offset for testing. |
| contracts/mocks/import.sol | Imports EtherReceiverMock so it’s available for tests exercising ETH send failures. |
| contracts/interfaces/README.adoc | Registers IERC7535 in the interfaces documentation and docgen placeholders. |
| contracts/interfaces/IERC7535.sol | New ERC-7535 interface definition. |
| CHANGELOG.md | Adds changelog entry for IERC7535/ERC7535 introduction. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1,229 @@ | |||
| // SPDX-License-Identifier: MIT | |||
|
|
|||
| pragma solidity >=0.6.2; | |||
Comment on lines
+106
to
+108
| /** | ||
| * @dev Mints `shares` Vault shares to `receiver` by depositing exactly `msg.value` of the native asset. | ||
| * |
Comment on lines
+20
to
+22
| * This extension allows the minting and burning of "shares" (represented using the ERC-20 inheritance) in exchange for | ||
| * the native asset through standardized {deposit}, {mint}, {redeem} and {burn} workflows. This contract extends the | ||
| * ERC-20 standard. Any additional extensions included along it would affect the "shares" token represented by this |
|
|
||
| pragma solidity ^0.8.24; | ||
|
|
||
| import {IERC20, IERC20Metadata, ERC20} from "@openzeppelin/contracts/token/ERC20/ERC20.sol"; |
Comment on lines
+402
to
+407
| it('withdraw with approval', async function () { | ||
| const assets = await this.vault.previewWithdraw(parseAsset(1n)); | ||
|
|
||
| await expect(this.vault.connect(this.other).withdraw(parseAsset(1n), this.recipient, this.holder)) | ||
| .to.be.revertedWithCustomError(this.vault, 'ERC20InsufficientAllowance') | ||
| .withArgs(this.other, 0n, assets); |
Comment on lines
+36
to
+51
| ERC7535VaultMock public vault; | ||
| Kind public kind; | ||
| uint256 public reenterShares; // shares the receiver tries to re-redeem during the callback | ||
|
|
||
| bool public reentered; | ||
| bool public reentryReverted; | ||
|
|
||
| // State snapshot observed *inside* the reentrant callback (i.e. mid-`_withdraw`, after the burn). | ||
| uint256 public observedSelfBalance; | ||
| uint256 public observedTotalSupply; | ||
|
|
||
| function setup(ERC7535VaultMock vault_, Kind kind_, uint256 reenterShares_) external { | ||
| vault = vault_; | ||
| kind = kind_; | ||
| reenterShares = reenterShares_; | ||
| } |
Comment on lines
+71
to
+77
| function reenter() external { | ||
| if (kind == Kind.Withdraw) { | ||
| vault.withdraw(reenterShares, address(this), address(this)); | ||
| } else if (kind == Kind.Redeem) { | ||
| vault.redeem(reenterShares, address(this), address(this)); | ||
| } | ||
| } |
Comment on lines
+383
to
+389
| // The attacker will try to re-redeem its FULL share balance again during the payout callback. | ||
| r.setup(vault, kind, attackerShares); | ||
|
|
||
| uint256 vaultBalBefore = address(vault).balance; | ||
| uint256 totalSupplyBefore = vault.totalSupply(); | ||
| uint256 expectedPayout = vault.previewRedeem(attackerShares); | ||
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
test/token/ERC20/extensions/ERC7535.test.js (1)
172-176: ⚡ Quick winPin the exact
Withdrawpayload in the zero-address documenting test.This case is meant to lock in current behavior, but it only checks that some
Withdrawevent fired. As written, a regression in the emitted receiver/owner/assets/shares would still pass.Suggested assertion
const value = ethers.parseEther('1'); const tx = this.vault.connect(this.holder).withdraw(value, ethers.ZeroAddress, this.holder); await expect(tx).to.changeEtherBalances([this.vault, ethers.ZeroAddress], [-value, value]); - await expect(tx).to.emit(this.vault, 'Withdraw'); + await expect(tx) + .to.emit(this.vault, 'Withdraw') + .withArgs(this.holder, ethers.ZeroAddress, this.holder, value, value);🤖 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 172 - 176, Update the test that calls this.vault.connect(this.holder).withdraw(...) to assert the exact Withdraw event payload: ensure the expectation includes withArgs(ethers.ZeroAddress, <holder address>, <assets expected>, <shares expected>) (e.g. await expect(tx).to.emit(this.vault, 'Withdraw').withArgs(ethers.ZeroAddress, await this.holder.getAddress(), value, value)); reference the withdraw call and the 'Withdraw' event on this.vault and replace the loose .to.emit check with the .withArgs assertion that pins receiver, owner, assets, and shares.
🤖 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 `@contracts/token/ERC20/extensions/ERC7535.sol`:
- Around line 20-22: The NatSpec public API text incorrectly lists the exit
workflows as {redeem} and {burn}; update the comment to reference {withdraw}
instead of {burn} so the docs match the actual external functions ({deposit},
{mint}, {redeem}, {withdraw}) exposed by this contract (look for the ERC7535
contract/public API description and replace `{burn}` with `{withdraw}`).
- Around line 135-252: The NatSpec header for ERC7535 incorrectly references
{burn} even though the public exit API uses withdraw/redeem; update the
contract/interface documentation on ERC7535 (the top-of-file/header block for
the ERC7535 contract) to replace or remove the `{burn}` reference and instead
reference the public exit functions `withdraw` and `redeem` (or simply document
the exit API as withdraw/redeem), keeping all function names like deposit, mint,
withdraw, redeem, previewWithdraw, previewRedeem consistent with IERC7535.
In `@test/token/ERC20/extensions/ERC7535.t.sol`:
- Around line 370-391: The test helper _assertReentrancyCEI always calls
vault.redeem(...) for the outer payout, so the Kind.Withdraw case never
exercises the vault.withdraw entrypoint; update _assertReentrancyCEI to branch
on ReentrantReceiver.Kind and call vault.withdraw(expectedPayout, address(r),
address(r)) when kind == ReentrantReceiver.Kind.Withdraw, otherwise keep the
existing vault.redeem(attackerShares, address(r), address(r)) call; adjust any
local variables used (e.g., expectedPayout vs attackerShares) so the correct
argument is passed for each branch.
In `@test/token/ERC20/extensions/ERC7535.test.js`:
- Line 194: The tests currently approve ethers.MaxUint256 via $_approve and only
assert authorization; change the delegated withdraw/redeem tests to approve a
finite amount equal to the shares being withdrawn (use the same shares value
used in the call), then after the delegated call assert the allowance was
reduced to zero (or attempt a second delegated call and assert it reverts with
ERC20InsufficientAllowance) to verify ERC7535._withdraw calls
_spendAllowance(owner, caller, shares). Also tighten the receiver == address(0)
test by asserting the Withdraw event's full arguments (owner,
receiver==address(0), shares, amount) rather than only checking ETH balance and
that the event emitted.
---
Nitpick comments:
In `@test/token/ERC20/extensions/ERC7535.test.js`:
- Around line 172-176: Update the test that calls
this.vault.connect(this.holder).withdraw(...) to assert the exact Withdraw event
payload: ensure the expectation includes withArgs(ethers.ZeroAddress, <holder
address>, <assets expected>, <shares expected>) (e.g. await
expect(tx).to.emit(this.vault, 'Withdraw').withArgs(ethers.ZeroAddress, await
this.holder.getAddress(), value, value)); reference the withdraw call and the
'Withdraw' event on this.vault and replace the loose .to.emit check with the
.withArgs assertion that pins receiver, owner, assets, and shares.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5e6fbbc1-af83-4b2c-89a1-2a3d3cc04e5e
📒 Files selected for processing (11)
CHANGELOG.mdcontracts/interfaces/IERC7535.solcontracts/interfaces/README.adoccontracts/mocks/import.solcontracts/mocks/token/ERC7535OffsetMock.solcontracts/token/ERC20/extensions/ERC7535.solcontracts/token/README.adocdocs/modules/ROOT/nav.adocdocs/modules/ROOT/pages/erc7535.adoctest/token/ERC20/extensions/ERC7535.t.soltest/token/ERC20/extensions/ERC7535.test.js
Comment on lines
+20
to
+22
| * This extension allows the minting and burning of "shares" (represented using the ERC-20 inheritance) in exchange for | ||
| * the native asset through standardized {deposit}, {mint}, {redeem} and {burn} workflows. This contract extends the | ||
| * ERC-20 standard. Any additional extensions included along it would affect the "shares" token represented by this |
There was a problem hiding this comment.
Replace {burn} with {withdraw} in the public API description.
The NatSpec says the standardized exit workflows are {redeem} and {burn}, but this contract exposes {withdraw}/{redeem}. That mismatch will leak into generated docs and misdescribe the external surface.
🤖 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 `@contracts/token/ERC20/extensions/ERC7535.sol` around lines 20 - 22, The
NatSpec public API text incorrectly lists the exit workflows as {redeem} and
{burn}; update the comment to reference {withdraw} instead of {burn} so the docs
match the actual external functions ({deposit}, {mint}, {redeem}, {withdraw})
exposed by this contract (look for the ERC7535 contract/public API description
and replace `{burn}` with `{withdraw}`).
Comment on lines
+135
to
+252
| function asset() public view virtual returns (address) { | ||
| return NATIVE_ASSET; | ||
| } | ||
|
|
||
| /// @inheritdoc IERC7535 | ||
| function totalAssets() public view virtual returns (uint256) { | ||
| return address(this).balance; | ||
| } | ||
|
|
||
| /// @inheritdoc IERC7535 | ||
| function convertToShares(uint256 assets) public view virtual returns (uint256) { | ||
| return _convertToShares(assets, Math.Rounding.Floor); | ||
| } | ||
|
|
||
| /// @inheritdoc IERC7535 | ||
| function convertToAssets(uint256 shares) public view virtual returns (uint256) { | ||
| return _convertToAssets(shares, Math.Rounding.Floor); | ||
| } | ||
|
|
||
| /// @inheritdoc IERC7535 | ||
| function maxDeposit(address) public view virtual returns (uint256) { | ||
| return type(uint256).max; | ||
| } | ||
|
|
||
| /// @inheritdoc IERC7535 | ||
| function maxMint(address) public view virtual returns (uint256) { | ||
| return type(uint256).max; | ||
| } | ||
|
|
||
| /// @inheritdoc IERC7535 | ||
| function maxWithdraw(address owner) public view virtual returns (uint256) { | ||
| return previewRedeem(maxRedeem(owner)); | ||
| } | ||
|
|
||
| /// @inheritdoc IERC7535 | ||
| function maxRedeem(address owner) public view virtual returns (uint256) { | ||
| return balanceOf(owner); | ||
| } | ||
|
|
||
| /// @inheritdoc IERC7535 | ||
| function previewDeposit(uint256 assets) public view virtual returns (uint256) { | ||
| return _convertToShares(assets, Math.Rounding.Floor); | ||
| } | ||
|
|
||
| /// @inheritdoc IERC7535 | ||
| function previewMint(uint256 shares) public view virtual returns (uint256) { | ||
| return _convertToAssets(shares, Math.Rounding.Ceil); | ||
| } | ||
|
|
||
| /// @inheritdoc IERC7535 | ||
| function previewWithdraw(uint256 assets) public view virtual returns (uint256) { | ||
| return _convertToShares(assets, Math.Rounding.Ceil); | ||
| } | ||
|
|
||
| /// @inheritdoc IERC7535 | ||
| function previewRedeem(uint256 shares) public view virtual returns (uint256) { | ||
| return _convertToAssets(shares, Math.Rounding.Floor); | ||
| } | ||
|
|
||
| /// @inheritdoc IERC7535 | ||
| function deposit(uint256 assets, address receiver) public payable virtual returns (uint256) { | ||
| if (msg.value != assets) { | ||
| revert ERC7535UnexpectedDepositValue(msg.value, assets); | ||
| } | ||
|
|
||
| uint256 maxAssets = maxDeposit(receiver); | ||
| if (assets > maxAssets) { | ||
| revert ERC7535ExceededMaxDeposit(receiver, assets, maxAssets); | ||
| } | ||
|
|
||
| uint256 shares = _convertToShares(assets, Math.Rounding.Floor); | ||
| _deposit(_msgSender(), receiver, assets, shares); | ||
|
|
||
| return shares; | ||
| } | ||
|
|
||
| /// @inheritdoc IERC7535 | ||
| function mint(uint256 shares, address receiver) public payable virtual returns (uint256) { | ||
| uint256 maxShares = maxMint(receiver); | ||
| if (shares > maxShares) { | ||
| revert ERC7535ExceededMaxMint(receiver, shares, maxShares); | ||
| } | ||
|
|
||
| uint256 assets = _convertToAssets(shares, Math.Rounding.Ceil); | ||
| if (msg.value != assets) { | ||
| revert ERC7535UnexpectedMintValue(msg.value, assets); | ||
| } | ||
|
|
||
| _deposit(_msgSender(), receiver, assets, shares); | ||
|
|
||
| return assets; | ||
| } | ||
|
|
||
| /// @inheritdoc IERC7535 | ||
| function withdraw(uint256 assets, address receiver, address owner) public virtual returns (uint256) { | ||
| uint256 maxAssets = maxWithdraw(owner); | ||
| if (assets > maxAssets) { | ||
| revert ERC7535ExceededMaxWithdraw(owner, assets, maxAssets); | ||
| } | ||
|
|
||
| uint256 shares = previewWithdraw(assets); | ||
| _withdraw(_msgSender(), receiver, owner, assets, shares); | ||
|
|
||
| return shares; | ||
| } | ||
|
|
||
| /// @inheritdoc IERC7535 | ||
| function redeem(uint256 shares, address receiver, address owner) public virtual returns (uint256) { | ||
| uint256 maxShares = maxRedeem(owner); | ||
| if (shares > maxShares) { | ||
| revert ERC7535ExceededMaxRedeem(owner, shares, maxShares); | ||
| } | ||
|
|
||
| uint256 assets = previewRedeem(shares); | ||
| _withdraw(_msgSender(), receiver, owner, assets, shares); | ||
|
|
||
| return assets; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does Solidity 0.8.x require the override specifier on a contract function that implements a function declared in an inherited interface?
💡 Result:
In Solidity 0.8.x, the requirement for the override specifier depends on the specific minor version and the inheritance structure: - Starting from Solidity 0.8.8, the override keyword is not required when implementing a function declared in a single interface [1][2]. - In versions prior to 0.8.8, the override keyword was mandatory for all interface function implementations [2][3]. - Regardless of the version, if a contract inherits the same function from multiple bases (e.g., multiple interfaces or a mix of contracts and interfaces) that define the same function, the override keyword is required, and you must explicitly specify the base contracts in the override specifier, such as override(Base1, Base2) [1][4][5]. In summary, for Solidity versions 0.8.8 and later, you only need the override specifier when the inheritance hierarchy creates ambiguity that requires explicit resolution [1][2].
Citations:
- 1: https://docs.soliditylang.org/en/v0.8.28/contracts.html
- 2: https://www.soliditylang.org/blog/2021/09/27/solidity-0.8.8-release-announcement/
- 3: https://soliditylang.org/blog/2020/06/18/solidity-0.6-inheritance/
- 4: https://docs.soliditylang.org/en/v0.8.22/060-breaking-changes.html
- 5: https://docs.soliditylang.org/en/v0.8.25/contracts.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate pragma + contract declaration
FILE="contracts/token/ERC20/extensions/ERC7535.sol"
if [ ! -f "$FILE" ]; then
echo "Missing file: $FILE" >&2
exit 1
fi
echo "---- $FILE: header (first 80 lines) ----"
sed -n '1,90p' "$FILE" | cat -n
echo "---- $FILE: contract declaration / inheritance ----"
rg -n "contract\s+ERC7535" "$FILE" && rg -n "is\s+IERC7535|is\s+.*IERC7535" "$FILE" || true
echo "---- $FILE: function signatures in 130-260 ----"
sed -n '120,270p' "$FILE" | cat -n
# Find IERC7535 interface to confirm exact function signatures and whether override is expected
# Search for interface IERC7535 in repo
echo "---- Searching for IERC7535 ----"
fd -t f "IERC7535.sol" . || true
rg -n "interface\s+IERC7535" . || trueRepository: OpenZeppelin/openzeppelin-community-contracts
Length of output: 13812
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read IERC7535 interface around the relevant function declarations
FILE="contracts/interfaces/IERC7535.sol"
echo "---- $FILE (first 220 lines) ----"
sed -n '1,220p' "$FILE" | cat -n
echo "---- Search for function declarations in IERC7535 for deployer/public API ----"
rg -n "function\s+(asset|totalAssets|convertToShares|convertToAssets|maxDeposit|maxMint|maxWithdraw|maxRedeem|previewDeposit|previewMint|previewWithdraw|previewRedeem|deposit|mint|withdraw|redeem|burn)\b" "$FILE"
# Search for burn in ERC7535
FILE2="contracts/token/ERC20/extensions/ERC7535.sol"
echo "---- Search for burn in $FILE2 ----"
rg -n "\bfunction\s+burn\b|\b burn\(" "$FILE2" || trueRepository: OpenZeppelin/openzeppelin-community-contracts
Length of output: 14908
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect rest of IERC7535 around the "burn" section
FILE="contracts/interfaces/IERC7535.sol"
echo "---- Tail of $FILE ----"
sed -n '200,280p' "$FILE" | cat -n
# Exact search for burn in the interface
echo "---- Search burn in IERC7535 ----"
rg -n "\bfunction\s+burn\b" "$FILE" || true
# Search burn usage/definition in ERC7535
FILE2="contracts/token/ERC20/extensions/ERC7535.sol"
echo "---- Search burn in ERC7535 ----"
rg -n "\bfunction\s+burn\b|_burn\(" "$FILE2" || trueRepository: OpenZeppelin/openzeppelin-community-contracts
Length of output: 2146
🌐 Web query:
Solidity 0.8.24 requirement override keyword implementing interface function: is override optional when inheriting from interface only? wording "override keyword is optional" and rules for multiple bases
💡 Result:
In Solidity 0.8.24, the override keyword is optional when implementing a function that comes from a single interface [1]. Key rules regarding this behavior and multiple inheritance are: 1. Optionality: Starting from Solidity 0.8.8, if a function implementation overrides only a single interface function, the override keyword is not required [2][1]. 2. Mandatory Override: The override keyword remains mandatory when overriding a function defined in a contract (base class), or when a function is defined in multiple base contracts [2][3]. 3. Multiple Bases: When a contract inherits the same function from multiple base contracts, you must use the override keyword and explicitly specify the base contracts being overridden [2][3]. The syntax is override(Base1, Base2,...), where you list all base contracts that define the same function and have not yet been overridden by another base contract along the inheritance path [2][3]. 4. Interface Functions: While functions in interfaces are implicitly virtual, they do not require the override keyword unless they are subject to the multiple inheritance ambiguity rules mentioned above [2][1].
Citations:
- 1: https://www.soliditylang.org/blog/2021/09/27/solidity-0.8.8-release-announcement/
- 2: https://github.com/ethereum/solidity/blob/develop/docs/contracts/inheritance.rst
- 3: https://docs.soliditylang.org/en/latest/contracts.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="contracts/token/ERC20/extensions/ERC7535.sol"
rg -n "\{burn\}" "$FILE" || true
rg -n "\bburn\b" "$FILE" | head -n 20Repository: OpenZeppelin/openzeppelin-community-contracts
Length of output: 329
Fix incorrect NatSpec {burn} reference in ERC7535 docs
Solidity ^0.8.24 doesn’t require override for these IERC7535-only implementations, so the missing override isn’t a build blocker; however, ERC7535’s header NatSpec references {burn} even though the public exit API is withdraw/redeem (no burn exists in IERC7535), so update the docs accordingly (around lines 20-22).
🤖 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 `@contracts/token/ERC20/extensions/ERC7535.sol` around lines 135 - 252, The
NatSpec header for ERC7535 incorrectly references {burn} even though the public
exit API uses withdraw/redeem; update the contract/interface documentation on
ERC7535 (the top-of-file/header block for the ERC7535 contract) to replace or
remove the `{burn}` reference and instead reference the public exit functions
`withdraw` and `redeem` (or simply document the exit API as withdraw/redeem),
keeping all function names like deposit, mint, withdraw, redeem,
previewWithdraw, previewRedeem consistent with IERC7535.
Comment on lines
+370
to
+391
| function _assertReentrancyCEI(ReentrantReceiver.Kind kind) internal { | ||
| ReentrantReceiver r = new ReentrantReceiver(); | ||
|
|
||
| // Seed a second, honest depositor so the vault holds extra ETH the attacker could try to steal. | ||
| uint256 otherDeposit = 10 ether; | ||
| vm.deal(other, otherDeposit); | ||
| vm.prank(other); | ||
| uint256 otherShares = vault.deposit{value: otherDeposit}(otherDeposit, other); | ||
|
|
||
| // Attacker becomes a share owner. | ||
| uint256 attackerDeposit = 5 ether; | ||
| uint256 attackerShares = _fundReceiver(r, attackerDeposit); | ||
|
|
||
| // The attacker will try to re-redeem its FULL share balance again during the payout callback. | ||
| r.setup(vault, kind, attackerShares); | ||
|
|
||
| uint256 vaultBalBefore = address(vault).balance; | ||
| uint256 totalSupplyBefore = vault.totalSupply(); | ||
| uint256 expectedPayout = vault.previewRedeem(attackerShares); | ||
|
|
||
| vm.prank(address(r)); | ||
| vault.redeem(attackerShares, address(r), address(r)); |
There was a problem hiding this comment.
testReentrancyWithdrawCannotOverDrain never hits withdraw.
_assertReentrancyCEI always uses vault.redeem(...) for the outer payout, so the Kind.Withdraw case only changes the nested reentrant call. That leaves the withdraw entrypoint’s own asset-to-share path uncovered in this callback scenario.
Suggested fix
function _assertReentrancyCEI(ReentrantReceiver.Kind kind) internal {
ReentrantReceiver r = new ReentrantReceiver();
@@
uint256 vaultBalBefore = address(vault).balance;
uint256 totalSupplyBefore = vault.totalSupply();
uint256 expectedPayout = vault.previewRedeem(attackerShares);
vm.prank(address(r));
- vault.redeem(attackerShares, address(r), address(r));
+ if (kind == ReentrantReceiver.Kind.Withdraw) {
+ vault.withdraw(expectedPayout, address(r), address(r));
+ } else {
+ vault.redeem(attackerShares, address(r), address(r));
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function _assertReentrancyCEI(ReentrantReceiver.Kind kind) internal { | |
| ReentrantReceiver r = new ReentrantReceiver(); | |
| // Seed a second, honest depositor so the vault holds extra ETH the attacker could try to steal. | |
| uint256 otherDeposit = 10 ether; | |
| vm.deal(other, otherDeposit); | |
| vm.prank(other); | |
| uint256 otherShares = vault.deposit{value: otherDeposit}(otherDeposit, other); | |
| // Attacker becomes a share owner. | |
| uint256 attackerDeposit = 5 ether; | |
| uint256 attackerShares = _fundReceiver(r, attackerDeposit); | |
| // The attacker will try to re-redeem its FULL share balance again during the payout callback. | |
| r.setup(vault, kind, attackerShares); | |
| uint256 vaultBalBefore = address(vault).balance; | |
| uint256 totalSupplyBefore = vault.totalSupply(); | |
| uint256 expectedPayout = vault.previewRedeem(attackerShares); | |
| vm.prank(address(r)); | |
| vault.redeem(attackerShares, address(r), address(r)); | |
| function _assertReentrancyCEI(ReentrantReceiver.Kind kind) internal { | |
| ReentrantReceiver r = new ReentrantReceiver(); | |
| // Seed a second, honest depositor so the vault holds extra ETH the attacker could try to steal. | |
| uint256 otherDeposit = 10 ether; | |
| vm.deal(other, otherDeposit); | |
| vm.prank(other); | |
| uint256 otherShares = vault.deposit{value: otherDeposit}(otherDeposit, other); | |
| // Attacker becomes a share owner. | |
| uint256 attackerDeposit = 5 ether; | |
| uint256 attackerShares = _fundReceiver(r, attackerDeposit); | |
| // The attacker will try to re-redeem its FULL share balance again during the payout callback. | |
| r.setup(vault, kind, attackerShares); | |
| uint256 vaultBalBefore = address(vault).balance; | |
| uint256 totalSupplyBefore = vault.totalSupply(); | |
| uint256 expectedPayout = vault.previewRedeem(attackerShares); | |
| vm.prank(address(r)); | |
| if (kind == ReentrantReceiver.Kind.Withdraw) { | |
| vault.withdraw(expectedPayout, address(r), address(r)); | |
| } else { | |
| vault.redeem(attackerShares, address(r), address(r)); | |
| } |
🤖 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.t.sol` around lines 370 - 391, The test
helper _assertReentrancyCEI always calls vault.redeem(...) for the outer payout,
so the Kind.Withdraw case never exercises the vault.withdraw entrypoint; update
_assertReentrancyCEI to branch on ReentrantReceiver.Kind and call
vault.withdraw(expectedPayout, address(r), address(r)) when kind ==
ReentrantReceiver.Kind.Withdraw, otherwise keep the existing
vault.redeem(attackerShares, address(r), address(r)) call; adjust any local
variables used (e.g., expectedPayout vs attackerShares) so the correct argument
is passed for each branch.
| // Fund the holder with plenty of native asset. | ||
| await setBalance(this.holder.address, ethers.MaxUint256 / 2n); | ||
| // Approve spender over holder's shares for third-party withdraw/redeem paths. | ||
| await vault.$_approve(this.holder, this.spender, ethers.MaxUint256); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Test setup and approval-based exit cases =="
rg -n -C2 '\$_approve|withdraw with approval|redeem with approval|MaxUint256' test/token/ERC20/extensions/ERC7535.test.js
echo
echo "== ERC20 allowance spending logic =="
fd -i 'ERC20.sol' . | while read -r file; do
echo "-- $file"
rg -n -C3 '_spendAllowance|type\(uint256\)\.max|MaxUint256' "$file" || true
doneRepository: OpenZeppelin/openzeppelin-community-contracts
Length of output: 3178
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate ERC7535 implementation files =="
rg -n --hidden --no-ignore -S "contract .*ERC7535|ERC7535" contracts test | head -n 50
echo
echo "== Find delegated withdraw/redeem functions in ERC7535 =="
rg -n "withdraw\\(|redeem\\(|previewWithdraw\\(|previewRedeem\\(|_spendAllowance|_allowance|allowance\\(" contracts | head -n 80
echo
echo "== Inspect the test assertions around withdraw/redeem with approval =="
sed -n '380,460p' test/token/ERC20/extensions/ERC7535.test.jsRepository: OpenZeppelin/openzeppelin-community-contracts
Length of output: 14175
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== ERC7535.sol: locate _withdraw allowance spending =="
sed -n '260,330p' contracts/token/ERC20/extensions/ERC7535.sol
echo
echo "== ERC20.sol: locate _spendAllowance implementation =="
rg -n "function _spendAllowance" contracts/token/ERC20 -S
# Print the first matching implementation with a bit of context
file=$(rg -l "function _spendAllowance" contracts/token/ERC20 | head -n 1)
echo "-- $file"
sed -n '1,260p' "$file" | sed -n '/function _spendAllowance/,+60p'Repository: OpenZeppelin/openzeppelin-community-contracts
Length of output: 3549
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== ERC7535.test.js: find zero-address receiver assertions =="
rg -n "ZeroAddress|address\\(0\\)" test/token/ERC20/extensions/ERC7535.test.js
echo
echo "== ERC7535.test.js: inspect around the first zero-address receiver-related block =="
# Show around the first match (if any)
first=$(rg -n "ZeroAddress|address\\(0\\)" test/token/ERC20/extensions/ERC7535.test.js | head -n 1 | cut -d: -f1)
start=$(( first-40 ))
end=$(( first+60 ))
sed -n "${start},${end}p" test/token/ERC20/extensions/ERC7535.test.jsRepository: OpenZeppelin/openzeppelin-community-contracts
Length of output: 6526
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== ERC7535.test.js: inspect beforeEach + approval setup =="
sed -n '150,230p' test/token/ERC20/extensions/ERC7535.test.js
echo
echo "== ERC7535.test.js: inspect withdraw with approval / redeem with approval tests =="
sed -n '390,460p' test/token/ERC20/extensions/ERC7535.test.jsRepository: OpenZeppelin/openzeppelin-community-contracts
Length of output: 7660
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Find ERC20.sol in this repo =="
fd -i "erc20.sol" contracts | head -n 50
fd -i "ERC20.sol" contracts | head -n 50
echo
echo "== Locate _spendAllowance implementation (ERC20 base) =="
# Search in token/ERC20 area first
rg -n "function _spendAllowance\\(" contracts/token/ERC20 -S
echo
echo "== Show the implementation with surrounding context =="
file=$(rg -l "function _spendAllowance\\(" contracts/token/ERC20 -S | head -n 1)
echo "-- $file"
sed -n '1,260p' "$file" | sed -n '/function _spendAllowance\\(/,+80p'Repository: OpenZeppelin/openzeppelin-community-contracts
Length of output: 275
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Search whole contracts tree for _spendAllowance implementation =="
rg -n "function _spendAllowance\\(" contracts -S || true
echo
echo "== If none in contracts/, search repo root for _spendAllowance =="
rg -n "function _spendAllowance\\(" . -S || trueRepository: OpenZeppelin/openzeppelin-community-contracts
Length of output: 225
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Search for _spendAllowance references anywhere =="
rg -n "_spendAllowance" . -S || true
echo
echo "== Search for ERC20InsufficientAllowance error =="
rg -n "ERC20InsufficientAllowance" . -S || true
echo
echo "== List ERC20-related Solidity files (tokens) =="
ls -R contracts/token | head -n 200Repository: OpenZeppelin/openzeppelin-community-contracts
Length of output: 1829
Strengthen delegated withdraw/redeem tests to assert allowance is actually spent (and tighten the zero-address Withdraw event assertions)
- In
test/token/ERC20/extensions/ERC7535.test.js, the shared setup approvesethers.MaxUint256, and bothwithdraw with approval/redeem with approvalonly assert authorization (revert/no-revert). They don’t assert that allowance was reduced/exhausted after the delegated call, even thoughERC7535._withdrawcalls_spendAllowance(owner, caller, shares)before burning shares and transferring out native assets. Approve a finite amount (the required shares) and assert post-call allowance (or that a second call reverts withERC20InsufficientAllowance).
($_approveat ~194; tests at ~402-410 and ~434-440) - The
receiver == address(0) (documenting test)only checks the ETH balance change and thatWithdrawis emitted; it doesn’t assertWithdrawevent arguments, leaving the documented semantics under-specified.
(receiver==address(0) test around ~160-176)
🤖 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` at line 194, The tests currently
approve ethers.MaxUint256 via $_approve and only assert authorization; change
the delegated withdraw/redeem tests to approve a finite amount equal to the
shares being withdrawn (use the same shares value used in the call), then after
the delegated call assert the allowance was reduced to zero (or attempt a second
delegated call and assert it reverts with ERC20InsufficientAllowance) to verify
ERC7535._withdraw calls _spendAllowance(owner, caller, shares). Also tighten the
receiver == address(0) test by asserting the Withdraw event's full arguments
(owner, receiver==address(0), shares, amount) rather than only checking ETH
balance and that the event emitted.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR adds an implementation of ERC-7535: Native Asset ERC-4626 Tokenized Vault — an adaptation of ERC-4626 in which the underlying asset is the chain's native asset (e.g. Ether) instead of an ERC-20 token. It complements the vault standards already in this repository (
ERC4626Fees,ERC7540,IERC7575).What's included
contracts/interfaces/IERC7535.sol— the full ERC-4626 interface surface withpayabledeposit/mint. It intentionally does not inheritIERC4626: Solidity does not allow apayablefunction to override anonpayableone.contracts/token/ERC20/extensions/ERC7535.sol— abstract base implementation mirroring the structure, rounding directions, internal seams and virtual-offset anti-inflation math of theERC4626implementation in@openzeppelin/contracts.contracts/mocks/token/ERC7535OffsetMock.sol— mock with configurable decimals offset.docs/modules/ROOT/pages/erc7535.adoc), API doc registrations, and a changelog entry.Design decisions
asset()returns the ERC-7528 native-asset placeholder (0xEeee…EEeE). Per ERC-7528/7535, a vault backed by a wrapped native asset (e.g. WETH9) must use plain ERC-4626 instead; this is documented as anIMPORTANTnote.totalAssets()is balance-based (address(this).balance). Becausedeposit/mintarepayable, the in-flightmsg.valueis already part of the balance when the exchange rate would be computed — naively usingtotalAssets()would misprice every deposit. The conversion math therefore prices against an internal_pretotalAssets()(=totalAssets() - msg.value), while all public previews usetotalAssets()directly. Result: a standalonepreviewDeposit(assets)returns exactly the shares a subsequentdepositmints.msg.value:deposit(assets, receiver)requiresmsg.value == assets, andmint(shares, receiver)requiresmsg.value == previewMint(shares), reverting with typed errors otherwise. ERC-7535 allows ignoring theassetsargument, but ignoring it lets a buggy integrator silently desync intent from payment; a refund path would introduce an outbound native call inside the deposit flow (reentrancy / refund-griefing surface). Strict equality keeps theDepositevent provably consistent with the wei that entered the vault. The guide documents whydepositshould be preferred overmintfor ETH-in flows.Address.sendValue(full gas, keeping smart-contract receivers like multisigs and AA wallets compatible) and rely on checks-effects-interactions — allowance spent and shares burned before the outbound transfer — matchingERC4626's approach rather than adding a reentrancy guard.ERC4626— virtual shares/assets parameterized by_decimalsOffset(). Note that a native-asset vault can always be force-fed balance (SELFDESTRUCT, block-reward/coinbasepayments) bypassing anyreceiverestriction, so the offset math — not balance gating — is the defense. The NatSpec documents that overrides must keepoffset <= 77:10 ** offsetoverflowsuint256beyond that, which would brick every conversion (theuint8decimals()bound alone would misleadingly allow up to 237). Regression tests pin both sides of the 77/78 boundary.receive()reverts with a named error (ERC7535UnsolicitedDeposit) so plain transfers fail loudly instead of silently skewing the exchange rate. It isvirtual, so integrators that need plain native-asset payments to land (e.g. a rewards distributor on a chain without beacon-layer withdrawals) can deliberately open it.Testing
[0, 6, 18]across empty / donated / populated vault states;msg.valuemismatch reverts; unsolicited-transfer rejection; outbound-send failure to a reverting receiver; decimals/offset boundaries (237-ceiling overflow ofdecimals(), 77/78 conversion boundary); yield-accrual scenario adapted from the ERC-4626 suite.invariantSolvency,invariantNoValueCreation) driven by a randomized handler (5000 runs × 500 calls, 0 reverts).All repo checks pass locally:
npm run lint, Hardhat + Foundry suites,test:inheritance,test:pragma,test:generation, and docgen ({{ERC7535}}/{{IERC7535}}placeholders resolve).PR Checklist
Summary by CodeRabbit
New Features
Documentation
Tests