Code Improvement: consider implementing isValidSignature directly

[naszam]

Context: PermitERC721.sol#L100-L110

Description:
PermitERC721 is currently implementing permit following the standard ERC-1271 regarding isValidSignature using the openzeppelin-contracts IERC1271.sol interface to check the function selector if the signer is a contract as well and uses ecrecover for the other case.

Recommendation:
Consider implementing _isValidSignature directly (also known as isValidSignatureNow in openzeppelin-contracts, see here).
Improvements:

• Improve code structure, by handling the code complexity directly into _isValidSignature making the code more compact and easier to read and reason about.
• Remove external dependencies, such as IERC1271 and Address.sol where isContract is not required as if the signature lenght is not 65, the _isValidSignature will proceed to perform a staticcall to the signer address. (see related issue Code Size Optimization: Address.sol imported to use only isContract #30)
• Include more checks for the signature, ensuring is well formed (see related issue isValidSignature doesn't check for well formed signature #33)
• Introduce some gas optimizations as well, by removing the redundant check recoveredAddress != address(0) assuming current openzeppelin-contracts implementation doesn't change, where ownerOf ensures already that owner is always different than address(0).
  (see related issue Gas Optimization: consider removing redundant check in PermitERC721 #36)

diff --git a/src/base/PermitERC721.sol b/src/base/PermitERC721.sol
index a92acaf..39f4c04 100644
--- a/src/base/PermitERC721.sol
+++ b/src/base/PermitERC721.sol
@@ -2,10 +2,7 @@
 
 pragma solidity 0.8.14;
 
-import { IERC1271 } from '@openzeppelin/contracts/interfaces/IERC1271.sol';
 import { ERC721 }   from '@openzeppelin/contracts/token/ERC721/ERC721.sol';
-import { Address }  from '@openzeppelin/contracts/utils/Address.sol';
-
 
 /**
  *  @dev Interface for token permits for ERC-721
@@ -19,6 +16,17 @@ interface IPermit {
     ) external;
 }
 
+/**
+ *  @dev Interface of the ERC1271 standard signature validation method
+ */
+
+interface IERC1271 {
+    function isValidSignature(
+        bytes32 digest,
+        bytes memory signature
+    ) external view returns (bytes4);
+}
+
 /**
  *  @notice https://soliditydeveloper.com/erc721-permit
  *  @notice Functionality to enable `EIP-4494` permit calls as part of interactions with Position `NFT`s
@@ -65,6 +73,33 @@ abstract contract PermitERC721 is ERC721, IPermit {
             );
     }
 
+    function _isValidSignature(
+        address signer,
+        bytes32 digest,
+        bytes memory signature
+    ) internal view returns (bool) {
+        if (signature.length == 65) {
+            bytes32 r;
+            bytes32 s;
+            uint8 v;
+            assembly {
+                r := mload(add(signature, 0x20))
+                s := mload(add(signature, 0x40))
+                v := byte(0, mload(add(signature, 0x60)))
+            }
+            if (signer == ecrecover(digest, v, r, s)) {
+                return true;
+            }
+        }
+
+        (bool success, bytes memory result) = signer.staticcall(
+            abi.encodeWithSelector(IERC1271.isValidSignature.selector, digest, signature)
+        );
+        return (success &&
+            result.length == 32 &&
+            abi.decode(result, (bytes4)) == IERC1271.isValidSignature.selector);
+    }
+
     /**
      *  @notice Called by a `NFT` owner to enable a third party spender to interact with their `NFT`.
      *  @param spender_  The address of the third party who will execute the transaction involving an owners `NFT`.
@@ -97,17 +132,7 @@ abstract contract PermitERC721 is ERC721, IPermit {
         address owner = ownerOf(tokenId_);
         require(spender_ != owner, "ERC721Permit: approval to current owner");
 
-        if (Address.isContract(owner)) {
-            // bytes4(keccak256("isValidSignature(bytes32,bytes)") == 0x1626ba7e
-            require(
-                IERC1271(owner).isValidSignature(digest, abi.encodePacked(r_, s_, v_)) == 0x1626ba7e,
-                "ajna/nft-unauthorized"
-            );
-        } else {
-            address recoveredAddress = ecrecover(digest, v_, r_, s_);
-            require(recoveredAddress != address(0), "ajna/nft-invalid-signature");
-            require(recoveredAddress == owner, "ajna/nft-unauthorized");
-        }
+        require(_isValidSignature(owner, digest, abi.encodePacked(r_, s_, v_)), "ajna/invalid-signature");
 
         _approve(spender_, tokenId_);
     }

diff --git a/tests/forge/unit/PositionManager.t.sol b/tests/forge/unit/PositionManager.t.sol
index bf3aa40..e5566be 100644
--- a/tests/forge/unit/PositionManager.t.sol
+++ b/tests/forge/unit/PositionManager.t.sol
@@ -1348,7 +1348,7 @@ contract PositionManagerERC20PoolTest is PositionManagerERC20PoolHelperContract
         // check contract owned nft can't be signed by non owner
         uint256 deadline = block.timestamp + 1 days;
         (uint8 v, bytes32 r, bytes32 s) = _getPermitSig(address(spenderContract), tokenId, deadline, recipientContractOwnerPrivateKey);
-        vm.expectRevert("ajna/nft-unauthorized");
+        vm.expectRevert("ajna/invalid-signature");
         spenderContract.transferFromWithPermit(address(recipientContract), tokenId, deadline, v, r, s );
 
         // check owner can permit their contract to transfer the NFT
@@ -1380,13 +1380,13 @@ contract PositionManagerERC20PoolTest is PositionManagerERC20PoolHelperContract
         // check signer is authorized to permit
         deadline = block.timestamp + 1 days;
         (v, r, s) = _getPermitSig(testSpender, tokenId, deadline, receiverPrivateKey);
-        vm.expectRevert("ajna/nft-unauthorized");
+        vm.expectRevert("ajna/invalid-signature");
         spenderContract.transferFromWithPermit(testReceiver, tokenId, deadline, v, r, s );
 
         // check signature is valid
         deadline = block.timestamp + 1 days;
         (v, r, s) = _getPermitSig(testSpender, tokenId, deadline, minterPrivateKey);
-        vm.expectRevert("ajna/nft-invalid-signature");
+        vm.expectRevert("ajna/invalid-signature");
         spenderContract.transferFromWithPermit(testReceiver, tokenId, deadline, 0, r, s );
 
         // check can't self approve

• Test Pass
• Gas Reports:

Before

| src/PositionManager.sol:PositionManager contract |                 |        |        |         |         |
|--------------------------------------------------|-----------------|--------|--------|---------|---------|
| Deployment Cost                                  | Deployment Size |        |        |         |         |
| 3922538                                          | 19868           |        |        |         |         |
| Function Name                                    | min             | avg    | median | max     | # calls |
| permit                                           | 618             | 30939  | 27698  | 54387   | 7       |

After

| src/PositionManager.sol:PositionManager contract |                 |        |        |         |         |
|--------------------------------------------------|-----------------|--------|--------|---------|---------|
| Deployment Cost                                  | Deployment Size |        |        |         |         |
| 3920730                                          | 19859           |        |        |         |         |
| Function Name                                    | min             | avg    | median | max     | # calls |
| permit                                           | 618             | 32664  | 29375  | 58659   | 7       |

• Related Issues:
• ERC Compliance: PermitERC721 and PositionManager implementations doesn't match with ERC-4494 Draft #27

Ajna:

Prototech: