Replace OZ EIP712 (#256)

* WOOF WOOF WOOF BARK BARK WOOF BARK * remove address(this) check * forge fmt * discard version from EIP712 * Update src/base/EIP712.sol Co-authored-by: Sara Reynolds <[email protected]> * natspec for delegatecall error * assembly optimization, pr nits * test to validate assembly --------- Co-authored-by: Sara Reynolds <[email protected]>
Uniswap · Aug 4, 2024 · 0c956bf · 0c956bf
1 parent d65f158
commit 0c956bf
Show file tree

Hide file tree

Showing 15 changed files with 119 additions and 31 deletions.
diff --git a/.forge-snapshots/PositionManager_decrease_take_take.snap b/.forge-snapshots/PositionManager_decrease_take_take.snap
@@ -1 +1 @@
-116877
+116874
diff --git a/.forge-snapshots/PositionManager_multicall_initialize_mint.snap b/.forge-snapshots/PositionManager_multicall_initialize_mint.snap
@@ -1 +1 @@
-416513
+416535
diff --git a/.forge-snapshots/PositionManager_permit.snap b/.forge-snapshots/PositionManager_permit.snap
@@ -1 +1 @@
-79486
+79467
diff --git a/.forge-snapshots/PositionManager_permit_secondPosition.snap b/.forge-snapshots/PositionManager_permit_secondPosition.snap
@@ -1 +1 @@
-62398
+62355
diff --git a/.forge-snapshots/PositionManager_permit_twice.snap b/.forge-snapshots/PositionManager_permit_twice.snap
@@ -1 +1 @@
-45298
+45243
diff --git a/remappings.txt b/remappings.txt
@@ -1,5 +1,4 @@
 @uniswap/v4-core/=lib/v4-core/
-@openzeppelin/=lib/v4-core/lib/openzeppelin-contracts/
 forge-gas-snapshot/=lib/v4-core/lib/forge-gas-snapshot/src/
 ds-test/=lib/v4-core/lib/forge-std/lib/ds-test/src/
 forge-std/=lib/v4-core/lib/forge-std/src/

diff --git a/src/PositionManager.sol b/src/PositionManager.sol
@@ -60,7 +60,7 @@ contract PositionManager is
     constructor(IPoolManager _poolManager, IAllowanceTransfer _permit2)
         BaseActionsRouter(_poolManager)
         Permit2Forwarder(_permit2)
-        ERC721Permit("Uniswap V4 Positions NFT", "UNI-V4-POSM", "1")
+        ERC721Permit("Uniswap V4 Positions NFT", "UNI-V4-POSM")
     {}
 
     /// @notice Reverts if the deadline has passed

diff --git a/src/base/EIP712.sol b/src/base/EIP712.sol
@@ -0,0 +1,51 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.20;
+
+import {IEIP712} from "../interfaces/IEIP712.sol";
+
+/// @notice Generic EIP712 implementation
+/// @dev Maintains cross-chain replay protection in the event of a fork
+/// @dev Should not be delegatecall'd because DOMAIN_SEPARATOR returns the cached hash and does not recompute with the delegatecallers address
+/// @dev Reference: https://github.com/Uniswap/permit2/blob/main/src/EIP712.sol
+/// @dev Reference: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/cryptography/EIP712.sol
+contract EIP712 is IEIP712 {
+    // Cache the domain separator as an immutable value, but also store the chain id that it
+    // corresponds to, in order to invalidate the cached domain separator if the chain id changes.
+    bytes32 private immutable _CACHED_DOMAIN_SEPARATOR;
+    uint256 private immutable _CACHED_CHAIN_ID;
+    bytes32 private immutable _HASHED_NAME;
+
+    /// @dev equal to keccak256("EIP712Domain(string name,uint256 chainId,address verifyingContract)")
+    bytes32 private constant _TYPE_HASH = 0x8cad95687ba82c2ce50e74f7b754645e5117c3a5bec8151c0726d5857980a866;
+
+    constructor(string memory name) {
+        _HASHED_NAME = keccak256(bytes(name));
+
+        _CACHED_CHAIN_ID = block.chainid;
+        _CACHED_DOMAIN_SEPARATOR = _buildDomainSeparator();
+    }
+
+    /// @notice Returns the domain separator for the current chain.
+    /// @dev Uses cached version if chainid is unchanged from construction.
+    function DOMAIN_SEPARATOR() public view override returns (bytes32) {
+        return block.chainid == _CACHED_CHAIN_ID ? _CACHED_DOMAIN_SEPARATOR : _buildDomainSeparator();
+    }
+
+    /// @notice Builds a domain separator using the current chainId and contract address.
+    function _buildDomainSeparator() private view returns (bytes32) {
+        return keccak256(abi.encode(_TYPE_HASH, _HASHED_NAME, block.chainid, address(this)));
+    }
+
+    /// @notice Creates an EIP-712 typed data hash
+    function _hashTypedData(bytes32 dataHash) internal view returns (bytes32 digest) {
+        // equal to keccak256(abi.encodePacked("\x19\x01", DOMAIN_SEPARATOR(), dataHash));
+        bytes32 domainSeparator = DOMAIN_SEPARATOR();
+        assembly ("memory-safe") {
+            let fmp := mload(0x40)
+            mstore(fmp, hex"1901")
+            mstore(add(fmp, 0x02), domainSeparator)
+            mstore(add(fmp, 0x22), dataHash)
+            digest := keccak256(fmp, 0x42)
+        }
+    }
+}
diff --git a/src/base/ERC721Permit.sol b/src/base/ERC721Permit.sol
@@ -3,7 +3,7 @@ pragma solidity ^0.8.24;
 
 import {IERC721} from "forge-std/interfaces/IERC721.sol";
 import {ERC721} from "solmate/src/tokens/ERC721.sol";
-import {EIP712} from "@openzeppelin/contracts/utils/cryptography/EIP712.sol";
+import {EIP712} from "./EIP712.sol";
 import {ERC721PermitHashLibrary} from "../libraries/ERC721PermitHash.sol";
 import {SignatureVerification} from "permit2/src/libraries/SignatureVerification.sol";
 
@@ -16,15 +16,7 @@ abstract contract ERC721Permit is ERC721, IERC721Permit, EIP712, UnorderedNonce
     using SignatureVerification for bytes;
 
     /// @notice Computes the nameHash and versionHash
-    constructor(string memory name_, string memory symbol_, string memory version_)
-        ERC721(name_, symbol_)
-        EIP712(name_, version_)
-    {}
-
-    /// @inheritdoc IERC721Permit
-    function DOMAIN_SEPARATOR() external view returns (bytes32) {
-        return _domainSeparatorV4();
-    }
+    constructor(string memory name_, string memory symbol_) ERC721(name_, symbol_) EIP712(name_) {}
 
     /// @inheritdoc IERC721Permit
     function permit(address spender, uint256 tokenId, uint256 deadline, uint256 nonce, bytes calldata signature)
@@ -37,7 +29,7 @@ abstract contract ERC721Permit is ERC721, IERC721Permit, EIP712, UnorderedNonce
         if (spender == owner) revert NoSelfPermit();
 
         bytes32 hash = ERC721PermitHashLibrary.hash(spender, tokenId, nonce, deadline);
-        signature.verify(_hashTypedDataV4(hash), owner);
+        signature.verify(_hashTypedData(hash), owner);
 
         _useUnorderedNonce(owner, nonce);
         _approve(owner, spender, tokenId);

diff --git a/src/interfaces/IEIP712.sol b/src/interfaces/IEIP712.sol
@@ -0,0 +1,6 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.0;
+
+interface IEIP712 {
+    function DOMAIN_SEPARATOR() external view returns (bytes32);
+}
diff --git a/src/interfaces/IERC721Permit.sol b/src/interfaces/IERC721Permit.sol
@@ -8,10 +8,6 @@ interface IERC721Permit {
     error NoSelfPermit();
     error Unauthorized();
 
-    /// @notice The domain separator used in the permit signature
-    /// @return The domain seperator used in encoding of permit signature
-    function DOMAIN_SEPARATOR() external view returns (bytes32);
-
     /// @notice Approve of a specific token ID for spending by spender via signature
     /// @param spender The account that is being approved
     /// @param tokenId The ID of the token that is being approved for spending

diff --git a/test/EIP712.t.sol b/test/EIP712.t.sol
@@ -0,0 +1,47 @@
+//SPDX-License-Identifier: MIT
+pragma solidity ^0.8.24;
+
+import "forge-std/Test.sol";
+
+import {EIP712} from "../src/base/EIP712.sol";
+
+contract EIP712Test is EIP712, Test {
+    constructor() EIP712("EIP712Test") {}
+
+    function setUp() public {}
+
+    function test_domainSeparator() public view {
+        assertEq(
+            DOMAIN_SEPARATOR(),
+            keccak256(
+                abi.encode(
+                    keccak256("EIP712Domain(string name,uint256 chainId,address verifyingContract)"),
+                    keccak256(bytes("EIP712Test")),
+                    block.chainid,
+                    address(this)
+                )
+            )
+        );
+    }
+
+    function test_hashTypedData() public view {
+        bytes32 dataHash = keccak256(abi.encodePacked("data"));
+        assertEq(_hashTypedData(dataHash), keccak256(abi.encodePacked("\x19\x01", DOMAIN_SEPARATOR(), dataHash)));
+    }
+
+    function test_rebuildDomainSeparator() public {
+        uint256 chainId = 4444;
+        vm.chainId(chainId);
+        assertEq(
+            DOMAIN_SEPARATOR(),
+            keccak256(
+                abi.encode(
+                    keccak256("EIP712Domain(string name,uint256 chainId,address verifyingContract)"),
+                    keccak256(bytes("EIP712Test")),
+                    chainId,
+                    address(this)
+                )
+            )
+        );
+    }
+}
diff --git a/test/ERC721Permit.t.sol b/test/ERC721Permit.t.sol
@@ -19,13 +19,12 @@ contract ERC721PermitTest is Test {
 
     string constant name = "Mock ERC721Permit";
     string constant symbol = "MOCK721";
-    string constant version = "1";
 
     function setUp() public {
         (alice, alicePK) = makeAddrAndKey("ALICE");
         (bob, bobPK) = makeAddrAndKey("BOB");
 
-        erc721Permit = new MockERC721Permit(name, symbol, version);
+        erc721Permit = new MockERC721Permit(name, symbol);
     }
 
     // --- Test the overriden approval ---
@@ -75,12 +74,11 @@ contract ERC721PermitTest is Test {
 
     function test_domainSeparator() public view {
         assertEq(
-            IERC721Permit(address(erc721Permit)).DOMAIN_SEPARATOR(),
+            erc721Permit.DOMAIN_SEPARATOR(),
             keccak256(
                 abi.encode(
-                    keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"),
+                    keccak256("EIP712Domain(string name,uint256 chainId,address verifyingContract)"),
                     keccak256(bytes(name)),
-                    keccak256(bytes(version)),
                     block.chainid,
                     address(erc721Permit)
                 )

diff --git a/test/mocks/MockERC721Permit.sol b/test/mocks/MockERC721Permit.sol
@@ -6,7 +6,7 @@ import {ERC721Permit} from "../../src/base/ERC721Permit.sol";
 contract MockERC721Permit is ERC721Permit {
     uint256 public lastTokenId;
 
-    constructor(string memory name, string memory symbol, string memory version) ERC721Permit(name, symbol, version) {}
+    constructor(string memory name, string memory symbol) ERC721Permit(name, symbol) {}
 
     function tokenURI(uint256) public pure override returns (string memory) {
         return "";

diff --git a/test/position-managers/Permit.t.sol b/test/position-managers/Permit.t.sol
@@ -60,12 +60,11 @@ contract PermitTest is Test, PosmTestSetup {
 
     function test_domainSeparator() public view {
         assertEq(
-            IERC721Permit(address(lpm)).DOMAIN_SEPARATOR(),
+            ERC721Permit(address(lpm)).DOMAIN_SEPARATOR(),
             keccak256(
                 abi.encode(
-                    keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"),
+                    keccak256("EIP712Domain(string name,uint256 chainId,address verifyingContract)"),
                     keccak256("Uniswap V4 Positions NFT"), // storage is private on EIP712.sol so we need to hardcode these
-                    keccak256("1"),
                     block.chainid,
                     address(lpm)
                 )