Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ERC: AUTHCALL ERC-4626 Deposit/Withdraw Extension #378

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

fubuloubu
Copy link
Contributor

Collaborating with some folks to finalize this Draft, but should be ready shortly. Looking for well-formed-ness feedback from new ERC process editors

@eip-review-bot
Copy link
Collaborator

eip-review-bot commented Apr 15, 2024

File ERCS/erc-7687.md

Requires 1 more reviewers from @axic, @g11tech, @SamWilsn, @xinbenlv

ERCS/ERC-XXX.md Outdated
All EIP-3074 `commit`s have been pre-pended with two additional string values to ensure replay protection within the context of this contract.
The number of this ERC has been added to ensure that it does not conflict any other ERCs which may be defined together in the implementation contract.
The method name of each method called has been additionally added to ensure replayability protection between other methods in this ERC, which have similar order and type to the list of parameters.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Note that all operations defined by this ERC are _NOT_ authenticated _except_ via the EIP-3074 mechanism.
The replay protection defined by this ERC and EIP-3074 is considered enough to prevent a user action from being initated by another party that violates that user's original intent when signing for that action.
In particular, the value of `deadline` MUST be chosen such that it is sufficiently short enough that the user is unlikely to want to be able to perform the same action again, but long enough for that action to succeed (at most, several minutes is suggested, but timing depends on on-chain fee conditions and/or downstream processes such as relayer batch times, etc.)
As a last resort, the EIP-3074 mechanism of performing a transaction to increase the value of that account's `nonce` to reassure the user that a replay is not possible, however this is not a substitute for the proper handling of constraints within the implementation contract.
(Note that for some transactions where the deposit is initiated by the same EOA that has signed for the deposit, there are less concerns due to the change of the user's `nonce` once the action has been processed as a transaction)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving this unresolved as an important note to read for collaborators

ERCS/ERC-XXX.md Outdated Show resolved Hide resolved
@eip-review-bot eip-review-bot changed the title ERC: Add AUTHCALL Deposit/Withdrawal Extension for ERC-4626 Add ERC: AUTHCALL Deposit/Withdrawal Extension for ERC-4626 Tokenized Vaults Apr 15, 2024
@github-actions github-actions bot added w-ci and removed w-ci labels Apr 15, 2024
ERCS/erc-7687.md Outdated Show resolved Hide resolved
---
eip: 7687
title: AUTHCALL Deposit/Withdrawal Extension for ERC-4626 Tokenized Vaults
description: Allow Deposits and Withdrawals from Tokenized Vaults using EIP-3074 wallet signing.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this will fix it

Suggested change
description: Allow Deposits and Withdrawals from Tokenized Vaults using EIP-3074 wallet signing.
description: Allow Deposits and Withdrawals from Tokenized Vaults using [EIP-3074](https://eips.ethereum.org/EIPS/eip-3074) wallet signing.

ERCS/erc-7687.md Outdated Show resolved Hide resolved
ERCS/erc-7687.md Outdated Show resolved Hide resolved

Tokenized Vaults have been a successful DeFi standard that help tokenize assets into various smart contract-managed automations, represented by shares exchangable at a contract-determined rate.
However, the standard makes it clear that additional functionality should be added to allow EOA or delegated deposits, in order to ensure that the user actions are handled in a manner that protects their assets from unintended share devaluation.
There have been several attempts at extensiions that add this functionality, but with the rise of EIP-3074 intent-based delegated actions, it makes it possible to define a much simpler standard that enables this.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this is the fix

Suggested change
There have been several attempts at extensiions that add this functionality, but with the rise of EIP-3074 intent-based delegated actions, it makes it possible to define a much simpler standard that enables this.
There have been several attempts at extensiions that add this functionality, but with the rise of [EIP-3074](https://eips.ethereum.org/EIPS/eip-3074) intent-based delegated actions, it makes it possible to define a much simpler standard that enables this.

MUST compute `commit` (per EIP-3074 parlance) as `keccak(b"ERC-<TBD>" || b"mintWithAuth" || depositor || shares || maxAssets || receiver || deadline)`.

```yaml
- name: depositWithAuth

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minWithAuth ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure what this suggestion means

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the yaml the name variable seems to be wrong. Replace depositWithAuth with mintWithAuth

ERCS/erc-7687.md Outdated Show resolved Hide resolved
ERCS/erc-7687.md Outdated Show resolved Hide resolved
ERCS/erc-7687.md Outdated Show resolved Hide resolved
ERCS/erc-7687.md Outdated Show resolved Hide resolved
ERCS/erc-7687.md Show resolved Hide resolved
@eip-review-bot eip-review-bot changed the title Add ERC: AUTHCALL Deposit/Withdrawal Extension for ERC-4626 Tokenized Vaults Add ERC: AUTHCALL ERC4626 Deposit/Withdraw Extension Apr 20, 2024
@github-actions github-actions bot removed the w-ci label Apr 20, 2024
ERCS/erc-7687.md Outdated Show resolved Hide resolved
@eip-review-bot eip-review-bot changed the title Add ERC: AUTHCALL ERC4626 Deposit/Withdraw Extension Add ERC: AUTHCALL ERC-4626 Deposit/Withdraw Extension Apr 20, 2024
Copy link

The commit a9a3125 (as a parent of 18b4b6d) contains errors.
Please inspect the Run Summary for details.

@github-actions github-actions bot added the w-ci label Apr 20, 2024
Copy link
Contributor Author

@fubuloubu fubuloubu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still considering this optimization, I feel like it is nice as it allows slicing most of msg.data wholesale when computing commit (only need to concatenate this ERC ahead of it)

One thing to note for EIP-3074 discussions: assuming there is the common implementation of the contract where the first 4 bytes of calldata is the method identifier of the handler that handles the call from the call table, then this is sort of "locking" the user expectation that only that method will be the one valid for the auth signature.

I am also considering removing the ERC identifier string entirely from the commit pre-image since (again assuming a "normal" contract) it should not be possible to define two functions with the same exact function selector (also ignoring method ID collisions, which are discouraged)

### Definitions:

All definitions from ERC-4626 and EIP-3074 apply. Additionally, the following definitions are useful:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- `METHOD_ID`: The ABI v2 method identifier, which is computed as the first 4 bytes of the keccak hash of the method selector string, and is prepended to `msg.data` on most EVM message calls targeting another contract


MUST return the _exact_ amount of shares issued to `receiver`.

MUST compute `commit` (per EIP-3074 parlance) as `keccak(b"ERC-7687" || b"depositWithAuth" || depositor || assets || minShares || receiver || deadline)`.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
MUST compute `commit` (per EIP-3074 parlance) as `keccak(b"ERC-7687" || b"depositWithAuth" || depositor || assets || minShares || receiver || deadline)`.
MUST compute `commit` (per EIP-3074 parlance) as `keccak(b"ERC-7687" || METHOD_ID || depositor || assets || minShares || receiver || deadline)`.


MUST return the _exact_ amount of shares issued to `receiver`.

MUST compute `commit` (per EIP-3074 parlance) as `keccak(b"ERC-7687" || b"mintWithAuth" || depositor || shares || maxAssets || receiver || deadline)`.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
MUST compute `commit` (per EIP-3074 parlance) as `keccak(b"ERC-7687" || b"mintWithAuth" || depositor || shares || maxAssets || receiver || deadline)`.
MUST compute `commit` (per EIP-3074 parlance) as `keccak(b"ERC-7687" || METHOD_ID || depositor || shares || maxAssets || receiver || deadline)`.


MUST return the _exact_ amount of shares redeemed from `owner`.

MUST compute `commit` (per EIP-3074 parlance) as `keccak(b"ERC-7687" || b"withdrawWithAuth" || depositor || assets || maxShares || receiver || deadline)`.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
MUST compute `commit` (per EIP-3074 parlance) as `keccak(b"ERC-7687" || b"withdrawWithAuth" || depositor || assets || maxShares || receiver || deadline)`.
MUST compute `commit` (per EIP-3074 parlance) as `keccak(b"ERC-7687" || METHOD_ID || depositor || assets || maxShares || receiver || deadline)`.


MUST return the _exact_ amount of assets transferred to `receiver`.

MUST compute `commit` (per EIP-3074 parlance) as `keccak(b"ERC-7687" || b"redeemWithAuth" || depositor || shares || minAssets || receiver || deadline)`.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
MUST compute `commit` (per EIP-3074 parlance) as `keccak(b"ERC-7687" || b"redeemWithAuth" || depositor || shares || minAssets || receiver || deadline)`.
MUST compute `commit` (per EIP-3074 parlance) as `keccak(b"ERC-7687" || METHOD_ID || depositor || shares || minAssets || receiver || deadline)`.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants