Potential Design of ERC4626 Yield-bearing Vault Tokens Standard

ERC4626.yaml

# ERC4626: Yield-Bearing Vaults

# Definitions:
# - asset: The underlying token managed by the Vault.
#          Has units defined by the corresponding ERC20 contract.
# - share: The token of the Vault. Has a ratio of underlying tokens
#          exchanged on deposit/withdraw (defined by the vault)
# - slippage: any difference between advertised share price and economic realities of
#             depositto or withdrawal from the Vault, which is not accounted by fees
# - deposit fee: fee charged on deposit to Vault
# - performance fee: fee charged on the yield generated by the Vault
# - management fee: fee charged on deposited tokens while in Vault
# - withdrawal fee: fee charged on exit to the Vault

# NOTE: This standard does not consider any more complex fee structures than the above

implements:
# All ERC4626 implementations must be use ERC20 to represent shares
- ERC20

# ERC4626 users should consider using the Metadata extension of ERC20,
# and represent `name` and `symbol` as an extension to the underlying
- ERC20Metadata

# ERC4626 users should consider using ERC165 to inform outside integrators
# that they indeed comply to the ERC4626 interface, to ensure that
# upstream contracts do not add a Vault that does not comply to the
# interface, risking loss of funds for their contracts
- ERC165

ERC4626:

# The address of the underlying token used for the Vault
- name: asset
  type: function
  stateMutability: view

  outputs:
  - name: assetTokenAddress
    type: address  # Must be an ERC20

# The current exchange rate of shares to tokens
# NOTE: Tokens per unit share e.g. `10**Vault.decimmals()`
# NOTE: Should be inclusive of any management or performance fee
# NOTE: Should *not* be inclusive of any deposit or withdrawal fee
# NOTE: In certain types of fee calculations, this calculation will
#       not reflect the "per-user" price-per-share, and instead
#       should reflect the "average-user" price-per-share, meaning
#       what the average user should expect to exchange to and from.
#       `assetsOf` should be used for more accurate calculations.
# NOTE: This value may not be completely accurate according to
#       slippage or other on-chain conditions.
- name: pricePerShare
  type: function
  stateMutability: view

  outputs:
  - name: assetsPerUnitShare
    type: uint256

# Total number of underyling assets that managed by Vault
# NOTE: This should include any compounding that occurs from yield
# NOTE: This is the Value that any management fees should be charged against
- name: totalAssets
  type: function
  stateMutability: view
  
  outputs:
  - name: totalAssets
    type: uint256

# Total number of underlying tokens that a depositor's shares represent
# NOTE: This function is more accurate than using `pricePerShare` or
#       `totalAssets / vault.totalSupply` for certain fee calculations
# NOTE: This value may not be completely accurate according to
#       slippage or other on-chain conditions.
- name: assetsOf
  type: function
  stateMutability: view

  inputs:
  - name: depositor
    type: address

  outputs:
  - name: assets
    type: uint256

# Allow an on-chain or off-chain user to simulate the effects of their
# deposit at the current block and on-chain conditions.
# NOTE: This value should simulate as closely as possible the real
#       outcome of a deposit. Integrators to this contract expect the
#       represented slippage or loss to be under 1 basis point of accuracy.
- name: previewDeposit
  type: function
  stateMutability: view

  inputs:
  # Maximum number of assets they wish to deposit
  # NOTE: Vault will simulate attempt to deposit up to `maxAssets`
  - name: maxAssets
    type: uint256

  outputs:
  # Number of assets that Vault has simulated as deposited
  # NOTE: This number may be less than `maxAssets` to communicate a
  #       "deposit limit" or other such concept, where less than
  #       `maxAssets` was taken from depositor, and the rest was either
  #       refunded or simply not transferred in the first place
  #       e.g. `depositedAssets <= maxAssets`
  - name: depositedAssets
    type: uint256
  # Number of shares issued for `depositedAssets`
  # NOTE: any discrepency between `shares * pricePerShare`
  #       and `depositedTokens` should be considered slippage in share
  #       price or some other type of condition, meaning the depositor
  #       will lose assets by depositing
  # NOTE: can also be used to communicate a deposit fee
  - name: shares
    type: uint256

# Perform a deposit of assets from the caller to the vault
# NOTE: Conditions of deposit should match as closely as possible to
#       those described by `previewDeposit`. Any discrepency could
#       cause a revert due to tight slippage bounds by caller.
# NOTE: caller should pre-approve this deposit with `asset.approve`
- name: deposit
  type: function
  stateMutability: nonpayable

  inputs:
  # Party who should receive the deposited shares
  - name: receiver
    type: address
  # Maximum number of tokens to process in the deposit
  - name: assetsToDeposit
    type: uint256

  outputs:
  # Number of shares created and given to `receiver`
  # NOTE: Should revert if `sharesCreated < minShares`
  - name: sharesCreated
    type: uint256

# Allow an on-chain or off-chain user to simulate the effects of their
# mint at the current block and on-chain conditions.
# NOTE: This value should simulate as closely as possible the real
#       outcome of a share mint. Integrators to this contract expect the
#       represented slippage or loss to be under 1 basis point of accuracy.
- name: previewMint
  type: function
  stateMutability: view

  inputs:
  # The maximuim number of shares the depositor wishes to create
  # NOTE: Vault will simulate attempt to deposit and create up to `maxShares`
  - name: maxShares
    type: uint256

  outputs:
  # Number of assets that need to deposited to mint
  # NOTE: any discrepency between `pricePerShare / minAssets`
  #       and `sharesCreated` should be considered slippage in share
  #       price or some other type of condition, meaning the depositor
  #       will lose assets by depositing
  # NOTE: can also be used to communicate a deposit fee
  - name: minAssets
    type: uint256
  # Exact number of shares created for `minAssets`
  # NOTE: This number may be less than `maxShares` to communicate a
  #       "deposit limit" or other such concept, where less than
  #       the expected number of shares were created.
  #       e.g. `sharesCreated <= maxShares`
  - name: sharesCreated
    type: uint256

# Perform a mint of shares from the Vault to the caller
# NOTE: Conditions of mint should match as closely as possible to
#       those described by `previewMint`. Any discrepency could
#       cause a revert due to tight slippage bounds by caller.
# NOTE: caller should pre-approve this minting with `asset.approve`
- name: mint
  type: function
  stateMutability: nonpayable

  inputs:
  # Party who should receive the deposited shares
  - name: receiver
    type: address
  # Maximum number of shares to create in the mint
  - name: sharesToMint
    type: uint256

  outputs:
  # Number of assets deposited to the Vault
  - name: assetsAccepted
    type: uint256
    
# Event emitted when tokens are deposited into the vault
- name: Deposit
  type: event

  inputs:
  # The user who triggered the deposit
  - name: sender
    indexed: true
    type: address
  # The user who is able to withdraw the created shares
  - name: receiver
    indexed: true
    type: address
  # Number of underlying tokens sent to the vault
  - name: value
    indexed: false
    type: uint256

# Allow an on-chain or off-chain user to simulate the effects of their
# withdrawal at the current block and on-chain conditions.
# NOTE: This value should simulate as closely as possible the real
#       outcome of a withdrawal. Integrators to this contract expect the
#       represented slippage or loss to be under 1 basis point of accuracy.
- name: previewWithdraw
  type: function
  stateMutability: view
  
  inputs:
  # Maximum number of shares they wish to withdraw
  # NOTE: Vault will simulate attempt to withdraw up to `maxShares`
  - name: maxShares
    type: uint256
  
  outputs:
  # Number of shares that Vault has redeemed
  # NOTE: This number may be less than `maxShares` to communicate a
  #       "withdrawal limit" or other such concept, where less than
  #       `maxShares` was redeemed by depositor, and the rest was left
  #       untouched e.g. `redeemedShares <= maxShares`
  - name: redeemedShares
    type: uint256
  # Number of tokens transferred out for `redeemedShares`
  # NOTE: any discrepency between `maxAssets / pricePerShare`
  #       and `redeemedShares` should be considered slippage in share
  #       price meaning the depositor has lost tokens by withdrawing
  # NOTE: `maxTokens` is only an *estimate* of the amount that will be
  #       withdrawn, and should not have to fully perform a withdrawal,
  #       but simply be as accurate as possible to the withdrawal amount
  #       so as to be useful for depositors to gauge potential slippage.
  # NOTE: can also be used to communicate a withdrawal fee
  - name: assets
    type: uint256

# Perform a redemption of shares from the caller's balance
# NOTE: Conditions of withdrawal should match as closely as possible to
#       those described by `previewWithdraw`. Any discrepency could
#       cause a revert due to tight slippage bounds by caller.
- name: withdraw
  type: function
  stateMutability: nonpayable

  inputs:
  # Number of shares they wish to redeem
  - name: sharesToRedeem
    type: uint256
  # Party who should receive redeemed tokens
  - name: receiver
    type: address
  
  outputs:
  # Number of tokens redeemed and sent to receiver
  # NOTE: Integrators to untrusted Vaults should indepedently check
  #       token balance increase
  - name: tokensWithdrawn
    type: uint256

# Same as `withdraw`, but shares are being redeemed on behalf of
# another party who has previous approved via Vault's ERC20 approval flow
- name: withdrawFrom
  type: function
  stateMutability: nonpayable

  inputs:
  # Owner who shares should be redeemed from
  # NOTE: `owner` should pre-approve this redemption with Vault
  - name: owner
    type: address
  # Same as `withdraw`
  - name: receiver
    type: address
  # Same as `withdraw`
  - name: sharesToRedeem
    type: uint256
  
  outputs:
  # Same as `withdraw`
  - name: tokensWithdrawn
    type: uint256
    
# Event emitted when tokens are withdrawn from the vault by a depositor.
- name: Withdraw
  type: event

  inputs:
  # The user who triggered the withdrawal
  - name: owner
    indexed: true
    type: address
  # The user who received the withdrawn tokens
  - name: receiver
    indexed: true
    type: address
  # The number of underlying tokens sent out of the vault
  - name: value
    indexed: false
    type: uint256
    
ERC4626SlippageProtection:
# Add this optional extension if you would like to give users
# the ability to specify what amount of slippage loss they are
# comfortable with.
# NOTE: Not needed in many cases.

# Same as `ERC4626.deposit`
- name: deposit
  type: function
  stateMutability: nonpayable

  inputs:
  # Same as `ERC4626.deposit`
  - name: receiver
    type: address
  # Same as `ERC4626.deposit`
  - name: assetsToDeposit
    type: uint256
  # Minimum number of shares to be accepted by depositor
  # Default:
  #   `minShares = assetsToDeposit / pricePerShare`
  # NOTE: Only used as a guide for untrusted Vaults, integrators
  #       should enforce that `minShares <= sharesCreated` afterwards
  - name: minShares
    optional: true
    type: uint256

  outputs:
  # Same as `ERC4626.deposit`
  # NOTE: Should revert if `sharesCreated < minShares`
  - name: sharesCreated
    type: uint256
    
# Same as `ERC4626.withdraw`
- name: withdraw
  type: function
  stateMutability: nonpayable

  inputs:
  # Same as `ERC4626.withdraw`
  - name: sharesToRedeem
    type: uint256
  # PSame as `ERC4626.withdraw`
  - name: receiver
    type: address
  # Minimum number of tokens to be accepted by depositor
  # Default:
  #   `minTokens = sharesToRedeem * pricePerShare`
  # NOTE: Only used as a guide for untrusted Vaults, integrators
  #       should enforce that `minTokens <= tokensWithdrawn` afterwards
  - name: minTokens
    optional: true
    type: uint256
  
  outputs:
  # Same as `ERC4626.withdraw`
  # NOTE: Should revert if `tokensWithdrawn < minTokens`
  - name: tokensWithdrawn
    type: uint256

I think a depositFor is needed in order to avoid to transfer tokens two times (Like there https://github.com/fei-protocol/ERC4626/blob/main/src/ERC4626Router.sol) since a staking contract or router contract could be the one executing the deposit, the prerequisite would be for the caller to allow the ERC4626 with an allowance. Any fee should be taken at ERC4626 and not at router or staking contract level.

For pricePerShare, I think it is important for transparency to keep exchangeRate for proper accounting of the value of shares vs total number of underlying tokens, and also have a way to get the exchange rate after tax through this additional pricePerShare function.

Line 139, # Number of underlying tokens received by the fault (instead of "sent to the vault" since there are tokens with transfer tax)

I think a depositFor is needed in order to avoid to transfer tokens two times

Not sure I see where the double movement of funds is avoided in the linked example (I don't see depositFor). I believe it to be unavoidable, or at least I would not recommend approving a contract that allows any 3rd party to process a deposit on your behalf without your explicit consent through an intermediate call (for security reasons).

I am thinking you mean something like this:

function depositFor(address depositor, address receiver, ...) ... {
    token.transferFrom(depositor, address(this), ...);
    ...
    balanceOf[receiver] += ... // rug vector
    ...
}

you could limit it via:

function depositFor(address depositor, address receiver, ...) ... {
    token.transferFrom(depositor, address(this), ...);
    ...
    balanceOf[depositor] += ... // safe, but still no consent given to caller
    ...
}

But that also seems bad

---

Edit: if you simple mean that the ERC4626 contract could be a router that does token.transferFrom(msg.sender, someOtherThing) then yes that would work just fine, as long as an equal exchange of shares went out

Any fee should be taken at ERC4626 and not at router or staking contract level.

Yes I agree the fee should be computed at the interchange to shares. No sure I get the point you are trying to convey tho

For pricePerShare, I think it is important for transparency to keep exchangeRate for proper accounting of the value of shares vs total number of underlying tokens, and also have a way to get the exchange rate after tax through this additional pricePerShare function.

1. A lot I don't get about this sentence. What is the difference between pricePerShare vs. exchangeRate other than the naming? The addition of baseRate?
2. What do you mean by "tax"? Are you using that as a synonym for "fees"?
3. I think if I grok correctly you are meaning you'd like the ability to get the ratio of tokens to shares "pre-tax" (e.g. before any fees). If that is the case, I believe it to be actually less transparent to show people this metric because they will not be able to obtain it, and additionally they can get that value from Vault.totalSupply and Vault,totalTokens if needed.

Line 139, # Number of underlying tokens received by the fault (instead of "sent to the vault" since there are token with transfer tax)

Thank you!

Thanks for your reply, on depositFor, this piece of code:

    function depositToVault(
        IERC4626 vault,
        address to,
        uint256 amountUnderlying,
        uint256 minSharesOut
    ) external returns (uint256 sharesOut) {
        ERC20 underlying = vault.underlying();

        underlying.safeTransferFrom(msg.sender, address(this), amountUnderlying);
        underlying.safeApprove(address(vault), amountUnderlying);
        if ((sharesOut = vault.deposit(to, amountUnderlying)) < minSharesOut) {
            revert MinAmountError();
        }
    }

There is a first transfer from sender to router, then from router to vault, this is gas intensive, and for tokens with tax on transfer it involves two taxations.

If sender approve the ERC4626 to withdraw the underlying token, instead of approving the router/staking contract, it would be more efficient, so we would have the following code for instance:

    function depositToVault(
        IERC4626 vault,
        address to,
        uint256 amountUnderlying,
        uint256 minSharesOut
    ) external returns (uint256 sharesOut) {
        ERC20 underlying = vault.underlying();

        // This is now avoided underlying.safeTransferFrom(msg.sender, address(this), amountUnderlying);
        // underlying.safeApprove(address(vault), amountUnderlying);
        if ((sharesOut = vault.depositFor(to, amountUnderlying)) < minSharesOut) {
            revert MinAmountError();
        }
    }

if only the sender can benefit from this deposit where would you see an attack vector ?

if only the sender can benefit from this deposit where would you see an attack vector

Let's say I've given an unlimited approval to the ERC4626 Vault. I'm a normal ape, I forget that I did that 5 seconds later.

Anyways, 2 weeks later the ERC4626 is hacked. The hack is still in progress. The ape had a lot of tokens in the Vault, so they pull out as many funds as they can as quickly as they can, before it all goes to 0. Phew, the ape saved like 80% of their funds!

3 days later the ape logs back into their computer, and... wait! All the funds are gone!

You see in this example, the hacker found that the ERC4626 also allowed a non-consensual deposit to the Vault from any user in the past who had approved it, for any amount of tokens they have. They leveraged this to their advantage and go through and sweep everyone's tokens by forcing a deposit and executing the attack again.

TL;DR - non-consensual deposits are bad. Practice safe depositing, ensure you trigger the method that processes your deposit for you.

Indeed tax = fee in my replies.

Regarding exchangeRate, It would be easier to keep it without any fee to simplify the code, and use the additional function you propose, pricePerShare for the price at the point of withdrawal.

We are thinking of applying different fees based on the duration of the deposit for instance.

For the attack, same hack could happen at router level, allowance should be strictly limited to the amount that is to be deposited. And I had proposed (and I am not alone) that we also use the ERC2612 permit.

We are thinking of applying different fees based on the duration of the deposit for instance.

Ah okay, now I see. Yes then Vault.tokensOf(depositor) would reflect this. Time based-fees was something I didn't consider.

I had specifically excluded deposit/withdrawal fees from this price though, to fit with the spirit of what you're saying (don't included fees based on context, only for what the "average" user of the Vault would see at this point in time)

Would it be okay to use tokensOf(depositor) to represent this more explicit case, and then use pricePerShare to represent the time- and size-weighted average of all depositors' share prices?

For the attack, same hack could happen at router level, allowance should be strictly limited to the amount that is to be deposited.

But ape doesn't know this because ape is just an ape lol

And I had proposed (and I am not alone) that we also use the ERC2612 permit.

Deposit w/ Permit seems like a good optional extension, as it would only be used directly by EOA accounts. Or another EIP.

Would it be okay to use tokensOf(depositor) to represent this more explicit case, and then use pricePerShare to represent the time- and size-weighted average of all depositors' share prices?

Yes I think it would be ok, over which period of time would this price be calculated ?
I also was thinking of adding an APY function.

Yes I think it would be ok, over which period of time would this price be calculated ?

The current average of all depositors, probably tracked by a continuously updating variable. I meant for that function to sort of serve as an "instantanous" gauge of current value, but I guess on average. Actually, maybe it makes more sense just to get rid of it, if it's contextual, it's better off just using tokensOf(depositor) instead to get an accurate reading.

I also was thinking of adding an APY function.

APY functions are really hard. And if you make it contextual to the depositor... ngmi lol. I think it should perhaps be an extension or separate EIP.