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.

Sign up for GitHub

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
Merged

Replace OZ EIP712 #256

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
b59b1b5
WOOF WOOF WOOF BARK BARK WOOF BARK
saucepoint Aug 3, 2024
4cd765a
remove address(this) check
saucepoint Aug 3, 2024
cd8b760
forge fmt
saucepoint Aug 3, 2024
9419e44
discard version from EIP712
saucepoint Aug 3, 2024
27d394e
Update src/base/EIP712.sol
saucepoint Aug 3, 2024
2c61e65
Merge branch 'main' into eip712
saucepoint Aug 3, 2024
e553340
natspec for delegatecall error
saucepoint Aug 3, 2024
4a74d8d
assembly optimization, pr nits
saucepoint Aug 4, 2024
0826c12
test to validate assembly
saucepoint Aug 4, 2024
92d6e03
merge in main; regenerate gas
saucepoint Aug 4, 2024
d99deef
merge in main; regenerate gas
saucepoint Aug 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .forge-snapshots/PositionManager_multicall_initialize_mint.snap
View file
Open in desktop
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
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
79585
79636
2 changes: 1 addition & 1 deletion .forge-snapshots/PositionManager_permit_secondPosition.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
62497
62524
2 changes: 1 addition & 1 deletion .forge-snapshots/PositionManager_permit_twice.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
45397
45412
1 change: 0 additions & 1 deletion remappings.txt
View file
Open in desktop
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
View file
Open in desktop
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
43 changes: 43 additions & 0 deletions src/base/EIP712.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
// 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;

bytes32 private constant _TYPE_HASH =
keccak256("EIP712Domain(string name,uint256 chainId,address verifyingContract)");
Copy link
Contributor

Choose a reason for hiding this comment

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

why dont we want a version string? i thought part of the idea was that we might have multiple versions? @snreynolds

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it shaves a bit of gas and sara thinks we can version in name

Copy link
Contributor

Choose a reason for hiding this comment

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

it shouldnt shave anything off gas except in the constructor? after that its not calculating the typehash right?

Copy link
Contributor

Choose a reason for hiding this comment

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

oh you should hardcode this hash - solidity calculates constants at runtime not compile time so itll be recalculating every time


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) {
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));
}
}
14 changes: 3 additions & 11 deletions src/base/ERC721Permit.sol
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
8 changes: 3 additions & 5 deletions test/ERC721Permit.t.sol
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
Loading