From 3a254d065be2b4233f2073f9f48094cc3f71a27c Mon Sep 17 00:00:00 2001 From: saucepoint Date: Sun, 4 Aug 2024 23:10:36 -0400 Subject: [PATCH] pr feedback --- .forge-snapshots/PositionManager_permit.snap | 2 +- ...PositionManager_permit_secondPosition.snap | 2 +- .../PositionManager_permit_twice.snap | 2 +- src/base/ERC721Permit_v4.sol | 24 ++++++---- src/interfaces/IERC721Permit_v4.sol | 2 +- test/erc721Permit/ERC721Permit.permit.t.sol | 4 +- .../ERC721Permit.permitForAll.t.sol | 46 +++++++++---------- 7 files changed, 42 insertions(+), 40 deletions(-) diff --git a/.forge-snapshots/PositionManager_permit.snap b/.forge-snapshots/PositionManager_permit.snap index ee34c59c..81715a67 100644 --- a/.forge-snapshots/PositionManager_permit.snap +++ b/.forge-snapshots/PositionManager_permit.snap @@ -1 +1 @@ -79445 \ No newline at end of file +79484 \ No newline at end of file diff --git a/.forge-snapshots/PositionManager_permit_secondPosition.snap b/.forge-snapshots/PositionManager_permit_secondPosition.snap index 0b63319b..bd214aa7 100644 --- a/.forge-snapshots/PositionManager_permit_secondPosition.snap +++ b/.forge-snapshots/PositionManager_permit_secondPosition.snap @@ -1 +1 @@ -62333 \ No newline at end of file +62372 \ No newline at end of file diff --git a/.forge-snapshots/PositionManager_permit_twice.snap b/.forge-snapshots/PositionManager_permit_twice.snap index 7772c13d..c0e053f6 100644 --- a/.forge-snapshots/PositionManager_permit_twice.snap +++ b/.forge-snapshots/PositionManager_permit_twice.snap @@ -1 +1 @@ -45221 \ No newline at end of file +45260 \ No newline at end of file diff --git a/src/base/ERC721Permit_v4.sol b/src/base/ERC721Permit_v4.sol index 4bdef2ac..6c67e7f8 100644 --- a/src/base/ERC721Permit_v4.sol +++ b/src/base/ERC721Permit_v4.sol @@ -17,18 +17,22 @@ abstract contract ERC721Permit_v4 is ERC721, IERC721Permit_v4, EIP712_v4, Unorde /// @notice Computes the nameHash and versionHash constructor(string memory name_, string memory symbol_) ERC721(name_, symbol_) EIP712_v4(name_) {} + modifier checkSignatureDeadline(uint256 deadline) { + if (block.timestamp > deadline) revert SignatureDeadlineExpired(); + _; + } + /// @inheritdoc IERC721Permit_v4 function permit(address spender, uint256 tokenId, uint256 deadline, uint256 nonce, bytes calldata signature) external payable + checkSignatureDeadline(deadline) { - if (block.timestamp > deadline) revert DeadlineExpired(); - address owner = ownerOf(tokenId); - if (spender == owner) revert NoSelfPermit(); + _checkNoSelfPermit(owner, spender); - bytes32 hash = ERC721PermitHashLibrary.hashPermit(spender, tokenId, nonce, deadline); - signature.verify(_hashTypedData(hash), owner); + bytes32 digest = ERC721PermitHashLibrary.hashPermit(spender, tokenId, nonce, deadline); + signature.verify(_hashTypedData(digest), owner); _useUnorderedNonce(owner, nonce); _approve(owner, spender, tokenId); @@ -42,10 +46,8 @@ abstract contract ERC721Permit_v4 is ERC721, IERC721Permit_v4, EIP712_v4, Unorde uint256 deadline, uint256 nonce, bytes calldata signature - ) external payable { - if (block.timestamp > deadline) revert DeadlineExpired(); - - if (operator == owner) revert NoSelfPermit(); + ) external payable checkSignatureDeadline(deadline) { + _checkNoSelfPermit(owner, operator); bytes32 hash = ERC721PermitHashLibrary.hashPermitForAll(operator, approved, nonce, deadline); signature.verify(_hashTypedData(hash), owner); @@ -94,4 +96,8 @@ abstract contract ERC721Permit_v4 is ERC721, IERC721Permit_v4, EIP712_v4, Unorde return spender == ownerOf(tokenId) || getApproved[tokenId] == spender || isApprovedForAll[ownerOf(tokenId)][spender]; } + + function _checkNoSelfPermit(address owner, address permitted) internal pure { + if (owner == permitted) revert NoSelfPermit(); + } } diff --git a/src/interfaces/IERC721Permit_v4.sol b/src/interfaces/IERC721Permit_v4.sol index 14557561..02b3eb97 100644 --- a/src/interfaces/IERC721Permit_v4.sol +++ b/src/interfaces/IERC721Permit_v4.sol @@ -4,7 +4,7 @@ pragma solidity >=0.7.5; /// @title ERC721 with permit /// @notice Extension to ERC721 that includes a permit function for signature based approvals interface IERC721Permit_v4 { - error DeadlineExpired(); + error SignatureDeadlineExpired(); error NoSelfPermit(); error Unauthorized(); diff --git a/test/erc721Permit/ERC721Permit.permit.t.sol b/test/erc721Permit/ERC721Permit.permit.t.sol index e9f662fd..654c20c9 100644 --- a/test/erc721Permit/ERC721Permit.permit.t.sol +++ b/test/erc721Permit/ERC721Permit.permit.t.sol @@ -229,7 +229,7 @@ contract ERC721PermitTest is Test { assertEq(erc721Permit.nonces(alice, wordPos) & (1 << bitPos), 0); } - function test_fuzz_erc721Permit_deadlineExpired(address spender) public { + function test_fuzz_erc721Permit_SignatureDeadlineExpired(address spender) public { vm.prank(alice); uint256 tokenId = erc721Permit.mint(); @@ -252,7 +252,7 @@ contract ERC721PermitTest is Test { // -- Permit but deadline expired -- // vm.startPrank(spender); - vm.expectRevert(IERC721Permit_v4.DeadlineExpired.selector); + vm.expectRevert(IERC721Permit_v4.SignatureDeadlineExpired.selector); erc721Permit.permit(spender, tokenId, deadline, nonce, signature); vm.stopPrank(); diff --git a/test/erc721Permit/ERC721Permit.permitForAll.t.sol b/test/erc721Permit/ERC721Permit.permitForAll.t.sol index aef23419..6d5f4be3 100644 --- a/test/erc721Permit/ERC721Permit.permitForAll.t.sol +++ b/test/erc721Permit/ERC721Permit.permitForAll.t.sol @@ -130,9 +130,8 @@ contract ERC721PermitForAllTest is Test { assertEq(erc721Permit.nonces(alice, wordPos) & (1 << bitPos), 2); // 2 = 0010 } - function test_fuzz_erc721permitForAll_nonceAlreadyUsed() public { + function test_fuzz_erc721permitForAll_nonceAlreadyUsed(uint256 nonce) public { // alice gives bob operator permissions - uint256 nonce = 1; _permitForAll(alicePK, alice, bob, true, nonce); // alice cannot reuse the nonce @@ -147,11 +146,7 @@ contract ERC721PermitForAllTest is Test { vm.stopPrank(); } - function test_fuzz_erc721permitForAll_unauthorized() public { - vm.prank(alice); - uint256 tokenId = erc721Permit.mint(); - - uint256 nonce = 1; + function test_fuzz_erc721permitForAll_invalidSigner(uint256 nonce) public { bytes32 digest = _getPermitForAllDigest(bob, true, nonce, block.timestamp); // bob attempts signing an approval for himself @@ -177,7 +172,7 @@ contract ERC721PermitForAllTest is Test { assertEq(erc721Permit.nonces(alice, wordPos) & (1 << bitPos), 0); } - function test_fuzz_erc721permitForAll_deadlineExpired(address operator) public { + function test_fuzz_erc721permitForAll_SignatureDeadlineExpired(address operator) public { uint256 nonce = 1; uint256 deadline = block.timestamp; bytes32 digest = _getPermitForAllDigest(operator, true, nonce, deadline); @@ -196,7 +191,7 @@ contract ERC721PermitForAllTest is Test { // -- PermitForAll but deadline expired -- // vm.startPrank(operator); - vm.expectRevert(IERC721Permit_v4.DeadlineExpired.selector); + vm.expectRevert(IERC721Permit_v4.SignatureDeadlineExpired.selector); erc721Permit.permitForAll(alice, operator, true, deadline, nonce, signature); vm.stopPrank(); @@ -214,13 +209,7 @@ contract ERC721PermitForAllTest is Test { uint256 nonce = 1; uint256 deadline = block.timestamp; - bytes32 digest = keccak256( - abi.encodePacked( - "\x19\x01", - erc721Permit.DOMAIN_SEPARATOR(), - keccak256(abi.encode(ERC721PermitHashLibrary.PERMIT_TYPEHASH, operator, tokenId, nonce, deadline)) - ) - ); + bytes32 digest = _getPermitDigest(operator, tokenId, nonce, deadline); // alice signs a permit for operator (uint8 v, bytes32 r, bytes32 s) = vm.sign(alicePK, digest); @@ -280,19 +269,12 @@ contract ERC721PermitForAllTest is Test { } /// @dev a nonce used in permit is unusable for permitForAll - function test_erc721PermitForAll_permitNonceUsed() public { + function test_fuzz_erc721PermitForAll_permitNonceUsed(uint256 nonce) public { vm.prank(alice); uint256 tokenId = erc721Permit.mint(); - uint256 nonce = 1; uint256 deadline = block.timestamp; - bytes32 digest = keccak256( - abi.encodePacked( - "\x19\x01", - erc721Permit.DOMAIN_SEPARATOR(), - keccak256(abi.encode(ERC721PermitHashLibrary.PERMIT_TYPEHASH, bob, tokenId, nonce, deadline)) - ) - ); + bytes32 digest = _getPermitDigest(bob, tokenId, nonce, deadline); // alice signs a permit for bob (uint8 v, bytes32 r, bytes32 s) = vm.sign(alicePK, digest); bytes memory signature = abi.encodePacked(r, s, v); @@ -344,6 +326,20 @@ contract ERC721PermitForAllTest is Test { ); } + function _getPermitDigest(address spender, uint256 tokenId, uint256 nonce, uint256 deadline) + internal + view + returns (bytes32 digest) + { + digest = keccak256( + abi.encodePacked( + "\x19\x01", + erc721Permit.DOMAIN_SEPARATOR(), + keccak256(abi.encode(ERC721PermitHashLibrary.PERMIT_TYPEHASH, spender, tokenId, nonce, deadline)) + ) + ); + } + // copied the private function from UnorderedNonce.sol function _getBitmapFromNonce(uint256 nonce) private pure returns (uint256 wordPos, uint256 bitPos) { wordPos = uint248(nonce >> 8);