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

EIP-712 nonces unification discussion #4425

Open
k06a opened this issue Jul 5, 2023 · 3 comments
Open

EIP-712 nonces unification discussion #4425

k06a opened this issue Jul 5, 2023 · 3 comments

Comments

@k06a
Copy link
Contributor

k06a commented Jul 5, 2023

Related PR: #3850

Current nonces() are incremental sequences per owner (signer) and they interfere for Votes and ERC20Permit:

contract Nonces {
    function nonces(address owner) public view virtual returns (uint256);
}

contract ERC20Permit is Nonces {
    function permit(address owner, address spender, uint value, uint deadline, uint8 v, bytes32 r, bytes32 s) external;
}

contract Votes is Nonces {
    function delegateBySig(address delegatee, uint256 nonce, uint256 expiry, uint8 v, bytes32 r, bytes32 s) external;
}
@k06a
Copy link
Contributor Author

k06a commented Jul 5, 2023

Proposing following changes:

  1. Use top bit of nonce to indicate nonces are related to pair of owner (signer) and beneficiary (aka parallel nonces)
  2. Introduce replacement for nonces(address owner):
    • operationNonces(bytes32 typehash, address owner)
    • advanceOperationNonce(bytes32 typehash, uint64 count)
  3. Introduce extension to support invalidation per pair of owner and beneficiary:
    • operationNoncesPerBeneficiary(bytes32 typehash, address owner, address beneficiary)
    • advanceOperationNoncePerBeneficiery(bytes32 typehash, address beneficiary, uint64 count)
    • These nonces top bit should be equal to 1
  4. Introduce extension to support invalidation per operation:
    • useOperationHash(bytes32 hash) to invalidate specific operation by it's hash

Let's discuss something like this:

contract Nonces {
    mapping(bytes32 typehash => mapping(address owner => uint256 nonce)) public operationNonces;
    mapping(bytes32 typehash => mapping(address owner => mapping(address beneficiary => uint256 nonce))) private _operationNoncesPerBeneficiary;

    // Deprecated and kind of backward compatible
    function nonces(address owner) public view returns(uint256) {
        return operationNonces[ERC20Permit.TYPEHASH][owner];
    }

    function operationNoncesPerBeneficiary(bytes32 typehash, address owner, address beneficiary) public view returns(uint256) {
        return (1 << 255) | _operationNoncesPerBeneficiary[typehash][owner][beneficiary];
    }

    function advanceOperationNonce(bytes32 typehash, uint64 count) public {
        _advanceOperationNonce(typehash, msg.sender, count);
    }

    function advanceOperationNoncesPerBeneficiary(bytes32 typehash, address beneficiary, uint256 count) public {
        _advanceOperationNoncesPerBeneficiary(typehash, msg.sender, beneficiary, count);
    }

    function useNonce(bytes32 typehash, address owner, address beneficiary, uint256 nonce) public {
        if ((nonce >> 255) > 0) {
            require(nonce == _advanceOperationNoncesPerBeneficiary(typehash, owner, beneficiary, 1), "Wrong nonce");
        } else {
            require(nonce == _advanceOperationNonces(typehash, owner, 1), "Wrong nonce");
        }
    }

    function _advanceOperationNonce(bytes32 typehash, address owner, uint64 count) internal returns(uint256 nonce) {
        nonce = operationNonces[typehash][owner];
        operationNonces[typehash][owner] = nonce + 1;
    }

    function _advanceOperationNoncesPerBeneficiary(bytes32 typehash, address owner, address beneficiary, uint256 count) internal returns(uint256 nonce) {
        nonce = _operationNoncesPerBeneficiary[typehash][owner][beneficiary] | (1 << 255);
        _operationNoncesPerBeneficiary[typehash][owner][beneficiary] = nonce & (type(uint256).max >> 1) + 1;
    }
}

contract ERC20Permit is Nonces {
    function permit(address owner, address spender, uint value, uint deadline, uint8 v, bytes32 r, bytes32 s) external {
        uint256 nonce = nonces(owner);
        // …
        _advanceOperationNonce(owner, TYPEHASH, 1);
    }
}

contract Votes is Nonces {
    function delegateBySig(address delegatee, uint256 nonce, uint256 expiry, uint8 v, bytes32 r, bytes32 s) external {
        address owner = ecrecover(…);
        require(nonce == nonces(owner), "Wrong nonce");
        // …
        _advanceOperationNonce(owner, TYPEHASH, 1);
    }
}

// Example of usage
contract MyContract is Nonces {
    // Nonce as arg could be used per owner or per pair of owner and proposal
    function voteBySig(address proposal, uint256 nonce, uint256 expiry, uint8 v, bytes32 r, bytes32 s) external {
        address owner = ecrecover(…);
        useNonce(TYPEHASH, owner, proposal, nonce);
        // …
    }
}

@frangio
Copy link
Contributor

frangio commented Jul 5, 2023

Also related to #4420, an up to date version of #3850.

What's the purpose of using the top bit of the nonce when it's specific to a beneficiary? Couldn't it be a normal nonce starting from 0? That's what's implemented in #4420 currently.

@frangio
Copy link
Contributor

frangio commented Jul 5, 2023

As mentioned in #4420 (comment) we don't really want to add all these new functions to the standard ERC20Permit since it adds a lot of bytecode even though it won't be using the advanced functionality, so it's probably going to be an extension.

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

No branches or pull requests

2 participants