Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace OZ EIP712 #256

Merged
merged 11 commits into from
Aug 4, 2024
Original file line number Diff line number Diff line change
@@ -1 +1 @@
416516
416538
2 changes: 1 addition & 1 deletion .forge-snapshots/PositionManager_permit.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
79585
79660
Original file line number Diff line number Diff line change
@@ -1 +1 @@
62497
62572
2 changes: 1 addition & 1 deletion .forge-snapshots/PositionManager_permit_twice.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
45397
45472
1 change: 0 additions & 1 deletion remappings.txt
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
@uniswap/v4-core/=lib/v4-core/
@openzeppelin/=lib/v4-core/lib/openzeppelin-contracts/
saucepoint marked this conversation as resolved.
Show resolved Hide resolved
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/
Expand Down
48 changes: 48 additions & 0 deletions src/base/EIP712.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
// 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 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;
address private immutable _CACHED_THIS;
bytes32 private immutable _HASHED_NAME;
bytes32 private immutable _HASHED_VERSION;

bytes32 private constant _TYPE_HASH =
keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)");
saucepoint marked this conversation as resolved.
Show resolved Hide resolved

constructor(string memory name, string memory version) {
_HASHED_NAME = keccak256(bytes(name));
_HASHED_VERSION = keccak256(bytes(version));

_CACHED_CHAIN_ID = block.chainid;
_CACHED_DOMAIN_SEPARATOR = _buildDomainSeparator();
_CACHED_THIS = address(this);
}

/// @notice Returns the domain separator for the current chain.
/// @dev Uses cached version if chainid and address are unchanged from construction.
saucepoint marked this conversation as resolved.
Show resolved Hide resolved
function DOMAIN_SEPARATOR() public view override returns (bytes32) {
return address(this) == _CACHED_THIS && 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, _HASHED_VERSION, block.chainid, address(this)));
}

/// @notice Creates an EIP-712 typed data hash
function _hashTypedData(bytes32 dataHash) internal view returns (bytes32) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok i went through OZ's contracts to figure out why theirs is cheaper lol. If you rewrite this function as:

    function _hashTypedData(bytes32 dataHash) internal view returns (bytes32 digest) {
        bytes32 domainSeparator = DOMAIN_SEPARATOR();
        assembly("memory-safe") {
            let fmp := mload(0x40)
            mstore(fmp, hex"19_01")
            mstore(add(fmp, 0x02), domainSeparator)
            mstore(add(fmp, 0x22), dataHash)
            digest := keccak256(fmp, 0x42)
        }
    }

our gas will be in line with theirs again! Based on this function

return keccak256(abi.encodePacked("\x19\x01", DOMAIN_SEPARATOR(), dataHash));
}
}
9 changes: 2 additions & 7 deletions src/base/ERC721Permit.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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";

Expand All @@ -21,11 +21,6 @@ abstract contract ERC721Permit is ERC721, IERC721Permit, EIP712, UnorderedNonce
EIP712(name_, version_)
{}

/// @inheritdoc IERC721Permit
function DOMAIN_SEPARATOR() external view returns (bytes32) {
return _domainSeparatorV4();
}

/// @inheritdoc IERC721Permit
function permit(address spender, uint256 tokenId, uint256 deadline, uint256 nonce, bytes calldata signature)
external
Expand All @@ -37,7 +32,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);
Expand Down
6 changes: 6 additions & 0 deletions src/interfaces/IEIP712.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

interface IEIP712 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think IPositionManager should inherit this?

function DOMAIN_SEPARATOR() external view returns (bytes32);
}
4 changes: 0 additions & 4 deletions src/interfaces/IERC721Permit.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion test/ERC721Permit.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ 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)"),
Expand Down
2 changes: 1 addition & 1 deletion test/position-managers/Permit.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ 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)"),
Expand Down
Loading