ERC7540 design proposal

[hieronx]

Implementation proposal for discussion of #4761

PR Checklist

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

[changeset-bot]

⚠️ No Changeset found

Latest commit: 2e5dcb0

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[ernestognw]

I'll stop here. After thoroughly reviewing the PR and looking at the forum discussion I'm not convinced this could be a contract in the library. As noted in the Ethereum Magicians forum, the initial design didn't intend the contract to be a "set in stone OpenZeppelin Contract", which I agree with.

As shown in the review, the implementation breaks in a couple ways around the library and includes some antipatterns that, although well-intentioned, become contradictory when trying to pursue an extendable implementation.

[ernestognw]

Why not to use ERC165 and call super?

function supportsInterface(bytes4 interfaceId) public view virtual override returns (bool) {
        return
            interfaceId == type(IERC7575).interfaceId ||
            interfaceId == type(IERC7540Operator).interfaceId ||
            super.supportsInterface(interfaceId);
    }

[ernestognw]

Why is this not inheriting from IERC7575? Especially if it's reported via supportsInterface

[ernestognw]

This should be just ERC7540. The concept of "Base" is redundant in the context of OOP and an abstract contract

Suggested change

	abstract contract BaseERC7540 is ERC4626, IERC7540Operator {
	abstract contract ERC7540 is ERC4626, IERC7540Operator {

[ernestognw]

Same applies for all the "Base" variants. The abstraction doesn't seem to be correct.

[ernestognw]

This whole file fits in 10 lines

Suggested change

	// SPDX-License-Identifier: MIT
	// OpenZeppelin Contracts (last updated v5.0.0) (interfaces/IERC7575.sol)
	pragma solidity ^0.8.20;
	import {IERC165} from "../utils/introspection/IERC165.sol";
	interface IERC7575 is IERC165 {
	event Deposit(address indexed sender, address indexed owner, uint256 assets, uint256 shares);
	event Withdraw(
	address indexed sender, address indexed receiver, address indexed owner, uint256 assets, uint256 shares
	);
	/**
	* @dev Returns the address of the underlying token used for the Vault for accounting, depositing, and withdrawing.
	*
	* - MUST be an ERC-20 token contract.
	* - MUST NOT revert.
	*/
	function asset() external view returns (address assetTokenAddress);
	/**
	* @dev Returns the address of the share token
	*
	* - MUST be an ERC-20 token contract.
	* - MUST NOT revert.
	*/
	function share() external view returns (address shareTokenAddress);
	/**
	* @dev Returns the amount of shares that the Vault would exchange for the amount of assets provided, in an ideal
	* scenario where all the conditions are met.
	*
	* - MUST NOT be inclusive of any fees that are charged against assets in the Vault.
	* - MUST NOT show any variations depending on the caller.
	* - MUST NOT reflect slippage or other on-chain conditions, when performing the actual exchange.
	* - MUST NOT revert.
	*
	* NOTE: This calculation MAY NOT reflect the “per-user” price-per-share, and instead should reflect the
	* “average-user’s” price-per-share, meaning what the average user should expect to see when exchanging to and
	* from.
	*/
	function convertToShares(uint256 assets) external view returns (uint256 shares);
	/**
	* @dev Returns the amount of assets that the Vault would exchange for the amount of shares provided, in an ideal
	* scenario where all the conditions are met.
	*
	* - MUST NOT be inclusive of any fees that are charged against assets in the Vault.
	* - MUST NOT show any variations depending on the caller.
	* - MUST NOT reflect slippage or other on-chain conditions, when performing the actual exchange.
	* - MUST NOT revert.
	*
	* NOTE: This calculation MAY NOT reflect the “per-user” price-per-share, and instead should reflect the
	* “average-user’s” price-per-share, meaning what the average user should expect to see when exchanging to and
	* from.
	*/
	function convertToAssets(uint256 shares) external view returns (uint256 assets);
	/**
	* @dev Returns the total amount of the underlying asset that is “managed” by Vault.
	*
	* - SHOULD include any compounding that occurs from yield.
	* - MUST be inclusive of any fees that are charged against assets in the Vault.
	* - MUST NOT revert.
	*/
	function totalAssets() external view returns (uint256 totalManagedAssets);
	/**
	* @dev Returns the maximum amount of the underlying asset that can be deposited into the Vault for the receiver,
	* through a deposit call.
	*
	* - MUST return a limited value if receiver is subject to some deposit limit.
	* - MUST return 2 ** 256 - 1 if there is no limit on the maximum amount of assets that may be deposited.
	* - MUST NOT revert.
	*/
	function maxDeposit(address receiver) external view returns (uint256 maxAssets);
	/**
	* @dev Allows an on-chain or off-chain user to simulate the effects of their deposit at the current block, given
	* current on-chain conditions.
	*
	* - MUST return as close to and no more than the exact amount of Vault shares that would be minted in a deposit
	* call in the same transaction. I.e. deposit should return the same or more shares as previewDeposit if called
	* in the same transaction.
	* - MUST NOT account for deposit limits like those returned from maxDeposit and should always act as though the
	* deposit would be accepted, regardless if the user has enough tokens approved, etc.
	* - MUST be inclusive of deposit fees. Integrators should be aware of the existence of deposit fees.
	* - MUST NOT revert.
	*
	* NOTE: any unfavorable discrepancy between convertToShares and previewDeposit SHOULD be considered slippage in
	* share price or some other type of condition, meaning the depositor will lose assets by depositing.
	*/
	function previewDeposit(uint256 assets) external view returns (uint256 shares);
	/**
	* @dev Mints shares Vault shares to receiver by depositing exactly amount of underlying tokens.
	*
	* - MUST emit the Deposit event.
	* - MAY support an additional flow in which the underlying tokens are owned by the Vault contract before the
	* deposit execution, and are accounted for during deposit.
	* - MUST revert if all of assets cannot be deposited (due to deposit limit being reached, slippage, the user not
	* approving enough underlying tokens to the Vault contract, etc).
	*
	* NOTE: most implementations will require pre-approval of the Vault with the Vault’s underlying asset token.
	*/
	function deposit(uint256 assets, address receiver) external returns (uint256 shares);
	/**
	* @dev Returns the maximum amount of the Vault shares that can be minted for the receiver, through a mint call.
	* - MUST return a limited value if receiver is subject to some mint limit.
	* - MUST return 2 ** 256 - 1 if there is no limit on the maximum amount of shares that may be minted.
	* - MUST NOT revert.
	*/
	function maxMint(address receiver) external view returns (uint256 maxShares);
	/**
	* @dev Allows an on-chain or off-chain user to simulate the effects of their mint at the current block, given
	* current on-chain conditions.
	*
	* - MUST return as close to and no fewer than the exact amount of assets that would be deposited in a mint call
	* in the same transaction. I.e. mint should return the same or fewer assets as previewMint if called in the
	* same transaction.
	* - MUST NOT account for mint limits like those returned from maxMint and should always act as though the mint
	* would be accepted, regardless if the user has enough tokens approved, etc.
	* - MUST be inclusive of deposit fees. Integrators should be aware of the existence of deposit fees.
	* - MUST NOT revert.
	*
	* NOTE: any unfavorable discrepancy between convertToAssets and previewMint SHOULD be considered slippage in
	* share price or some other type of condition, meaning the depositor will lose assets by minting.
	*/
	function previewMint(uint256 shares) external view returns (uint256 assets);
	/**
	* @dev Mints exactly shares Vault shares to receiver by depositing amount of underlying tokens.
	*
	* - MUST emit the Deposit event.
	* - MAY support an additional flow in which the underlying tokens are owned by the Vault contract before the mint
	* execution, and are accounted for during mint.
	* - MUST revert if all of shares cannot be minted (due to deposit limit being reached, slippage, the user not
	* approving enough underlying tokens to the Vault contract, etc).
	*
	* NOTE: most implementations will require pre-approval of the Vault with the Vault’s underlying asset token.
	*/
	function mint(uint256 shares, address receiver) external returns (uint256 assets);
	/**
	* @dev Returns the maximum amount of the underlying asset that can be withdrawn from the owner balance in the
	* Vault, through a withdraw call.
	*
	* - MUST return a limited value if owner is subject to some withdrawal limit or timelock.
	* - MUST NOT revert.
	*/
	function maxWithdraw(address owner) external view returns (uint256 maxAssets);
	/**
	* @dev Allows an on-chain or off-chain user to simulate the effects of their withdrawal at the current block,
	* given current on-chain conditions.
	*
	* - MUST return as close to and no fewer than the exact amount of Vault shares that would be burned in a withdraw
	* call in the same transaction. I.e. withdraw should return the same or fewer shares as previewWithdraw if
	* called
	* in the same transaction.
	* - MUST NOT account for withdrawal limits like those returned from maxWithdraw and should always act as though
	* the withdrawal would be accepted, regardless if the user has enough shares, etc.
	* - MUST be inclusive of withdrawal fees. Integrators should be aware of the existence of withdrawal fees.
	* - MUST NOT revert.
	*
	* NOTE: any unfavorable discrepancy between convertToShares and previewWithdraw SHOULD be considered slippage in
	* share price or some other type of condition, meaning the depositor will lose assets by depositing.
	*/
	function previewWithdraw(uint256 assets) external view returns (uint256 shares);
	/**
	* @dev Burns shares from owner and sends exactly assets of underlying tokens to receiver.
	*
	* - MUST emit the Withdraw event.
	* - MAY support an additional flow in which the underlying tokens are owned by the Vault contract before the
	* withdraw execution, and are accounted for during withdraw.
	* - MUST revert if all of assets cannot be withdrawn (due to withdrawal limit being reached, slippage, the owner
	* not having enough shares, etc).
	*
	* Note that some implementations will require pre-requesting to the Vault before a withdrawal may be performed.
	* Those methods should be performed separately.
	*/
	function withdraw(uint256 assets, address receiver, address owner) external returns (uint256 shares);
	/**
	* @dev Returns the maximum amount of Vault shares that can be redeemed from the owner balance in the Vault,
	* through a redeem call.
	*
	* - MUST return a limited value if owner is subject to some withdrawal limit or timelock.
	* - MUST return balanceOf(owner) if owner is not subject to any withdrawal limit or timelock.
	* - MUST NOT revert.
	*/
	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,
	* 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
	* in the same transaction. I.e. redeem should return the same or more assets as previewRedeem if called in the
	* same transaction.
	* - MUST NOT account for redemption limits like those returned from maxRedeem and should always act as though the
	* redemption would be accepted, regardless if the user has enough shares, etc.
	* - MUST be inclusive of withdrawal fees. Integrators should be aware of the existence of withdrawal fees.
	* - MUST NOT revert.
	*
	* NOTE: any unfavorable discrepancy between convertToAssets and previewRedeem SHOULD be considered slippage in
	* share price or some other type of condition, meaning the depositor will lose assets by redeeming.
	*/
	function previewRedeem(uint256 shares) external view returns (uint256 assets);
	/**
	* @dev Burns exactly shares from owner and sends assets of underlying tokens to receiver.
	*
	* - MUST emit the Withdraw event.
	* - MAY support an additional flow in which the underlying tokens are owned by the Vault contract before the
	* redeem execution, and are accounted for during redeem.
	* - MUST revert if all of shares cannot be redeemed (due to withdrawal limit being reached, slippage, the owner
	* not having enough shares, etc).
	*
	* NOTE: some implementations will require pre-requesting to the Vault before a withdrawal may be performed.
	* Those methods should be performed separately.
	*/
	function redeem(uint256 shares, address receiver, address owner) external returns (uint256 assets);
	}
	// SPDX-License-Identifier: MIT
	pragma solidity ^0.8.20;
	import {IERC165} from "./IERC165.sol";
	import {IERC4626} from "./IERC4626.sol";
	interface IERC7575 is IERC4626 {
	function share() external view returns (address);
	}

[ernestognw]

There's no reason to force a constructor if the contract is abstract

Suggested change

	/**
	* @dev Set the underlying asset contract. This must be an ERC20-compatible contract (ERC-20 or ERC-777).
	*/
	constructor(IERC20 asset_) ERC4626(asset_) {}

[ernestognw]

This should be a noop too

[ernestognw]

No need to perform a external call

Suggested change

	ERC20(address(this)).transfer(receiver, shares);
	transfer(receiver, shares);

[ernestognw]

Wouldn't be better if this behaves as a linear vesting? You wouldn't need the _fulfillDeposit, which I understand should move from pending state into claimable.

My intuition is that the transition can be automated

[ernestognw]

Suggested change

	shares = deposit(assets, receiver, receiver);
	shares = deposit(assets, receiver);

[ernestognw]

perhaps:

Suggested change

	shares = deposit(assets, receiver, receiver);
	return super.deposit(assets, receiver);

[ernestognw]

I'm afraid the deposit and mint functions with the controller argument are an antipattern since the ERC attempts to make this contract an extension to ERC4626. By definition, it branches this and the other deposit implementation (i.e. there's no way to share the logic cleanly between them as an extension).

I think there are 2 ways of solving this:

1. By repeating the deposit logic in ERC4626:

function deposit(uint256 assets, address receiver, address controller) public virtual returns (uint256) {
    require(controller == _msgSender() || isOperator(controller, _msgSender()));
    uint256 maxAssets = maxDeposit(receiver);
    if (assets > maxAssets) {
        revert ERC4626ExceededMaxDeposit(receiver, assets, maxAssets);
    }

    uint256 shares = previewDeposit(assets);
    _deposit(controller, receiver, assets, shares);

    return shares;
}

However, this approach has the issue that developers must take care of overriding 2 deposit functions instead of just one.

2. Refactor ERC4626 to branch out the validation logic. Perhaps:

function deposit(uint256 assets, address receiver) public virtual returns (uint256) {
    return _depositWithController(_msgSender(), assets, receiver);
}

function _depositWithController(
    address controller,
    uint256 assets,
    address receiver
) public virtual returns (uint256) {
    uint256 maxAssets = maxDeposit(receiver);
    if (assets > maxAssets) {
        revert ERC4626ExceededMaxDeposit(receiver, assets, maxAssets);
    }

    uint256 shares = previewDeposit(assets);
    _deposit(controller, receiver, assets, shares);

    return shares;
}

However, the second one introduces the concept of a controller in the context of ERC4626, which is also an antipattern (depends on a future child use case)