diff --git a/.forge-snapshots/PositionManager_decrease_take_take.snap b/.forge-snapshots/PositionManager_decrease_take_take.snap index 1e5f58d8..59ec6411 100644 --- a/.forge-snapshots/PositionManager_decrease_take_take.snap +++ b/.forge-snapshots/PositionManager_decrease_take_take.snap @@ -1 +1 @@ -116877 \ No newline at end of file +116874 \ No newline at end of file diff --git a/.forge-snapshots/PositionManager_multicall_initialize_mint.snap b/.forge-snapshots/PositionManager_multicall_initialize_mint.snap index 05da9fe8..806bba59 100644 --- a/.forge-snapshots/PositionManager_multicall_initialize_mint.snap +++ b/.forge-snapshots/PositionManager_multicall_initialize_mint.snap @@ -1 +1 @@ -416513 \ No newline at end of file +416535 \ No newline at end of file diff --git a/.forge-snapshots/PositionManager_permit.snap b/.forge-snapshots/PositionManager_permit.snap index dfab40b6..296e82ca 100644 --- a/.forge-snapshots/PositionManager_permit.snap +++ b/.forge-snapshots/PositionManager_permit.snap @@ -1 +1 @@ -79486 \ No newline at end of file +79467 \ No newline at end of file diff --git a/.forge-snapshots/PositionManager_permit_secondPosition.snap b/.forge-snapshots/PositionManager_permit_secondPosition.snap index 19350c74..3f96aa23 100644 --- a/.forge-snapshots/PositionManager_permit_secondPosition.snap +++ b/.forge-snapshots/PositionManager_permit_secondPosition.snap @@ -1 +1 @@ -62398 \ No newline at end of file +62355 \ No newline at end of file diff --git a/.forge-snapshots/PositionManager_permit_twice.snap b/.forge-snapshots/PositionManager_permit_twice.snap index ecadddb0..913eee8f 100644 --- a/.forge-snapshots/PositionManager_permit_twice.snap +++ b/.forge-snapshots/PositionManager_permit_twice.snap @@ -1 +1 @@ -45298 \ No newline at end of file +45243 \ No newline at end of file diff --git a/remappings.txt b/remappings.txt index c7951f83..e7868fe9 100644 --- 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 index 131d4526..39bfe195 100644 --- 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 new file mode 100644 index 00000000..5b4ea0f5 --- /dev/null +++ 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 index 7a4acc4d..1605b79c 100644 --- 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 new file mode 100644 index 00000000..355c443f --- /dev/null +++ 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 index 646c1af0..29a6fe2a 100644 --- 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 new file mode 100644 index 00000000..badf6c9c --- /dev/null +++ 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 index 16f4f39f..53e3ecf8 100644 --- 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 index d6e82a3e..ba1c732d 100644 --- 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 index 7da608a2..91a22a25 100644 --- 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) )