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

PosM: owner-level nonce for permit #139

Closed
wants to merge 17 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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/autocompound_exactUnclaimedFees.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
291244
293396
Original file line number Diff line number Diff line change
@@ -1 +1 @@
223603
225755
2 changes: 1 addition & 1 deletion .forge-snapshots/autocompound_excessFeesCredit.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
311783
313935
2 changes: 1 addition & 1 deletion .forge-snapshots/decreaseLiquidity_erc20.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
209314
211447
2 changes: 1 addition & 1 deletion .forge-snapshots/decreaseLiquidity_erc6909.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
209326
211459
2 changes: 1 addition & 1 deletion .forge-snapshots/increaseLiquidity_erc20.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
194862
197014
2 changes: 1 addition & 1 deletion .forge-snapshots/increaseLiquidity_erc6909.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
194874
197026
2 changes: 1 addition & 1 deletion .forge-snapshots/mint.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
493163
490008
1 change: 1 addition & 0 deletions .forge-snapshots/mintWithLiquidity.snap
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
490196
1 change: 1 addition & 0 deletions .forge-snapshots/permit.snap
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
75071
1 change: 1 addition & 0 deletions .forge-snapshots/permit_secondPosition.snap
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
57971
1 change: 1 addition & 0 deletions .forge-snapshots/permit_twice.snap
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
40871
17 changes: 11 additions & 6 deletions contracts/NonfungiblePositionManager.sol
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ contract NonfungiblePositionManager is INonfungiblePositionManager, BaseLiquidit

// mint receipt token
_mint(owner, (tokenId = nextTokenId++));
tokenPositions[tokenId] = TokenPosition({owner: owner, range: range});
tokenPositions[tokenId] = TokenPosition({owner: owner, range: range, operator: address(0x0)});
Copy link
Member

Choose a reason for hiding this comment

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

if you mint, maybe you should be allowed to specify a default operator? maybe thats wack though lol

}

function increaseLiquidity(uint256 tokenId, uint256 liquidity, bytes memory hookData, bool claims, address sender)
Expand Down Expand Up @@ -166,19 +166,24 @@ contract NonfungiblePositionManager is INonfungiblePositionManager, BaseLiquidit
TokenPosition storage tokenPosition = tokenPositions[tokenId];
LiquidityRangeId rangeId = tokenPosition.range.toId();
Position storage position = positions[from][rangeId];
position.operator = address(0x0);

// transfer position data to destination
positions[to][rangeId] = position;
delete positions[from][rangeId];

// update token position
tokenPositions[tokenId] = TokenPosition({owner: to, range: tokenPosition.range});
tokenPositions[tokenId] = TokenPosition({owner: to, range: tokenPosition.range, operator: address(0x0)});
}

function _getAndIncrementNonce(uint256 tokenId) internal override returns (uint256) {
TokenPosition memory tokenPosition = tokenPositions[tokenId];
return uint256(positions[tokenPosition.owner][tokenPosition.range.toId()].nonce++);
// override ERC721 approval by setting operator
function _approve(address spender, uint256 tokenId) internal override {
tokenPositions[tokenId].operator = spender;
}

function getApproved(uint256 tokenId) public view override returns (address) {
require(_exists(tokenId), "ERC721: approved query for nonexistent token");

return tokenPositions[tokenId].operator;
}

modifier isAuthorizedForToken(uint256 tokenId, address sender) {
Expand Down
48 changes: 38 additions & 10 deletions contracts/base/ERC721Permit.sol
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +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 {
/// @dev Gets the current nonce for a token ID and then increments it, returning the original value
function _getAndIncrementNonce(uint256 tokenId) internal virtual returns (uint256);
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;
Expand Down Expand Up @@ -46,23 +45,18 @@ 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
{
require(block.timestamp <= deadline, "Permit expired");

bytes32 digest = keccak256(
abi.encodePacked(
"\x19\x01",
DOMAIN_SEPARATOR(),
keccak256(abi.encode(PERMIT_TYPEHASH, spender, tokenId, _getAndIncrementNonce(tokenId), deadline))
)
);
address owner = ownerOf(tokenId);
require(spender != owner, "ERC721Permit: approval to current owner");

bytes32 digest = getDigest(spender, tokenId, nonce, deadline);

if (Address.isContract(owner)) {
require(IERC1271(owner).isValidSignature(digest, abi.encodePacked(r, s, v)) == 0x1626ba7e, "Unauthorized");
} else {
Expand All @@ -71,6 +65,40 @@ abstract contract ERC721Permit is ERC721, IERC721Permit {
require(recoveredAddress == owner, "Unauthorized");
}

_useUnorderedNonce(owner, nonce);
approve(spender, tokenId);
}

function getDigest(address spender, uint256 tokenId, uint256 _nonce, uint256 deadline)
public
view
returns (bytes32 digest)
{
digest = keccak256(
abi.encodePacked(
"\x19\x01",
DOMAIN_SEPARATOR(),
keccak256(abi.encode(PERMIT_TYPEHASH, spender, tokenId, _nonce, deadline))
)
);
}

/// @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();
}
}
4 changes: 0 additions & 4 deletions contracts/interfaces/IBaseLiquidityManagement.sol
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,6 @@ interface IBaseLiquidityManagement {

// details about the liquidity position
struct Position {
// the nonce for permits
uint96 nonce;
// the address that is approved for spending this token
address operator;
uint256 liquidity;
// the fee growth of the aggregate position as of the last action on the individual position
uint256 feeGrowthInside0LastX128;
Expand Down
4 changes: 3 additions & 1 deletion contracts/interfaces/IERC721Permit.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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;
}
1 change: 1 addition & 0 deletions contracts/interfaces/INonfungiblePositionManager.sol
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ interface INonfungiblePositionManager {
struct TokenPosition {
address owner;
LiquidityRange range;
address operator;
}

error MustBeUnlockedByThisContract();
Expand Down
6 changes: 3 additions & 3 deletions test/position-managers/Execute.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ contract ExecuteTest is Test, Deployers, GasSnapshot, LiquidityFuzzers, Liquidit

_increaseLiquidity(tokenId, liquidityToAdd, ZERO_BYTES, false);

(,, uint256 liquidity,,,,) = lpm.positions(address(this), range.toId());
(uint256 liquidity,,,,) = lpm.positions(address(this), range.toId());
assertEq(liquidity, initialLiquidity + liquidityToAdd);
}

Expand Down Expand Up @@ -117,7 +117,7 @@ contract ExecuteTest is Test, Deployers, GasSnapshot, LiquidityFuzzers, Liquidit
currencies[1] = currency1;
lpm.modifyLiquidities(abi.encode(planner.actions, planner.params, currencies));

(,, uint256 liquidity,,,,) = lpm.positions(address(this), range.toId());
(uint256 liquidity,,,,) = lpm.positions(address(this), range.toId());
assertEq(liquidity, initialiLiquidity + liquidityToAdd + liquidityToAdd2);
}

Expand All @@ -140,7 +140,7 @@ contract ExecuteTest is Test, Deployers, GasSnapshot, LiquidityFuzzers, Liquidit
currencies[1] = currency1;
lpm.modifyLiquidities(abi.encode(planner.actions, planner.params, currencies));

(,, uint256 liquidity,,,,) = lpm.positions(address(this), range.toId());
(uint256 liquidity,,,,) = lpm.positions(address(this), range.toId());
assertEq(liquidity, initialLiquidity + liquidityToAdd);
}

Expand Down
4 changes: 2 additions & 2 deletions test/position-managers/FeeCollection.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -172,8 +172,8 @@ contract FeeCollectionTest is Test, Deployers, GasSnapshot, LiquidityFuzzers, Li
uint256 tokenIdBob = lpm.nextTokenId() - 1;

// confirm the positions are same range
(, LiquidityRange memory rangeAlice) = lpm.tokenPositions(tokenIdAlice);
(, LiquidityRange memory rangeBob) = lpm.tokenPositions(tokenIdBob);
(, LiquidityRange memory rangeAlice,) = lpm.tokenPositions(tokenIdAlice);
(, LiquidityRange memory rangeBob,) = lpm.tokenPositions(tokenIdBob);
assertEq(rangeAlice.tickLower, rangeBob.tickLower);
assertEq(rangeAlice.tickUpper, rangeBob.tickUpper);

Expand Down
84 changes: 82 additions & 2 deletions test/position-managers/Gas.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,10 @@ contract GasTest is Test, Deployers, GasSnapshot, LiquidityOperations {
using Planner for Planner.Plan;

PoolId poolId;
address alice = makeAddr("ALICE");
address bob = makeAddr("BOB");
address alice;
uint256 alicePK;
address bob;
uint256 bobPK;

uint256 constant STARTING_USER_BALANCE = 10_000_000 ether;

Expand All @@ -46,6 +48,9 @@ contract GasTest is Test, Deployers, GasSnapshot, LiquidityOperations {
LiquidityRange range;

function setUp() public {
(alice, alicePK) = makeAddrAndKey("ALICE");
(bob, bobPK) = makeAddrAndKey("BOB");

Deployers.deployFreshManagerAndRouters();
Deployers.deployMintAndApprove2Currencies();

Expand Down Expand Up @@ -298,4 +303,79 @@ contract GasTest is Test, Deployers, GasSnapshot, LiquidityOperations {
function test_gas_burn() public {}
function test_gas_burnEmpty() public {}
function test_gas_collect() public {}

function test_gas_permit() public {
// alice permits for the first time
uint256 liquidityAlice = 1e18;
vm.prank(alice);
_mint(range, liquidityAlice, block.timestamp + 1, alice, ZERO_BYTES);
uint256 tokenIdAlice = lpm.nextTokenId() - 1;

// alice gives operator permission to bob
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, nonce, v, r, s);
snapLastCall("permit");
}

function test_gas_permit_secondPosition() public {
// alice permits for her two tokens, benchmark the 2nd permit
uint256 liquidityAlice = 1e18;
vm.prank(alice);
_mint(range, liquidityAlice, block.timestamp + 1, alice, ZERO_BYTES);
uint256 tokenIdAlice = lpm.nextTokenId() - 1;

// alice gives operator permission to bob
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, nonce, v, r, s);

// alice creates another position
vm.prank(alice);
_mint(range, liquidityAlice, block.timestamp + 1, alice, ZERO_BYTES);
tokenIdAlice = lpm.nextTokenId() - 1;

// alice gives operator permission to bob
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, nonce, v, r, s);
snapLastCall("permit_secondPosition");
}

function test_gas_permit_twice() public {
// alice permits the same token, twice
address charlie = makeAddr("CHARLIE");

uint256 liquidityAlice = 1e18;
vm.prank(alice);
_mint(range, liquidityAlice, block.timestamp + 1, alice, ZERO_BYTES);
uint256 tokenIdAlice = lpm.nextTokenId() - 1;

// alice gives operator permission to bob
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, nonce, v, r, s);

// alice gives operator permission to charlie
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, nonce, v, r, s);
snapLastCall("permit_twice");
}
}
Loading
Loading