Fix: ERC-4494 permit() does not consume the token nonce (signature replay / unrevocable approvals)#295
Open
thistehneisen wants to merge 1 commit into
Open
Conversation
permit() never incremented the per-token nonce (only _transfer did), so a permit signature stayed valid until the token moved: signatures were replayable and approve(address(0)) did not durably revoke an approval. Increment _nonces[tokenId] on every successful permit, in both the ERC-1271 and EOA branches, per ERC-4494, in ERC721Permit, ERC721HybridPermit and ERC721HybridPermitV2. Add a regression test.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix: ERC-4494
permit()does not consume the token nonce — signatures are replayable and approvals unrevocableSummary
ERC721Permit._permit()(and itsERC721HybridPermit/ERC721HybridPermitV2copies) never increments the per-token nonce. The nonce is only incremented in_transfer(). Because_buildPermitDigest()derives the EIP-712 digest from_nonces[tokenId], a permit signature stays valid for as long as the token is not transferred.Consequences:
approve(address(0), tokenId)does not durably revoke access: anyone can replay the original (still-valid) signature to re-instate the approval. The owner's only way to invalidate the signature is to transfer the token (the sole nonce bump), which is not a revocation mechanism any user or wallet UI expects.This deviates from ERC-4494, which states the
permitfunction MUST increment the nonce oftokenId. The bug is present in the V1 contracts and persists unchanged into V2.Severity
High — integrity/confidentiality of asset ownership. An owner who uses the contract's advertised gasless-permit feature once, then later "revokes" the approval, can have the NFT transferred out from under them. No privileged roles and no atypical configuration are required; the only mitigating factor is a short permit
deadline, while long/maximum deadlines are common in real integrations (and in this repo's own tests).Affected code
contracts/token/erc721/abstract/ERC721Permit.sol—_permit()/_buildPermitDigest()(root cause)contracts/token/erc721/abstract/ERC721HybridPermit.sol— same logiccontracts/token/erc721/abstract/ERC721HybridPermitV2.sol— same logic (issue carried into V2)Inherited by the shipped presets
ImmutableERC721,ImmutableERC721MintByID,ImmutableERC721V2.Note:
contracts/token/erc1155/abstract/ERC1155Permit.solis not affected — it correctly increments its nonce inside the digest builder and checks the owner. The ERC-721 path should match that behaviour.Root cause
ERC721Permit.sol:Proof of concept
Verified against the real presets using the repository's own test harness (
ERC721BaseTest). The test mints to an owner, has the owner sign a normal ERC-4494 permit, has the owner revoke viaapprove(address(0), …)repeatedly, shows each revocation is undone by replaying the original signature, and finally transfers the NFT away from the owner.Result:
(PoC test reproduced in the security report; not included in this PR.)
Proposed fix
Increment the nonce on every successful permit, in both the ERC-1271 and EOA branches, before/at the point the approval is granted — matching ERC-4494 and the existing
ERC1155Permitbehaviour. Apply the equivalent change in all three files.function _permit(address spender, uint256 tokenId, uint256 deadline, bytes memory sig) internal virtual { if (deadline < block.timestamp) { revert PermitExpired(); } bytes32 digest = _buildPermitDigest(spender, tokenId, deadline); // smart contract signature validation if (_isValidERC1271Signature(ownerOf(tokenId), digest, sig)) { + _nonces[tokenId]++; _approve(spender, tokenId); return; } address recoveredSigner = address(0); // EOA signature validation if (sig.length == 64) { recoveredSigner = ECDSA.recover(digest, bytes32(BytesLib.slice(sig, 0, 32)), bytes32(BytesLib.slice(sig, 32, 64))); } else if (sig.length == 65) { recoveredSigner = ECDSA.recover(digest, sig); } else { revert InvalidSignature(); } if (_isValidEOASignature(recoveredSigner, tokenId)) { + _nonces[tokenId]++; _approve(spender, tokenId); } else { revert InvalidSignature(); } }Optional hardening (separate, spec-alignment): change
_isValidEOASignatureto requirerecoveredSigner == ownerOf(tokenId)instead of_isApprovedOrOwner(recoveredSigner, tokenId), matching ERC-4494 and the ERC-1271 branch. This is not required to close the replay/revocation issue but tightens the accepted signer set.Backwards compatibility
nonces(tokenId)value in the digest and read it fresh per signature, so correctly-implemented integrations are unaffected.nonces(tokenId)will advance on permits in addition to transfers; any system that assumed the nonce only moved on transfer should readnonces()rather than infer it.Test plan
permit().permit();approve(address(0), tokenId)durably revokes (replay of the prior signature reverts withInvalidSignature).ERC721Permit.sol,ERC721HybridPermit.sol, andERC721HybridPermitV2.sol.Nils Putnins / OffSeq Cybersecurity
npu@offseq.com / https://offseq.com / https://radar.offseq.com