diff --git a/.forge-snapshots/autocompound_exactUnclaimedFees.snap b/.forge-snapshots/autocompound_exactUnclaimedFees.snap index b1bc5219..c71a0ae9 100644 --- a/.forge-snapshots/autocompound_exactUnclaimedFees.snap +++ b/.forge-snapshots/autocompound_exactUnclaimedFees.snap @@ -1 +1 @@ -295470 \ No newline at end of file +295448 \ No newline at end of file diff --git a/.forge-snapshots/autocompound_exactUnclaimedFees_exactCustodiedFees.snap b/.forge-snapshots/autocompound_exactUnclaimedFees_exactCustodiedFees.snap index b0cc5d95..ddf3aa2d 100644 --- a/.forge-snapshots/autocompound_exactUnclaimedFees_exactCustodiedFees.snap +++ b/.forge-snapshots/autocompound_exactUnclaimedFees_exactCustodiedFees.snap @@ -1 +1 @@ -227829 \ No newline at end of file +227807 \ No newline at end of file diff --git a/.forge-snapshots/autocompound_excessFeesCredit.snap b/.forge-snapshots/autocompound_excessFeesCredit.snap index 455648f4..058e94fb 100644 --- a/.forge-snapshots/autocompound_excessFeesCredit.snap +++ b/.forge-snapshots/autocompound_excessFeesCredit.snap @@ -1 +1 @@ -316009 \ No newline at end of file +315987 \ No newline at end of file diff --git a/.forge-snapshots/increaseLiquidity_erc20.snap b/.forge-snapshots/increaseLiquidity_erc20.snap index 7ed8a8ea..afcbd9a4 100644 --- a/.forge-snapshots/increaseLiquidity_erc20.snap +++ b/.forge-snapshots/increaseLiquidity_erc20.snap @@ -1 +1 @@ -199086 \ No newline at end of file +199064 \ No newline at end of file diff --git a/.forge-snapshots/increaseLiquidity_erc6909.snap b/.forge-snapshots/increaseLiquidity_erc6909.snap index 9f04cc1a..5009a3e7 100644 --- a/.forge-snapshots/increaseLiquidity_erc6909.snap +++ b/.forge-snapshots/increaseLiquidity_erc6909.snap @@ -1 +1 @@ -199098 \ No newline at end of file +199076 \ No newline at end of file diff --git a/.forge-snapshots/permit.snap b/.forge-snapshots/permit.snap index 9927819f..269d80a6 100644 --- a/.forge-snapshots/permit.snap +++ b/.forge-snapshots/permit.snap @@ -1 +1 @@ -74708 \ No newline at end of file +75071 \ No newline at end of file diff --git a/.forge-snapshots/permit_secondPosition.snap b/.forge-snapshots/permit_secondPosition.snap index 9281d22b..d863e28d 100644 --- a/.forge-snapshots/permit_secondPosition.snap +++ b/.forge-snapshots/permit_secondPosition.snap @@ -1 +1 @@ -57608 \ No newline at end of file +57959 \ No newline at end of file diff --git a/.forge-snapshots/permit_twice.snap b/.forge-snapshots/permit_twice.snap index 6e951055..6ef9d761 100644 --- a/.forge-snapshots/permit_twice.snap +++ b/.forge-snapshots/permit_twice.snap @@ -1 +1 @@ -40496 \ No newline at end of file +40871 \ No newline at end of file diff --git a/contracts/base/ERC721Permit.sol b/contracts/base/ERC721Permit.sol index 8323c77e..4668f2c5 100644 --- a/contracts/base/ERC721Permit.sol +++ b/contracts/base/ERC721Permit.sol @@ -11,7 +11,7 @@ import {IERC1271} from "../interfaces/external/IERC1271.sol"; /// @title ERC721 with permit /// @notice Nonfungible tokens that support an approve via signature, i.e. permit abstract contract ERC721Permit is ERC721, IERC721Permit { - mapping(address owner => uint256 nonce) public nonce; + mapping(address owner => mapping(uint256 word => uint256 bitmap)) public nonces; /// @dev The hash of the name used in the permit signature verification bytes32 private immutable nameHash; @@ -45,7 +45,7 @@ abstract contract ERC721Permit is ERC721, IERC721Permit { 0x49ecf333e5b8c95c40fdafc95c1ad136e8914a8fb55e9dc8bb01eaa83a2df9ad; /// @inheritdoc IERC721Permit - function permit(address spender, uint256 tokenId, uint256 deadline, uint8 v, bytes32 r, bytes32 s) + function permit(address spender, uint256 tokenId, uint256 deadline, uint256 nonce, uint8 v, bytes32 r, bytes32 s) external payable override @@ -55,7 +55,7 @@ abstract contract ERC721Permit is ERC721, IERC721Permit { address owner = ownerOf(tokenId); require(spender != owner, "ERC721Permit: approval to current owner"); - bytes32 digest = getDigest(spender, tokenId, nonce[owner]++, deadline); + bytes32 digest = getDigest(spender, tokenId, nonce, deadline); if (Address.isContract(owner)) { require(IERC1271(owner).isValidSignature(digest, abi.encodePacked(r, s, v)) == 0x1626ba7e, "Unauthorized"); @@ -65,6 +65,7 @@ abstract contract ERC721Permit is ERC721, IERC721Permit { require(recoveredAddress == owner, "Unauthorized"); } + _useUnorderedNonce(owner, nonce); approve(spender, tokenId); } @@ -81,4 +82,23 @@ abstract contract ERC721Permit is ERC721, IERC721Permit { ) ); } + + /// @notice Returns the index of the bitmap and the bit position within the bitmap. Used for unordered nonces + /// @param nonce The nonce to get the associated word and bit positions + /// @return wordPos The word position or index into the nonceBitmap + /// @return bitPos The bit position + /// @dev The first 248 bits of the nonce value is the index of the desired bitmap + /// @dev The last 8 bits of the nonce value is the position of the bit in the bitmap + function bitmapPositions(uint256 nonce) private pure returns (uint256 wordPos, uint256 bitPos) { + wordPos = uint248(nonce >> 8); + bitPos = uint8(nonce); + } + + function _useUnorderedNonce(address from, uint256 nonce) internal { + (uint256 wordPos, uint256 bitPos) = bitmapPositions(nonce); + uint256 bit = 1 << bitPos; + uint256 flipped = nonces[from][wordPos] ^= bit; + + if (flipped & bit == 0) revert NonceAlreadyUsed(); + } } diff --git a/contracts/interfaces/IERC721Permit.sol b/contracts/interfaces/IERC721Permit.sol index daa27030..213bca2a 100644 --- a/contracts/interfaces/IERC721Permit.sol +++ b/contracts/interfaces/IERC721Permit.sol @@ -4,6 +4,8 @@ pragma solidity >=0.7.5; /// @title ERC721 with permit /// @notice Extension to ERC721 that includes a permit function for signature based approvals interface IERC721Permit { + error NonceAlreadyUsed(); + /// @notice The permit typehash used in the permit signature /// @return The typehash for the permit function PERMIT_TYPEHASH() external pure returns (bytes32); @@ -19,7 +21,7 @@ interface IERC721Permit { /// @param v Must produce valid secp256k1 signature from the holder along with `r` and `s` /// @param r Must produce valid secp256k1 signature from the holder along with `v` and `s` /// @param s Must produce valid secp256k1 signature from the holder along with `r` and `v` - function permit(address spender, uint256 tokenId, uint256 deadline, uint8 v, bytes32 r, bytes32 s) + function permit(address spender, uint256 tokenId, uint256 deadline, uint256 nonce, uint8 v, bytes32 r, bytes32 s) external payable; } diff --git a/test/position-managers/Gas.t.sol b/test/position-managers/Gas.t.sol index 928ad3a2..23b81f39 100644 --- a/test/position-managers/Gas.t.sol +++ b/test/position-managers/Gas.t.sol @@ -324,12 +324,13 @@ contract GasTest is Test, Deployers, GasSnapshot, LiquidityOperations { uint256 tokenIdAlice = lpm.nextTokenId() - 1; // alice gives operator permission to bob - bytes32 digest = lpm.getDigest(bob, tokenIdAlice, lpm.nonce(alice), block.timestamp + 1); + uint256 nonce = 1; + bytes32 digest = lpm.getDigest(bob, tokenIdAlice, nonce, block.timestamp + 1); (uint8 v, bytes32 r, bytes32 s) = vm.sign(alicePK, digest); vm.prank(alice); - lpm.permit(bob, tokenIdAlice, block.timestamp + 1, v, r, s); + lpm.permit(bob, tokenIdAlice, block.timestamp + 1, nonce, v, r, s); snapLastCall("permit"); } @@ -341,11 +342,12 @@ contract GasTest is Test, Deployers, GasSnapshot, LiquidityOperations { uint256 tokenIdAlice = lpm.nextTokenId() - 1; // alice gives operator permission to bob - bytes32 digest = lpm.getDigest(bob, tokenIdAlice, lpm.nonce(alice), block.timestamp + 1); + uint256 nonce = 1; + bytes32 digest = lpm.getDigest(bob, tokenIdAlice, nonce, block.timestamp + 1); (uint8 v, bytes32 r, bytes32 s) = vm.sign(alicePK, digest); vm.prank(alice); - lpm.permit(bob, tokenIdAlice, block.timestamp + 1, v, r, s); + lpm.permit(bob, tokenIdAlice, block.timestamp + 1, nonce, v, r, s); // alice creates another position vm.prank(alice); @@ -353,11 +355,12 @@ contract GasTest is Test, Deployers, GasSnapshot, LiquidityOperations { tokenIdAlice = lpm.nextTokenId() - 1; // alice gives operator permission to bob - digest = lpm.getDigest(bob, tokenIdAlice, lpm.nonce(alice), block.timestamp + 1); + nonce = 2; + digest = lpm.getDigest(bob, tokenIdAlice, nonce, block.timestamp + 1); (v, r, s) = vm.sign(alicePK, digest); vm.prank(alice); - lpm.permit(bob, tokenIdAlice, block.timestamp + 1, v, r, s); + lpm.permit(bob, tokenIdAlice, block.timestamp + 1, nonce, v, r, s); snapLastCall("permit_secondPosition"); } @@ -371,18 +374,20 @@ contract GasTest is Test, Deployers, GasSnapshot, LiquidityOperations { uint256 tokenIdAlice = lpm.nextTokenId() - 1; // alice gives operator permission to bob - bytes32 digest = lpm.getDigest(bob, tokenIdAlice, lpm.nonce(alice), block.timestamp + 1); + uint256 nonce = 1; + bytes32 digest = lpm.getDigest(bob, tokenIdAlice, nonce, block.timestamp + 1); (uint8 v, bytes32 r, bytes32 s) = vm.sign(alicePK, digest); vm.prank(alice); - lpm.permit(bob, tokenIdAlice, block.timestamp + 1, v, r, s); + lpm.permit(bob, tokenIdAlice, block.timestamp + 1, nonce, v, r, s); // alice gives operator permission to charlie - digest = lpm.getDigest(charlie, tokenIdAlice, lpm.nonce(alice), block.timestamp + 1); + nonce = 2; + digest = lpm.getDigest(charlie, tokenIdAlice, nonce, block.timestamp + 1); (v, r, s) = vm.sign(alicePK, digest); vm.prank(alice); - lpm.permit(charlie, tokenIdAlice, block.timestamp + 1, v, r, s); + lpm.permit(charlie, tokenIdAlice, block.timestamp + 1, nonce, v, r, s); snapLastCall("permit_twice"); } } diff --git a/test/position-managers/Permit.t.sol b/test/position-managers/Permit.t.sol index e7939896..1075ebcb 100644 --- a/test/position-managers/Permit.t.sol +++ b/test/position-managers/Permit.t.sol @@ -19,6 +19,7 @@ import {StateLibrary} from "@uniswap/v4-core/src/libraries/StateLibrary.sol"; import {IERC20} from "forge-std/interfaces/IERC20.sol"; import {ERC721} from "@openzeppelin/contracts/token/ERC721/ERC721.sol"; +import {IERC721Permit} from "../../contracts/interfaces/IERC721Permit.sol"; import {NonfungiblePositionManager} from "../../contracts/NonfungiblePositionManager.sol"; import {LiquidityRange, LiquidityRangeId, LiquidityRangeIdLibrary} from "../../contracts/types/LiquidityRange.sol"; @@ -85,7 +86,7 @@ contract PermitTest is Test, Deployers, GasSnapshot, Fuzzers, LiquidityOperation uint256 tokenIdAlice = lpm.nextTokenId() - 1; // alice gives bob operator permissions - _permit(alice, alicePK, tokenIdAlice, bob); + _permit(alice, alicePK, tokenIdAlice, bob, 1); // bob can increase liquidity on alice's token uint256 newLiquidity = 2e18; @@ -110,7 +111,7 @@ contract PermitTest is Test, Deployers, GasSnapshot, Fuzzers, LiquidityOperation uint256 tokenIdAlice = lpm.nextTokenId() - 1; // alice gives bob operator permissions - _permit(alice, alicePK, tokenIdAlice, bob); + _permit(alice, alicePK, tokenIdAlice, bob, 1); // bob can decrease liquidity on alice's token uint256 liquidityToRemove = 0.4444e18; @@ -134,7 +135,7 @@ contract PermitTest is Test, Deployers, GasSnapshot, Fuzzers, LiquidityOperation donateRouter.donate(key, currency0Revenue, currency1Revenue, ZERO_BYTES); // alice gives bob operator permissions - _permit(alice, alicePK, tokenIdAlice, bob); + _permit(alice, alicePK, tokenIdAlice, bob, 1); // TODO: enable once we fix recipient collection @@ -158,13 +159,13 @@ contract PermitTest is Test, Deployers, GasSnapshot, Fuzzers, LiquidityOperation uint256 tokenIdAlice = lpm.nextTokenId() - 1; // bob cannot permit himself on alice's token - bytes32 digest = lpm.getDigest(bob, tokenIdAlice, lpm.nonce(bob), block.timestamp + 1); + bytes32 digest = lpm.getDigest(bob, tokenIdAlice, 0, block.timestamp + 1); (uint8 v, bytes32 r, bytes32 s) = vm.sign(bobPK, digest); vm.startPrank(bob); vm.expectRevert("Unauthorized"); - lpm.permit(bob, tokenIdAlice, block.timestamp + 1, v, r, s); + lpm.permit(bob, tokenIdAlice, block.timestamp + 1, 0, v, r, s); vm.stopPrank(); } @@ -219,4 +220,52 @@ contract PermitTest is Test, Deployers, GasSnapshot, Fuzzers, LiquidityOperation _collect(range, tokenIdAlice, recipient, ZERO_BYTES, false); vm.stopPrank(); } + + function test_permit_nonceAlreadyUsed() public { + uint256 liquidityAlice = 1e18; + vm.prank(alice); + _mint(range, liquidityAlice, block.timestamp + 1, alice, ZERO_BYTES); + uint256 tokenIdAlice = lpm.nextTokenId() - 1; + + // alice gives bob operator permissions + uint256 nonce = 1; + _permit(alice, alicePK, tokenIdAlice, bob, nonce); + + // alice cannot reuse the nonce + bytes32 digest = lpm.getDigest(bob, tokenIdAlice, nonce, block.timestamp + 1); + + (uint8 v, bytes32 r, bytes32 s) = vm.sign(alicePK, digest); + + vm.startPrank(alice); + vm.expectRevert(IERC721Permit.NonceAlreadyUsed.selector); + lpm.permit(bob, tokenIdAlice, block.timestamp + 1, nonce, v, r, s); + vm.stopPrank(); + } + + function test_permit_nonceAlreadyUsed_twoPositions() public { + uint256 liquidityAlice = 1e18; + vm.prank(alice); + _mint(range, liquidityAlice, block.timestamp + 1, alice, ZERO_BYTES); + uint256 tokenIdAlice = lpm.nextTokenId() - 1; + + vm.prank(alice); + range.tickLower = -600; + range.tickUpper = 600; + _mint(range, liquidityAlice, block.timestamp + 1, alice, ZERO_BYTES); + uint256 tokenIdAlice2 = lpm.nextTokenId() - 1; + + // alice gives bob operator permissions for first token + uint256 nonce = 1; + _permit(alice, alicePK, tokenIdAlice, bob, nonce); + + // alice cannot reuse the nonce for the second token + bytes32 digest = lpm.getDigest(bob, tokenIdAlice2, nonce, block.timestamp + 1); + + (uint8 v, bytes32 r, bytes32 s) = vm.sign(alicePK, digest); + + vm.startPrank(alice); + vm.expectRevert(IERC721Permit.NonceAlreadyUsed.selector); + lpm.permit(bob, tokenIdAlice2, block.timestamp + 1, nonce, v, r, s); + vm.stopPrank(); + } } diff --git a/test/shared/LiquidityOperations.sol b/test/shared/LiquidityOperations.sol index 5b01b1f7..def18fde 100644 --- a/test/shared/LiquidityOperations.sol +++ b/test/shared/LiquidityOperations.sol @@ -108,12 +108,12 @@ contract LiquidityOperations { } // TODO: organize somewhere else, or rename this file to NFTLiquidityHelpers? - function _permit(address signer, uint256 privateKey, uint256 tokenId, address operator) internal { - bytes32 digest = lpm.getDigest(operator, tokenId, lpm.nonce(signer), block.timestamp + 1); + function _permit(address signer, uint256 privateKey, uint256 tokenId, address operator, uint256 nonce) internal { + bytes32 digest = lpm.getDigest(operator, tokenId, 1, block.timestamp + 1); (uint8 v, bytes32 r, bytes32 s) = _vm1.sign(privateKey, digest); _vm1.prank(signer); - lpm.permit(operator, tokenId, block.timestamp + 1, v, r, s); + lpm.permit(operator, tokenId, block.timestamp + 1, nonce, v, r, s); } }