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 support for typehash and beneficiary specific nonces #4420

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/good-penguins-hammer.md
@@ -0,0 +1,5 @@
---
'openzeppelin-solidity': major
---

`Nonces`: support typehash and beneficiary specific nonces following ERC-6077 proposal.
84 changes: 76 additions & 8 deletions contracts/utils/Nonces.sol
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, we have to consider that this will add a lot of new functions to the token interfaces. Do we want to do that for ERC20Permit when it's not necessary? Starting to think that this should just be a separate feature.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

IMO the storage should be defined in Nonce.sol for all these usecase. Having storage split in 2 (when using the separate feature) is not something I would be happy with)

I also agree with not including all the public functions by default.

Where do we set the limit between the two parts ? Would Nonce contain the internal getters and setters, and the "extension" the public ones ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Would Nonce contain the internal getters and setters, and the "extension" the public ones ?

Yes this makes sense.

@@ -1,22 +1,74 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;

import {Context} from "./Context.sol";

/**
* @dev Provides tracking nonces for addresses. Nonces will only increment.
*/
abstract contract Nonces {
abstract contract Nonces is Context {
bytes32 internal constant DEFAULT_TYPEHASH = 0;

/**
* @dev The nonce used for an `account` is not the expected current nonce.
*/
error InvalidAccountNonce(address account, uint256 currentNonce);

mapping(address => uint256) private _nonces;
error InvalidFastForward(address account, uint256 currentNonce);

// signer → typehash → beneficiary (track) → nonce
mapping(address => mapping(bytes32 => mapping(bytes32 => uint256))) private _nonces;

/**
* @dev Returns an the next unused nonce for an address.
* @dev Returns the next unused nonce for an address.
*/
function nonces(address owner) public view virtual returns (uint256) {
return _nonces[owner];
return operationIds(DEFAULT_TYPEHASH, owner, 0);
}

/**
* @dev Returns the next unused nonce for an address and a typehash.
*/
function operationNonces(bytes32 typehash, address signer) public view virtual returns (uint256) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nonces(owner) comes from EIP-2616 but is a really bad name.

This one should be something like getUnusedOperationNonce?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

operationNonces (and other) are names defined in the 6077 proposal. We should probably take some time to discuss all the naming.

return operationIds(typehash, signer, 0);
}

/**
* @dev Returns the next unused nonce for an address, a typehash and a beneficiary.
*/
function operationIds(bytes32 typehash, address signer, bytes32 beneficiary) public view virtual returns (uint256) {
Copy link
Contributor

Choose a reason for hiding this comment

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

"Beneficiary" is not the right term, we should probably use "track"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Again, name comes from 6077. I like "track"

return _nonces[signer][typehash][beneficiary];
}

/**
* @dev Invalidate a chunk of nonces, up to `last` for the calling account and the specified typehash. After this
* call is executed, the next unused nonce for that account and that typehash will be `last + 1`.
*
* Requirements:
* - last must be at least the current nonce.
* - last must not invalidate more than 5000 nonces at once.
*/
function useOperationNonce(bytes32 typehash, uint256 last) public virtual {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I would remove this one, useOperationIds should be enough. Having many aliases/variations pollutes the interface.

useOperationIds(typehash, 0, last);
}

/**
* @dev Invalidate a chunk of nonces, up to `last` for the calling account and the specified typehash and
* beneficiary. After this call is executed, the next unused nonce for that account, that typehash and that
* beneficiary will be `last + 1`.
*
* Requirements:
* - last must be at least the current nonce.
* - last must not invalidate more than 5000 nonces at once.
*/
function useOperationIds(bytes32 typehash, bytes32 beneficiary, uint256 last) public virtual {
address caller = _msgSender();

uint256 current = _nonces[caller][typehash][beneficiary];
if (last < current || last > current + 5000) {
revert InvalidFastForward(caller, current);
}
_nonces[caller][typehash][beneficiary] = last + 1;
}

/**
Expand All @@ -25,21 +77,37 @@ abstract contract Nonces {
* Returns the current value and increments nonce.
*/
function _useNonce(address owner) internal virtual returns (uint256) {
return _useNonce(DEFAULT_TYPEHASH, owner, 0);
}

/**
* @dev Same as {_useNonce} but checking that `nonce` is the next valid for `owner`.
*/
function _useCheckedNonce(address owner, uint256 nonce) internal virtual returns (uint256) {
return _useCheckedNonce(DEFAULT_TYPEHASH, owner, 0, nonce);
}

/**
* @dev Consumes a nonce for a given typehash, signer and beneficiary.
*
* Returns the current value and increments nonce.
*/
function _useNonce(bytes32 typehash, address signer, bytes32 beneficiary) internal virtual returns (uint256) {
// For each account, the nonce has an initial value of 0, can only be incremented by one, and cannot be
// decremented or reset. This guarantees that the nonce never overflows.
unchecked {
// It is important to do x++ and not ++x here.
return _nonces[owner]++;
return _nonces[signer][typehash][beneficiary]++;
}
}

/**
* @dev Same as {_useNonce} but checking that `nonce` is the next valid for `owner`.
*/
function _useCheckedNonce(address owner, uint256 nonce) internal virtual returns (uint256) {
uint256 current = _useNonce(owner);
function _useCheckedNonce(bytes32 typehash, address signer, bytes32 beneficiary, uint256 nonce) internal virtual returns (uint256) {
uint256 current = _useNonce(typehash, signer, beneficiary);
if (nonce != current) {
revert InvalidAccountNonce(owner, current);
revert InvalidAccountNonce(signer, current);
}
return current;
}
Expand Down
3 changes: 2 additions & 1 deletion contracts/utils/cryptography/EIP712.sol
Expand Up @@ -6,6 +6,7 @@ pragma solidity ^0.8.19;
import {ECDSA} from "./ECDSA.sol";
import {ShortStrings, ShortString} from "../ShortStrings.sol";
import {IERC5267} from "../../interfaces/IERC5267.sol";
import {Context} from "../../utils/Context.sol";

/**
* @dev https://eips.ethereum.org/EIPS/eip-712[EIP 712] is a standard for hashing and signing of typed structured data.
Expand All @@ -32,7 +33,7 @@ import {IERC5267} from "../../interfaces/IERC5267.sol";
*
* @custom:oz-upgrades-unsafe-allow state-variable-immutable state-variable-assignment
*/
abstract contract EIP712 is IERC5267 {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is required for contract linearization.

abstract contract EIP712 is Context, IERC5267 {
using ShortStrings for *;

bytes32 private constant _TYPE_HASH =
Expand Down
4 changes: 2 additions & 2 deletions test/utils/Nonces.test.js
Expand Up @@ -21,7 +21,7 @@ contract('Nonces', function (accounts) {
expect(await this.nonces.nonces(sender)).to.be.bignumber.equal('0');

const { receipt } = await this.nonces.$_useNonce(sender);
expectEvent(receipt, 'return$_useNonce', ['0']);
expectEvent(receipt, 'return$_useNonce_address', ['0']);

expect(await this.nonces.nonces(sender)).to.be.bignumber.equal('1');
});
Expand All @@ -43,7 +43,7 @@ contract('Nonces', function (accounts) {
expect(currentNonce).to.be.bignumber.equal('0');

const { receipt } = await this.nonces.$_useCheckedNonce(sender, currentNonce);
expectEvent(receipt, 'return$_useCheckedNonce', [currentNonce]);
expectEvent(receipt, 'return$_useCheckedNonce_address_uint256', [currentNonce]);

expect(await this.nonces.nonces(sender)).to.be.bignumber.equal('1');
});
Expand Down