pr feedback

.forge-snapshots/PositionManager_permit.snap

@@ -1 +1 @@
-79445
+79484

.forge-snapshots/PositionManager_permit_secondPosition.snap

@@ -1 +1 @@
-62333
+62372

.forge-snapshots/PositionManager_permit_twice.snap

@@ -1 +1 @@
-45221
+45260

src/base/ERC721Permit_v4.sol

@@ -17,18 +17,22 @@ abstract contract ERC721Permit_v4 is ERC721, IERC721Permit_v4, EIP712_v4, Unorde
     /// @notice Computes the nameHash and versionHash
     constructor(string memory name_, string memory symbol_) ERC721(name_, symbol_) EIP712_v4(name_) {}
 
+    modifier checkSignatureDeadline(uint256 deadline) {
+        if (block.timestamp > deadline) revert SignatureDeadlineExpired();
+        _;
+    }
+
     /// @inheritdoc IERC721Permit_v4
     function permit(address spender, uint256 tokenId, uint256 deadline, uint256 nonce, bytes calldata signature)
         external
         payable
+        checkSignatureDeadline(deadline)
     {
-        if (block.timestamp > deadline) revert DeadlineExpired();
-
         address owner = ownerOf(tokenId);
-        if (spender == owner) revert NoSelfPermit();
+        _checkNoSelfPermit(owner, spender);
 
-        bytes32 hash = ERC721PermitHashLibrary.hashPermit(spender, tokenId, nonce, deadline);
-        signature.verify(_hashTypedData(hash), owner);
+        bytes32 digest = ERC721PermitHashLibrary.hashPermit(spender, tokenId, nonce, deadline);
+        signature.verify(_hashTypedData(digest), owner);
 
         _useUnorderedNonce(owner, nonce);
         _approve(owner, spender, tokenId);
@@ -42,10 +46,8 @@ abstract contract ERC721Permit_v4 is ERC721, IERC721Permit_v4, EIP712_v4, Unorde
         uint256 deadline,
         uint256 nonce,
         bytes calldata signature
-    ) external payable {
-        if (block.timestamp > deadline) revert DeadlineExpired();
-
-        if (operator == owner) revert NoSelfPermit();
+    ) external payable checkSignatureDeadline(deadline) {
+        _checkNoSelfPermit(owner, operator);
 
         bytes32 hash = ERC721PermitHashLibrary.hashPermitForAll(operator, approved, nonce, deadline);
         signature.verify(_hashTypedData(hash), owner);
@@ -94,4 +96,8 @@ abstract contract ERC721Permit_v4 is ERC721, IERC721Permit_v4, EIP712_v4, Unorde
         return spender == ownerOf(tokenId) || getApproved[tokenId] == spender
             || isApprovedForAll[ownerOf(tokenId)][spender];
     }
+
+    function _checkNoSelfPermit(address owner, address permitted) internal pure {
+        if (owner == permitted) revert NoSelfPermit();
+    }
 }

src/interfaces/IERC721Permit_v4.sol

@@ -4,7 +4,7 @@ pragma solidity >=0.7.5;
 /// @title ERC721 with permit
 /// @notice Extension to ERC721 that includes a permit function for signature based approvals
 interface IERC721Permit_v4 {
-    error DeadlineExpired();
+    error SignatureDeadlineExpired();
     error NoSelfPermit();
     error Unauthorized();
 

test/erc721Permit/ERC721Permit.permit.t.sol

@@ -229,7 +229,7 @@ contract ERC721PermitTest is Test {
         assertEq(erc721Permit.nonces(alice, wordPos) & (1 << bitPos), 0);
     }
 
-    function test_fuzz_erc721Permit_deadlineExpired(address spender) public {
+    function test_fuzz_erc721Permit_SignatureDeadlineExpired(address spender) public {
         vm.prank(alice);
         uint256 tokenId = erc721Permit.mint();
 
@@ -252,7 +252,7 @@ contract ERC721PermitTest is Test {
 
         // -- Permit but deadline expired -- //
         vm.startPrank(spender);
-        vm.expectRevert(IERC721Permit_v4.DeadlineExpired.selector);
+        vm.expectRevert(IERC721Permit_v4.SignatureDeadlineExpired.selector);
         erc721Permit.permit(spender, tokenId, deadline, nonce, signature);
         vm.stopPrank();
 

test/erc721Permit/ERC721Permit.permitForAll.t.sol

@@ -130,9 +130,8 @@ contract ERC721PermitForAllTest is Test {
         assertEq(erc721Permit.nonces(alice, wordPos) & (1 << bitPos), 2); // 2 = 0010
     }
 
-    function test_fuzz_erc721permitForAll_nonceAlreadyUsed() public {
+    function test_fuzz_erc721permitForAll_nonceAlreadyUsed(uint256 nonce) public {
         // alice gives bob operator permissions
-        uint256 nonce = 1;
         _permitForAll(alicePK, alice, bob, true, nonce);
 
         // alice cannot reuse the nonce
@@ -147,11 +146,7 @@ contract ERC721PermitForAllTest is Test {
         vm.stopPrank();
     }
 
-    function test_fuzz_erc721permitForAll_unauthorized() public {
-        vm.prank(alice);
-        uint256 tokenId = erc721Permit.mint();
-
-        uint256 nonce = 1;
+    function test_fuzz_erc721permitForAll_invalidSigner(uint256 nonce) public {
         bytes32 digest = _getPermitForAllDigest(bob, true, nonce, block.timestamp);
 
         // bob attempts signing an approval for himself
@@ -177,7 +172,7 @@ contract ERC721PermitForAllTest is Test {
         assertEq(erc721Permit.nonces(alice, wordPos) & (1 << bitPos), 0);
     }
 
-    function test_fuzz_erc721permitForAll_deadlineExpired(address operator) public {
+    function test_fuzz_erc721permitForAll_SignatureDeadlineExpired(address operator) public {
         uint256 nonce = 1;
         uint256 deadline = block.timestamp;
         bytes32 digest = _getPermitForAllDigest(operator, true, nonce, deadline);
@@ -196,7 +191,7 @@ contract ERC721PermitForAllTest is Test {
 
         // -- PermitForAll but deadline expired -- //
         vm.startPrank(operator);
-        vm.expectRevert(IERC721Permit_v4.DeadlineExpired.selector);
+        vm.expectRevert(IERC721Permit_v4.SignatureDeadlineExpired.selector);
         erc721Permit.permitForAll(alice, operator, true, deadline, nonce, signature);
         vm.stopPrank();
 
@@ -214,13 +209,7 @@ contract ERC721PermitForAllTest is Test {
 
         uint256 nonce = 1;
         uint256 deadline = block.timestamp;
-        bytes32 digest = keccak256(
-            abi.encodePacked(
-                "\x19\x01",
-                erc721Permit.DOMAIN_SEPARATOR(),
-                keccak256(abi.encode(ERC721PermitHashLibrary.PERMIT_TYPEHASH, operator, tokenId, nonce, deadline))
-            )
-        );
+        bytes32 digest = _getPermitDigest(operator, tokenId, nonce, deadline);
 
         // alice signs a permit for operator
         (uint8 v, bytes32 r, bytes32 s) = vm.sign(alicePK, digest);
@@ -280,19 +269,12 @@ contract ERC721PermitForAllTest is Test {
     }
 
     /// @dev a nonce used in permit is unusable for permitForAll
-    function test_erc721PermitForAll_permitNonceUsed() public {
+    function test_fuzz_erc721PermitForAll_permitNonceUsed(uint256 nonce) public {
         vm.prank(alice);
         uint256 tokenId = erc721Permit.mint();
 
-        uint256 nonce = 1;
         uint256 deadline = block.timestamp;
-        bytes32 digest = keccak256(
-            abi.encodePacked(
-                "\x19\x01",
-                erc721Permit.DOMAIN_SEPARATOR(),
-                keccak256(abi.encode(ERC721PermitHashLibrary.PERMIT_TYPEHASH, bob, tokenId, nonce, deadline))
-            )
-        );
+        bytes32 digest = _getPermitDigest(bob, tokenId, nonce, deadline);
         // alice signs a permit for bob
         (uint8 v, bytes32 r, bytes32 s) = vm.sign(alicePK, digest);
         bytes memory signature = abi.encodePacked(r, s, v);
@@ -344,6 +326,20 @@ contract ERC721PermitForAllTest is Test {
         );
     }
 
+    function _getPermitDigest(address spender, uint256 tokenId, uint256 nonce, uint256 deadline)
+        internal
+        view
+        returns (bytes32 digest)
+    {
+        digest = keccak256(
+            abi.encodePacked(
+                "\x19\x01",
+                erc721Permit.DOMAIN_SEPARATOR(),
+                keccak256(abi.encode(ERC721PermitHashLibrary.PERMIT_TYPEHASH, spender, tokenId, nonce, deadline))
+            )
+        );
+    }
+
     // copied the private function from UnorderedNonce.sol
     function _getBitmapFromNonce(uint256 nonce) private pure returns (uint256 wordPos, uint256 bitPos) {
         wordPos = uint248(nonce >> 8);