remove isApprovedOrOwner from increase (#207)

* remove isApprovedOrOwner from increase * separate test * simplify test * extra comment
Uniswap · Jul 26, 2024 · b973438 · b973438
1 parent 2a9026f
commit b973438
Show file tree

Hide file tree

Showing 18 changed files with 47 additions and 24 deletions.
diff --git a/.forge-snapshots/PositionManager_burn_empty.snap b/.forge-snapshots/PositionManager_burn_empty.snap
@@ -1 +1 @@
-50237
+50222
diff --git a/.forge-snapshots/PositionManager_burn_empty_native.snap b/.forge-snapshots/PositionManager_burn_empty_native.snap
@@ -1 +1 @@
-50055
+50040
diff --git a/.forge-snapshots/PositionManager_burn_nonEmpty.snap b/.forge-snapshots/PositionManager_burn_nonEmpty.snap
@@ -1 +1 @@
-135881
+135866
diff --git a/.forge-snapshots/PositionManager_burn_nonEmpty_native.snap b/.forge-snapshots/PositionManager_burn_nonEmpty_native.snap
@@ -1 +1 @@
-128803
+128788
diff --git a/.forge-snapshots/PositionManager_collect.snap b/.forge-snapshots/PositionManager_collect.snap
@@ -1 +1 @@
-158176
+158157
diff --git a/.forge-snapshots/PositionManager_collect_native.snap b/.forge-snapshots/PositionManager_collect_native.snap
@@ -1 +1 @@
-149328
+149309
diff --git a/.forge-snapshots/PositionManager_collect_sameRange.snap b/.forge-snapshots/PositionManager_collect_sameRange.snap
@@ -1 +1 @@
-158176
+158157
diff --git a/.forge-snapshots/PositionManager_decreaseLiquidity.snap b/.forge-snapshots/PositionManager_decreaseLiquidity.snap
@@ -1 +1 @@
-123719
+123700
diff --git a/.forge-snapshots/PositionManager_decreaseLiquidity_native.snap b/.forge-snapshots/PositionManager_decreaseLiquidity_native.snap
@@ -1 +1 @@
-114937
+114922
diff --git a/.forge-snapshots/PositionManager_decrease_burnEmpty.snap b/.forge-snapshots/PositionManager_decrease_burnEmpty.snap
@@ -1 +1 @@
-142224
+142193
diff --git a/.forge-snapshots/PositionManager_decrease_burnEmpty_native.snap b/.forge-snapshots/PositionManager_decrease_burnEmpty_native.snap
@@ -1 +1 @@
-134963
+134932
diff --git a/.forge-snapshots/PositionManager_decrease_sameRange_allLiquidity.snap b/.forge-snapshots/PositionManager_decrease_sameRange_allLiquidity.snap
@@ -1 +1 @@
-136435
+136416
diff --git a/.forge-snapshots/PositionManager_increaseLiquidity_erc20.snap b/.forge-snapshots/PositionManager_increaseLiquidity_erc20.snap
@@ -1 +1 @@
-152896
+150444
diff --git a/.forge-snapshots/PositionManager_increaseLiquidity_native.snap b/.forge-snapshots/PositionManager_increaseLiquidity_native.snap
@@ -1 +1 @@
-138106
+135654
diff --git a/.forge-snapshots/PositionManager_increase_autocompoundExactUnclaimedFees.snap b/.forge-snapshots/PositionManager_increase_autocompoundExactUnclaimedFees.snap
@@ -1 +1 @@
-144757
+142305
diff --git a/.forge-snapshots/PositionManager_increase_autocompoundExcessFeesCredit.snap b/.forge-snapshots/PositionManager_increase_autocompoundExcessFeesCredit.snap
@@ -1 +1 @@
-180911
+178459
diff --git a/src/PositionManager.sol b/src/PositionManager.sol
@@ -105,7 +105,7 @@ contract PositionManager is
         (uint256 tokenId, PositionConfig memory config, uint256 liquidity, bytes memory hookData) =
             abi.decode(params, (uint256, PositionConfig, uint256, bytes));
 
-        _validateModify(config, tokenId);
+        if (positionConfigs[tokenId] != config.toId()) revert IncorrectPositionConfigForTokenId(tokenId);
         // Note: The tokenId is used as the salt for this position, so every minted position has unique storage in the pool manager.
         BalanceDelta delta = _modifyLiquidity(config, liquidity.toInt256(), bytes32(tokenId), hookData);
         return abi.encode(delta);
@@ -118,7 +118,8 @@ contract PositionManager is
         (uint256 tokenId, PositionConfig memory config, uint256 liquidity, bytes memory hookData) =
             abi.decode(params, (uint256, PositionConfig, uint256, bytes));
 
-        _validateModify(config, tokenId);
+        if (!_isApprovedOrOwner(_getLocker(), tokenId)) revert NotApproved(_getLocker());
+        if (positionConfigs[tokenId] != config.toId()) revert IncorrectPositionConfigForTokenId(tokenId);
 
         // Note: the tokenId is used as the salt.
         BalanceDelta delta = _modifyLiquidity(config, -(liquidity.toInt256()), bytes32(tokenId), hookData);
@@ -175,7 +176,8 @@ contract PositionManager is
         (uint256 tokenId, PositionConfig memory config, bytes memory hookData) =
             abi.decode(params, (uint256, PositionConfig, bytes));
 
-        _validateModify(config, tokenId);
+        if (!_isApprovedOrOwner(_getLocker(), tokenId)) revert NotApproved(_getLocker());
+        if (positionConfigs[tokenId] != config.toId()) revert IncorrectPositionConfigForTokenId(tokenId);
         uint256 liquidity = uint256(_getPositionLiquidity(config, tokenId));
 
         // Can only call modify if there is non zero liquidity.
@@ -189,11 +191,6 @@ contract PositionManager is
         return abi.encode(delta);
     }
 
-    function _validateModify(PositionConfig memory config, uint256 tokenId) private view {
-        if (!_isApprovedOrOwner(_getLocker(), tokenId)) revert NotApproved(_getLocker());
-        if (positionConfigs[tokenId] != config.toId()) revert IncorrectPositionConfigForTokenId(tokenId);
-    }
-
     function _modifyLiquidity(PositionConfig memory config, int256 liquidityChange, bytes32 salt, bytes memory hookData)
         internal
         returns (BalanceDelta liquidityDelta)

diff --git a/test/position-managers/IncreaseLiquidity.t.sol b/test/position-managers/IncreaseLiquidity.t.sol
@@ -11,6 +11,7 @@ import {PoolKey} from "@uniswap/v4-core/src/types/PoolKey.sol";
 import {BalanceDelta} from "@uniswap/v4-core/src/types/BalanceDelta.sol";
 import {LiquidityAmounts} from "@uniswap/v4-core/test/utils/LiquidityAmounts.sol";
 import {TickMath} from "@uniswap/v4-core/src/libraries/TickMath.sol";
+import {StateLibrary} from "@uniswap/v4-core/src/libraries/StateLibrary.sol";
 import {FixedPointMathLib} from "solmate/src/utils/FixedPointMathLib.sol";
 import {StateLibrary} from "@uniswap/v4-core/src/libraries/StateLibrary.sol";
 import {Fuzzers} from "@uniswap/v4-core/src/test/Fuzzers.sol";
@@ -164,6 +165,31 @@ contract IncreaseLiquidityTest is Test, PosmTestSetup, Fuzzers {
         assertApproxEqAbs(balance1BeforeAlice, currency1.balanceOf(alice), tolerance);
     }
 
+    function test_increaseLiquidity_withUnapprovedCaller() public {
+        // Alice provides liquidity
+        // Bob increases Alice's liquidity without being approved
+        uint256 liquidityAlice = 3_000e18;
+
+        // alice provides liquidity
+        vm.prank(alice);
+        mint(config, liquidityAlice, alice, ZERO_BYTES);
+        uint256 tokenIdAlice = lpm.nextTokenId() - 1;
+
+        bytes32 positionId =
+            keccak256(abi.encodePacked(address(lpm), config.tickLower, config.tickUpper, bytes32(tokenIdAlice)));
+        uint128 oldLiquidity = StateLibrary.getPositionLiquidity(manager, config.poolKey.toId(), positionId);
+
+        // bob can increase liquidity for alice even though he is not the owner / not approved
+        vm.startPrank(bob);
+        increaseLiquidity(tokenIdAlice, config, 100e18, ZERO_BYTES);
+        vm.stopPrank();
+
+        uint128 newLiquidity = StateLibrary.getPositionLiquidity(manager, config.poolKey.toId(), positionId);
+
+        // assert liqudity increased by the correct amount
+        assertEq(newLiquidity, oldLiquidity + uint128(100e18));
+    }
+
     function test_increaseLiquidity_sameRange_withExcessFees() public {
         // Alice and Bob provide liquidity on the same range
         // Alice uses half her fees to increase liquidity. The other half are collected to her wallet.