OZ-L07: Unsubscribe on Burn (+ remove arbitrary data) (#314)

* wip: unsubscribe on burn

* remove arbitrary data on unsubscribe

.forge-snapshots/PositionManager_burn_empty.snap

@@ -1 +1 @@
-47167
+47320

.forge-snapshots/PositionManager_burn_empty_native.snap

@@ -1 +1 @@
-46984
+47137

.forge-snapshots/PositionManager_burn_nonEmpty_native_withClose.snap

@@ -1 +1 @@
-123815
+123968

.forge-snapshots/PositionManager_burn_nonEmpty_native_withTakePair.snap

@@ -1 +1 @@
-123322
+123475

.forge-snapshots/PositionManager_burn_nonEmpty_withClose.snap

@@ -1 +1 @@
-130893
+131046

.forge-snapshots/PositionManager_burn_nonEmpty_withTakePair.snap

@@ -1 +1 @@
-130400
+130553

.forge-snapshots/PositionManager_decrease_burnEmpty.snap

@@ -1 +1 @@
-134968
+135121

.forge-snapshots/PositionManager_decrease_burnEmpty_native.snap

@@ -1 +1 @@
-127708
+127860

src/PositionManager.sol

@@ -328,6 +328,8 @@ contract PositionManager is
             (liquidityDelta - feesAccrued).validateMinOut(amount0Min, amount1Min);
         }
 
+        if (positionConfigs[tokenId].hasSubscriber()) _unsubscribe(tokenId, config);
+
         delete positionConfigs[tokenId];
         // Burn the token.
         _burn(tokenId);

src/base/Notifier.sol

@@ -59,19 +59,23 @@ abstract contract Notifier is INotifier {
     }
 
     /// @inheritdoc INotifier
-    function unsubscribe(uint256 tokenId, PositionConfig calldata config, bytes calldata data)
+    function unsubscribe(uint256 tokenId, PositionConfig calldata config)
         external
         payable
         onlyIfApproved(msg.sender, tokenId)
         onlyValidConfig(tokenId, config)
     {
+        _unsubscribe(tokenId, config);
+    }
+
+    function _unsubscribe(uint256 tokenId, PositionConfig calldata config) internal {
         _positionConfigs(tokenId).setUnsubscribe();
         ISubscriber _subscriber = subscriber[tokenId];
 
         delete subscriber[tokenId];
 
         uint256 subscriberGasLimit = block.gaslimit.calculatePortion(BLOCK_LIMIT_BPS);
-        try _subscriber.notifyUnsubscribe{gas: subscriberGasLimit}(tokenId, config, data) {} catch {}
+        try _subscriber.notifyUnsubscribe{gas: subscriberGasLimit}(tokenId, config) {} catch {}
 
         emit Unsubscribed(tokenId, address(_subscriber));
     }

src/interfaces/INotifier.sol

@@ -32,10 +32,9 @@ interface INotifier {
     /// @notice Removes the subscriber from receiving notifications for a respective position
     /// @param tokenId the ERC721 tokenId
     /// @param config the corresponding PositionConfig for the tokenId
-    /// @param data caller-provided data that's forwarded to the subscriber contract
     /// @dev payable so it can be multicalled with NATIVE related actions
     /// @dev Must always allow a user to unsubscribe. In the case of a malicious subscriber, a user can always unsubscribe safely, ensuring liquidity is always modifiable.
-    function unsubscribe(uint256 tokenId, PositionConfig calldata config, bytes calldata data) external payable;
+    function unsubscribe(uint256 tokenId, PositionConfig calldata config) external payable;
 
     /// @notice Returns whether a position should call out to notify a subscribing contract on modification or transfer
     /// @param tokenId the ERC721 tokenId

src/interfaces/ISubscriber.sol

@@ -12,8 +12,7 @@ interface ISubscriber {
     function notifySubscribe(uint256 tokenId, PositionConfig memory config, bytes memory data) external;
     /// @param tokenId the token ID of the position
     /// @param config details about the position
-    /// @param data additional data passed in by the caller
-    function notifyUnsubscribe(uint256 tokenId, PositionConfig memory config, bytes memory data) external;
+    function notifyUnsubscribe(uint256 tokenId, PositionConfig memory config) external;
     /// @param tokenId the token ID of the position
     /// @param config details about the position
     /// @param liquidityChange the change in liquidity on the underlying position

test/mocks/MockBadSubscribers.sol

@@ -34,7 +34,7 @@ contract MockReturnDataSubscriber is ISubscriber {
         notifySubscribeCount++;
     }
 
-    function notifyUnsubscribe(uint256, PositionConfig memory, bytes memory) external onlyByPosm {
+    function notifyUnsubscribe(uint256, PositionConfig memory) external onlyByPosm {
         notifyUnsubscribeCount++;
         uint256 _memPtr = memPtr;
         assembly {
@@ -83,7 +83,7 @@ contract MockRevertSubscriber is ISubscriber {
         }
     }
 
-    function notifyUnsubscribe(uint256, PositionConfig memory, bytes memory) external view onlyByPosm {
+    function notifyUnsubscribe(uint256, PositionConfig memory) external view onlyByPosm {
         revert TestRevert("notifyUnsubscribe");
     }
 

test/mocks/MockSubscriber.sol

@@ -18,7 +18,6 @@ contract MockSubscriber is ISubscriber {
     BalanceDelta public feesAccrued;
 
     bytes public subscribeData;
-    bytes public unsubscribeData;
 
     error NotAuthorizedNotifer(address sender);
 
@@ -38,9 +37,8 @@ contract MockSubscriber is ISubscriber {
         subscribeData = data;
     }
 
-    function notifyUnsubscribe(uint256, PositionConfig memory, bytes memory data) external onlyByPosm {
+    function notifyUnsubscribe(uint256, PositionConfig memory) external onlyByPosm {
         notifyUnsubscribeCount++;
-        unsubscribeData = data;
     }
 
     function notifyModifyLiquidity(uint256, PositionConfig memory, int256 _liquidityChange, BalanceDelta _feesAccrued)

test/position-managers/PositionManager.notifier.t.sol

@@ -222,7 +222,7 @@ contract PositionManagerNotifierTest is Test, PosmTestSetup, GasSnapshot {
 
         lpm.subscribe(tokenId, config, address(sub), ZERO_BYTES);
 
-        lpm.unsubscribe(tokenId, config, ZERO_BYTES);
+        lpm.unsubscribe(tokenId, config);
 
         assertEq(sub.notifyUnsubscribeCount(), 1);
         assertEq(lpm.hasSubscriber(tokenId), false);
@@ -241,7 +241,7 @@ contract PositionManagerNotifierTest is Test, PosmTestSetup, GasSnapshot {
         lpm.subscribe(tokenId, config, address(badSubscriber), ZERO_BYTES);
 
         MockReturnDataSubscriber(badSubscriber).setReturnDataSize(0x600000);
-        lpm.unsubscribe(tokenId, config, ZERO_BYTES);
+        lpm.unsubscribe(tokenId, config);
 
         // the subscriber contract call failed bc it used too much gas
         assertEq(MockReturnDataSubscriber(badSubscriber).notifyUnsubscribeCount(), 0);
@@ -321,7 +321,7 @@ contract PositionManagerNotifierTest is Test, PosmTestSetup, GasSnapshot {
         vm.stopPrank();
 
         vm.expectRevert();
-        lpm.unsubscribe(tokenId, config, ZERO_BYTES);
+        lpm.unsubscribe(tokenId, config);
     }
 
     function test_subscribe_withData() public {
@@ -343,27 +343,6 @@ contract PositionManagerNotifierTest is Test, PosmTestSetup, GasSnapshot {
         assertEq(abi.decode(sub.subscribeData(), (address)), address(this));
     }
 
-    function test_unsubscribe_withData() public {
-        uint256 tokenId = lpm.nextTokenId();
-        mint(config, 100e18, alice, ZERO_BYTES);
-
-        bytes memory subData = abi.encode(address(this));
-
-        // approve this contract to operate on alices liq
-        vm.startPrank(alice);
-        lpm.approve(address(this), tokenId);
-        vm.stopPrank();
-
-        lpm.subscribe(tokenId, config, address(sub), ZERO_BYTES);
-
-        lpm.unsubscribe(tokenId, config, subData);
-
-        assertEq(sub.notifyUnsubscribeCount(), 1);
-        assertEq(lpm.hasSubscriber(tokenId), false);
-        assertEq(address(lpm.subscriber(tokenId)), address(0));
-        assertEq(abi.decode(sub.unsubscribeData(), (address)), address(this));
-    }
-
     function test_subscribe_wraps_revert() public {
         uint256 tokenId = lpm.nextTokenId();
         mint(config, 100e18, alice, ZERO_BYTES);
@@ -477,4 +456,29 @@ contract PositionManagerNotifierTest is Test, PosmTestSetup, GasSnapshot {
         );
         lpm.safeTransferFrom(alice, bob, tokenId, "");
     }
+
+    /// @notice burning a position will automatically notify unsubscribe
+    function test_burn_unsubscribe() public {
+        uint256 tokenId = lpm.nextTokenId();
+        mint(config, 100e18, alice, ZERO_BYTES);
+
+        bytes memory subData = abi.encode(address(this));
+
+        // approve this contract to operate on alices liq
+        vm.startPrank(alice);
+        lpm.approve(address(this), tokenId);
+        vm.stopPrank();
+
+        lpm.subscribe(tokenId, config, address(sub), subData);
+
+        assertEq(lpm.hasSubscriber(tokenId), true);
+        assertEq(sub.notifyUnsubscribeCount(), 0);
+
+        // burn the position, causing an unsubscribe
+        burn(tokenId, config, ZERO_BYTES);
+
+        // position is now unsubscribed
+        assertEq(lpm.hasSubscriber(tokenId), false);
+        assertEq(sub.notifyUnsubscribeCount(), 1);
+    }
 }