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
79566
Original file line number Diff line number Diff line change
@@ -1 +1 @@
62497
62454
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
45342
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
2 changes: 1 addition & 1 deletion src/PositionManager.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
51 changes: 51 additions & 0 deletions src/base/EIP712.sol
Original file line number Diff line number Diff line change
@@ -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)));
hensha256 marked this conversation as resolved.
Show resolved Hide resolved
}

/// @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)
}
}
}
14 changes: 3 additions & 11 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 @@ -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)
Expand All @@ -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);
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
47 changes: 47 additions & 0 deletions test/EIP712.t.sol
Original file line number Diff line number Diff line change
@@ -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)
)
)
);
}
}
8 changes: 3 additions & 5 deletions test/ERC721Permit.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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 ---
Expand Down Expand Up @@ -69,12 +68,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)
)
Expand Down
2 changes: 1 addition & 1 deletion test/mocks/MockERC721Permit.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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 "";
Expand Down
5 changes: 2 additions & 3 deletions test/position-managers/Permit.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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)
)
Expand Down