Add ERC20TransferAuthorization following ERC-3009#6354
Add ERC20TransferAuthorization following ERC-3009#6354ernestognw wants to merge 11 commits intoOpenZeppelin:masterfrom
Conversation
🦋 Changeset detectedLatest commit: a3db8c7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
ernestognw
force-pushed
the
feat/erc-3009
branch
from
34f182b to
481470f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
contracts/token/ERC20/extensions/ERC7540Deposit.sol (1)
5-104: Consider removing or implementing the commented-out skeleton.
As shipped, the file compiles to no contract and the large commented block can drift or confuse; a small stub/TODO or a real implementation would be clearer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/token/ERC20/extensions/ERC7540Deposit.sol` around lines 5 - 104, The file contains a large commented-out abstract contract ERC7540Deposit which leaves the source delivering no contract and can confuse maintainers; either remove this dead block or implement a minimal compiling stub: uncomment and implement ERC7540Deposit as an abstract contract inheriting ERC165, ERC7540Operator, IERC7540Deposit and define the referenced state (uint256 _totalPendingDepositAssets, mapping _pendingDeposit, mapping _claimableDeposit, struct ClaimableDeposit) and the methods shown (totalAssets, pendingDepositRequest, claimableDepositRequest, supportsInterface, requestDeposit, mint, _checkAuthorized, _requestId) — or replace the block with a short TODO-stub comment explaining why it’s omitted; ensure function signatures match IERC7540Deposit and ERC7540Operator (e.g., totalAssets, requestDeposit, mint, supportsInterface) so the file stays consistent and compiles.contracts/token/ERC20/extensions/ERC7540Operator.sol (1)
12-20: Either use_underlyingDecimalsor remove it.
It’s computed in the constructor but never read; if it’s meant for decimals logic, expose/use it (e.g., in_decimalsOffsetor a decimals override) or drop it to save bytecode.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/token/ERC20/extensions/ERC7540Operator.sol` around lines 12 - 20, The _underlyingDecimals private immutable field is computed in the constructor but never used; either remove the field and the tryGetDecimals logic from the constructor to save bytecode, or wire it into the contract's decimals handling (for example expose it via an override of decimals() or use it to compute _decimalsOffset) so the value is actually read. Locate the constructor and the _underlyingDecimals declaration and either delete both the storage and its assignment OR add a decimals() override (or incorporate into _decimalsOffset calculation) that returns or uses _underlyingDecimals so the stored value is utilized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@contracts/token/ERC20/extensions/ERC7540Deposit.sol`:
- Around line 5-104: The file contains a large commented-out abstract contract
ERC7540Deposit which leaves the source delivering no contract and can confuse
maintainers; either remove this dead block or implement a minimal compiling
stub: uncomment and implement ERC7540Deposit as an abstract contract inheriting
ERC165, ERC7540Operator, IERC7540Deposit and define the referenced state
(uint256 _totalPendingDepositAssets, mapping _pendingDeposit, mapping
_claimableDeposit, struct ClaimableDeposit) and the methods shown (totalAssets,
pendingDepositRequest, claimableDepositRequest, supportsInterface,
requestDeposit, mint, _checkAuthorized, _requestId) — or replace the block with
a short TODO-stub comment explaining why it’s omitted; ensure function
signatures match IERC7540Deposit and ERC7540Operator (e.g., totalAssets,
requestDeposit, mint, supportsInterface) so the file stays consistent and
compiles.
In `@contracts/token/ERC20/extensions/ERC7540Operator.sol`:
- Around line 12-20: The _underlyingDecimals private immutable field is computed
in the constructor but never used; either remove the field and the
tryGetDecimals logic from the constructor to save bytecode, or wire it into the
contract's decimals handling (for example expose it via an override of
decimals() or use it to compute _decimalsOffset) so the value is actually read.
Locate the constructor and the _underlyingDecimals declaration and either delete
both the storage and its assignment OR add a decimals() override (or incorporate
into _decimalsOffset calculation) that returns or uses _underlyingDecimals so
the stored value is utilized.
|
There's an issue with ERC-3009: it defines interfaces with ECDSA-specific parameters // ERC-3009 standard - ECDSA only
function transferWithAuthorization(..., uint8 v, bytes32 r, bytes32 s) external;As a solution to support ERC-1271 I added accepting // Standard interface (ECDSA)
function transferWithAuthorization(..., uint8 v, bytes32 r, bytes32 s) public
// Extension for contract wallets (ERC-1271)
function transferWithAuthorization(..., bytes memory signature) publicHowever, I've been thinking that it may not necessary if we assume that smart accounts can batch, but under that logic, EOAs can also batch with 7702 delegations. At this point I think it's better to include the overload although it's not standard, and perhaps have a dedicated flow for I'm curious in other's opinions if we move forward with this PR |
contracts/token/ERC20/extensions/ERC20TransferAuthorization.sol
Outdated
Show resolved
Hide resolved
contracts/token/ERC20/extensions/ERC20TransferAuthorization.sol
Outdated
Show resolved
Hide resolved
Amxx
left a comment
Amxx
left a comment
There was a problem hiding this comment.
Its really not great that this use a nonce system that si so expensive (one fresh sstore per operation) that doesn't support any sequentiallity. IMO it would be way better if we used the NoncesKeyed approach.
We would need to propose that to the ERC.
Why skip the SignatureChecker ? |
I think we can still use NoncesKeyed since the ERC doesn't mandate fully parallel nonces, right? I would be more interested in having the ERC to add the interface function variants with
The flow that uses |
contracts/token/ERC20/extensions/ERC20TransferAuthorization.sol
Outdated
Show resolved
Hide resolved
All that is true, but I was affraid this efficiency would comes with a complexity cost. If we need two paths with duplicated internal function, just to make sure we get the separation at signature verification, its error prone and harder to verify. I see the current code handles that early. This is great. |
| * @dev Returns the domain separator used in the encoding of the signature for | ||
| * {transferWithAuthorization} and {receiveWithAuthorization}, as defined by {EIP712}. | ||
| */ | ||
| // solhint-disable-next-line func-name-mixedcase | ||
| function DOMAIN_SEPARATOR() external view returns (bytes32) { | ||
| return _domainSeparatorV4(); | ||
| } |
There was a problem hiding this comment.
We don't need that, do we?
Unless the ERC mandates this function, we should NOT expose this. EIP-712 already comes with ERC5267
There was a problem hiding this comment.
True. We have it in ERC20Permit. We should deprecate it and remove for 6.0
Not exactly. The NoncesKeyed forces sequentiallity within a "key". This means the left part (16 bytes) of the bytes32 can be anything, but the right part (16 bytes) has to be sequential. A fully random bytes32 will not be accespted as a valid nonce by NoncesKeyed. |
Yeah true, using afaik, the only ERC requirement is replay protection, which is handled already |
WalkthroughThe pull request introduces ERC-3009 signed transfer authorization support. It adds interface definitions (IERC3009 and IERC3009Cancel) for authorizing, executing, and canceling transfers via signatures. An ERC20 extension (ERC20TransferAuthorization) implements these interfaces with EIP-712-based signature validation, nonce tracking, and time-window validation. The implementation supports both raw and ERC-1271 wallet signatures. Test helpers are updated with EIP-712 type definitions for the authorization operations, and a comprehensive test suite validates authorization workflows including signature validation, nonce reuse prevention, and time constraints. 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Comment |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
contracts/token/ERC20/extensions/ERC20TransferAuthorization.sol (1)
70-87: Duplicated hash computation across v,r,s and bytes overloads.Each operation (transfer, receive, cancel) computes the EIP-712 hash identically in both its
(v,r,s)and(bytes)overloads. Per the earlier feedback to packabi.encodePacked(r,s,v)inside the v,r,s public function and delegate to the bytes path, the current structure still maintains two separate verification flows with duplicated hash logic.Also applies to: 90-104
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/token/ERC20/extensions/ERC20TransferAuthorization.sol` around lines 70 - 87, The transferWithAuthorization(v,r,s) overload duplicates the EIP-712 hash/comparison logic; instead pack the signature as abi.encodePacked(r, s, v) and delegate to the existing bytes-signature overload (e.g., transferWithAuthorization(..., bytes memory signature)) so the bytes overload performs the _hashTypedDataV4 and ECDSA.tryRecover check once; apply the same refactor pattern to the corresponding receiveWithAuthorization and cancelAuthorization v,r,s overloads so all hash/recover logic lives in the bytes overloads and the v,r,s variants only pack and forward the signature.
🧹 Nitpick comments (3)
test/token/ERC20/extensions/ERC20TransferAuthorization.test.js (1)
216-264: Consider adding a test for sequential ordering within the same key.The "parallel keys" test verifies key independence, but there's no test asserting that nonces within the same key must be consumed in order (e.g., using
seq=1beforeseq=0should revert withInvalidAccountNonce). This would directly validate the keyed sequential nonce guarantee documented in the contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/token/ERC20/extensions/ERC20TransferAuthorization.test.js` around lines 216 - 264, Add a new test that verifies nonces for the same derived key must be consumed in sequence: reuse packNonce to create nonceA = packNonce(key, 0n) and nonceB = packNonce(key, 1n) with the same key, build signatures via buildData and holder.signTypedData, then call transferWithAuthorization using nonceB first and assert it reverts with the InvalidAccountNonce error (or expected revert message), and finally call transferWithAuthorization with nonceA and/or nonceB in proper order and assert authorizationState(holder, nonceA/nonceB) becomes true; reference the existing test 'works with different keys in parallel' for structure and use functions transferWithAuthorization, buildData, packNonce, authorizationState, and holder.signTypedData.contracts/token/ERC20/extensions/ERC20TransferAuthorization.sol (2)
60-62:authorizationStatetruncatesnonces()return value touint64.The
nonces()return type isuint256, but the comparison casts it touint64. While practically safe (reaching 2^64 uses per key is infeasible), this silent truncation could mask bugs ifNoncesKeyedinternals ever change. Consider adding a brief comment explaining the safety assumption.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/token/ERC20/extensions/ERC20TransferAuthorization.sol` around lines 60 - 62, The function authorizationState currently truncates nonces(authorizer, uint192(...)) to uint64 before comparing to uint64(uint256(nonce)); update it to compare the full uint256 values (e.g., compute uint192 idx = uint192(uint256(nonce) >> 64) and return nonces(authorizer, idx) > uint256(nonce)) or, if you prefer to keep the cast, add a clear comment in authorizationState explaining the safety assumption that nonces() will never exceed 2^64 and why truncation is acceptable; reference the authorizationState function and the nonces(...) call when making the change.
188-205:_receiveWithAuthorizationerror type may mislead integrators.Line 197 uses
ERC20InvalidReceiver(to)when the caller isn't thetoaddress. This error is typically associated with invalid receiver addresses (e.g., zero address) in the ERC-20 standard errors. A caller-mismatch scenario is semantically different. That said, I see this was already discussed and accepted in prior review rounds, so flagging only as a minor consideration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/token/ERC20/extensions/ERC20TransferAuthorization.sol` around lines 188 - 205, The require in _receiveWithAuthorization currently throws ERC20InvalidReceiver(to) on caller mismatch which is misleading; change the check in _receiveWithAuthorization to revert with a semantically correct error (e.g., introduce and use ERC3009CallerNotRecipient(_msgSender(), to) or ERC3009InvalidRecipient(to)) instead of ERC20InvalidReceiver(to), add the corresponding error declaration, and update any tests or callers that expect the old error to use the new error symbol; locate the check in function _receiveWithAuthorization and replace the error token and declaration consistently throughout the contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@contracts/token/ERC20/extensions/ERC20TransferAuthorization.sol`:
- Around line 70-87: The transferWithAuthorization(v,r,s) overload duplicates
the EIP-712 hash/comparison logic; instead pack the signature as
abi.encodePacked(r, s, v) and delegate to the existing bytes-signature overload
(e.g., transferWithAuthorization(..., bytes memory signature)) so the bytes
overload performs the _hashTypedDataV4 and ECDSA.tryRecover check once; apply
the same refactor pattern to the corresponding receiveWithAuthorization and
cancelAuthorization v,r,s overloads so all hash/recover logic lives in the bytes
overloads and the v,r,s variants only pack and forward the signature.
---
Nitpick comments:
In `@contracts/token/ERC20/extensions/ERC20TransferAuthorization.sol`:
- Around line 60-62: The function authorizationState currently truncates
nonces(authorizer, uint192(...)) to uint64 before comparing to
uint64(uint256(nonce)); update it to compare the full uint256 values (e.g.,
compute uint192 idx = uint192(uint256(nonce) >> 64) and return
nonces(authorizer, idx) > uint256(nonce)) or, if you prefer to keep the cast,
add a clear comment in authorizationState explaining the safety assumption that
nonces() will never exceed 2^64 and why truncation is acceptable; reference the
authorizationState function and the nonces(...) call when making the change.
- Around line 188-205: The require in _receiveWithAuthorization currently throws
ERC20InvalidReceiver(to) on caller mismatch which is misleading; change the
check in _receiveWithAuthorization to revert with a semantically correct error
(e.g., introduce and use ERC3009CallerNotRecipient(_msgSender(), to) or
ERC3009InvalidRecipient(to)) instead of ERC20InvalidReceiver(to), add the
corresponding error declaration, and update any tests or callers that expect the
old error to use the new error symbol; locate the check in function
_receiveWithAuthorization and replace the error token and declaration
consistently throughout the contract.
In `@test/token/ERC20/extensions/ERC20TransferAuthorization.test.js`:
- Around line 216-264: Add a new test that verifies nonces for the same derived
key must be consumed in sequence: reuse packNonce to create nonceA =
packNonce(key, 0n) and nonceB = packNonce(key, 1n) with the same key, build
signatures via buildData and holder.signTypedData, then call
transferWithAuthorization using nonceB first and assert it reverts with the
InvalidAccountNonce error (or expected revert message), and finally call
transferWithAuthorization with nonceA and/or nonceB in proper order and assert
authorizationState(holder, nonceA/nonceB) becomes true; reference the existing
test 'works with different keys in parallel' for structure and use functions
transferWithAuthorization, buildData, packNonce, authorizationState, and
holder.signTypedData.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
contracts/interfaces/draft-IERC3009.solcontracts/token/ERC20/extensions/ERC20TransferAuthorization.soltest/helpers/eip712-types.jstest/token/ERC20/extensions/ERC20TransferAuthorization.test.js
ernestognw
changed the title
|
Closes #2436 |
Now that you mention this. I'm realizing the extension must be prefixed with |
This comment was marked as spam.
This comment was marked as spam.
gonzaotc
left a comment
gonzaotc
left a comment
There was a problem hiding this comment.
Looks good. Although slightly differs from the ERC on the "unique random nonces" point, I tend to agree that optional sequentiality via NoncesKeyed offers the most flexible path
| /** | ||
| * @dev Returns the state of an authorization. | ||
| * | ||
| * Nonces are randomly generated 32-byte values unique to the authorizer's address. | ||
| */ | ||
| function authorizationState(address authorizer, bytes32 nonce) external view returns (bool); |
There was a problem hiding this comment.
Can we be a bit more specific in the comments here about the boolean returned value?
i.e, what does it mean the authorization state to be returned as true or false?
There was a problem hiding this comment.
Without a clear explanation of the returned boolean, true sounds to me like the authorization state is "positive", while it is actually the other way around (the nonce is invalid, has been consumed already)
| _cancelAuthorization(authorizer, nonce); | ||
| } | ||
|
|
||
| /// @dev Internal version of {transferWithAuthorization} that accepts a bytes signature. |
There was a problem hiding this comment.
_transferWithAuthorization, _receiveWithAuthorization and _cancelAuthorization NatSpec claims that they receive a bytes signature, while they don't
| * | ||
| * It's a good idea to use the same `name` that is defined as the ERC-20 token name. | ||
| */ | ||
| constructor(string memory name) EIP712(name, "1") {} |
There was a problem hiding this comment.
If we ever do contract MyToken is ERC20Permit, ERC20TransferAuthorization {...} would this cause a conflict ?
There was a problem hiding this comment.
Confirmed, it would cause
DeclarationError: Base constructor arguments given twice.
--> contracts/mocks/token/ERC20TransferAuthorizationBlockNumberMock.sol:9:1:
|
9 | abstract contract ERC20TransferAuthorizationBlockNumberMock is ERC20Permit, ERC20TransferAuthorization {
| ^ (Relevant source part starts here and spans across multiple lines).
Note: First constructor call is here:
--> contracts/token/ERC20/extensions/ERC20Permit.sol:39:37:
|
39 | constructor(string memory name) EIP712(name, "1") {}
| ^^^^^^^^^^^^^^^^^
Note: Second constructor call is here:
--> contracts/token/ERC20/extensions/ERC20TransferAuthorization.sol:56:37:
|
56 | constructor(string memory name) EIP712(name, "1") {}
| ^^^^^^^^^^^^^^^^^
There was a problem hiding this comment.
Solutions I see:
- have
ERC20TransferAuthorizationinherit fromERC20Permit. That includes EIP712, and the corresponding constructor. - remove that constructor from
ERC20TransferAuthorization- if the contract inherit from
ERC20PermitandERC20TransferAuthorization, then the EIP constructor fromERC20Permitwould do the work - if the contract doesn't inherit from
ERC20Permit, then the user will have to callEIP721constructor itself.
- if the contract inherit from
| (address recovered, ECDSA.RecoverError err, ) = ECDSA.tryRecover(hash, v, r, s); | ||
| require(err == ECDSA.RecoverError.NoError && recovered == from, ERC3009InvalidSignature()); |
There was a problem hiding this comment.
What about using the ECDSA custom errors to be more explicit when the failure is caused by an improper signature format ?
| (address recovered, ECDSA.RecoverError err, ) = ECDSA.tryRecover(hash, v, r, s); | |
| require(err == ECDSA.RecoverError.NoError && recovered == from, ERC3009InvalidSignature()); | |
| require(from == ECDSA.recover(hash, v, r, s), ERC3009InvalidSignature()); |
| (address recovered, ECDSA.RecoverError err, ) = ECDSA.tryRecover(hash, v, r, s); | ||
| require(err == ECDSA.RecoverError.NoError && recovered == from, ERC3009InvalidSignature()); |
There was a problem hiding this comment.
discussion here : https://github.com/OpenZeppelin/openzeppelin-contracts/pull/6354/changes#r2883684250
| (address recovered, ECDSA.RecoverError err, ) = ECDSA.tryRecover(hash, v, r, s); | |
| require(err == ECDSA.RecoverError.NoError && recovered == from, ERC3009InvalidSignature()); | |
| require(from == ECDSA.recover(hash, v, r, s), ERC3009InvalidSignature()); |
| (address recovered, ECDSA.RecoverError err, ) = ECDSA.tryRecover(hash, v, r, s); | ||
| require(err == ECDSA.RecoverError.NoError && recovered == authorizer, ERC3009InvalidSignature()); |
There was a problem hiding this comment.
discussion here : https://github.com/OpenZeppelin/openzeppelin-contracts/pull/6354/changes#r2883684250
| (address recovered, ECDSA.RecoverError err, ) = ECDSA.tryRecover(hash, v, r, s); | |
| require(err == ECDSA.RecoverError.NoError && recovered == authorizer, ERC3009InvalidSignature()); | |
| require(from == ECDSA.recover(hash, v, r, s), ERC3009InvalidSignature()); |
4 participants
Fixes #????
PR Checklist
npx changeset add)