feat: allow NFT operator to withdraw max and transfer (#1054)

* feat: allow NFT operator to call 'withdrawMaxAndTransfer' * test: update and improve 'withdrawMaxAndTransfer'-related tests * docs: update 'withdrawMaxAndTransfer' related documentation * feat: allow NFT operator to call 'withdrawMaxAndTransfer' * test: update and improve 'withdrawMaxAndTransfer'-related tests * docs: update 'withdrawMaxAndTransfer' related documentation * feat: update 'burn' method to pass the 'currentRecipient' to caller check * test: use bulloak to generate test method name * test: polish tests --------- Co-authored-by: smol-ninja <[email protected]>
sablier-labs · Oct 8, 2024 · da4c076 · da4c076
1 parent e82b2de
commit da4c076
Show file tree

Hide file tree

Showing 4 changed files with 68 additions and 13 deletions.
diff --git a/src/core/abstracts/SablierLockup.sol b/src/core/abstracts/SablierLockup.sol
@@ -266,10 +266,13 @@ abstract contract SablierLockup is
             revert Errors.SablierLockup_StreamNotDepleted(streamId);
         }
 
+        // Retrieve the current owner.
+        address currentRecipient = _ownerOf(streamId);
+
         // Check:
         // 1. NFT exists (see {IERC721.getApproved}).
         // 2. `msg.sender` is either the owner of the NFT or an approved third party.
-        if (!_isCallerStreamRecipientOrApproved(streamId)) {
+        if (!_isCallerStreamRecipientOrApproved(streamId, currentRecipient)) {
             revert Errors.SablierLockup_Unauthorized(streamId, msg.sender);
         }
 
@@ -364,9 +367,9 @@ abstract contract SablierLockup is
         // Retrieve the recipient from storage.
         address recipient = _ownerOf(streamId);
 
-        // Check: if `msg.sender` is neither the stream's recipient nor an approved third party, the withdrawal address
+        // Check: `msg.sender` is neither the stream's recipient nor an approved third party, the withdrawal address
         // must be the recipient.
-        if (to != recipient && !_isCallerStreamRecipientOrApproved(streamId)) {
+        if (to != recipient && !_isCallerStreamRecipientOrApproved(streamId, recipient)) {
             revert Errors.SablierLockup_WithdrawalAddressNotRecipient(streamId, msg.sender, to);
         }
 
@@ -420,9 +423,11 @@ abstract contract SablierLockup is
         notNull(streamId)
         returns (uint128 withdrawnAmount)
     {
-        // Check: the caller is the current recipient. This also checks that the NFT was not burned.
+        // Retrieve the current owner. This also checks that the NFT was not burned.
         address currentRecipient = _ownerOf(streamId);
-        if (msg.sender != currentRecipient) {
+
+        // Check: `msg.sender` is neither the stream's recipient nor an approved third party.
+        if (!_isCallerStreamRecipientOrApproved(streamId, currentRecipient)) {
             revert Errors.SablierLockup_Unauthorized(streamId, msg.sender);
         }
 
@@ -467,10 +472,11 @@ abstract contract SablierLockup is
     /// @dev This function is implemented by child contracts, so the logic varies depending on the model.
     function _calculateStreamedAmount(uint256 streamId) internal view virtual returns (uint128);
 
-    /// @notice Checks whether `msg.sender` is the stream's recipient or an approved third party.
+    /// @notice Checks whether `msg.sender` is the stream's recipient or an approved third party, when the
+    /// `recipient` is known in advance.
     /// @param streamId The stream ID for the query.
-    function _isCallerStreamRecipientOrApproved(uint256 streamId) internal view returns (bool) {
-        address recipient = _ownerOf(streamId);
+    /// @param recipient The address of the stream's recipient.
+    function _isCallerStreamRecipientOrApproved(uint256 streamId, address recipient) internal view returns (bool) {
         return msg.sender == recipient || isApprovedForAll({ owner: recipient, operator: msg.sender })
             || getApproved(streamId) == msg.sender;
     }

diff --git a/src/core/interfaces/ISablierLockup.sol b/src/core/interfaces/ISablierLockup.sol
@@ -325,7 +325,7 @@ interface ISablierLockup is
     /// - Refer to the notes in {withdraw}.
     ///
     /// Requirements:
-    /// - `msg.sender` must be the stream's recipient.
+    /// - `msg.sender` must be either the NFT owner or an approved third party.
     /// - Refer to the requirements in {withdraw}.
     /// - Refer to the requirements in {IERC721.transferFrom}.
     ///

diff --git a/test/core/integration/concrete/lockup/withdraw-max-and-transfer/withdrawMaxAndTransfer.t.sol b/test/core/integration/concrete/lockup/withdraw-max-and-transfer/withdrawMaxAndTransfer.t.sol
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: UNLICENSED
 pragma solidity >=0.8.22 <0.9.0;
 
+import { IERC721Errors } from "@openzeppelin/contracts/interfaces/draft-IERC6093.sol";
 import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
 import { ISablierLockup } from "src/core/interfaces/ISablierLockup.sol";
 import { Errors } from "src/core/libraries/Errors.sol";
@@ -41,10 +42,8 @@ abstract contract WithdrawMaxAndTransfer_Integration_Concrete_Test is
         // Burn the NFT.
         lockup.burn({ streamId: defaultStreamId });
 
-        // It should revert.
-        vm.expectRevert(
-            abi.encodeWithSelector(Errors.SablierLockup_Unauthorized.selector, defaultStreamId, users.recipient)
-        );
+        // Run the test.
+        vm.expectRevert(abi.encodeWithSelector(IERC721Errors.ERC721NonexistentToken.selector, defaultStreamId));
         lockup.withdrawMaxAndTransfer({ streamId: defaultStreamId, newRecipient: users.alice });
     }
 
@@ -84,6 +83,53 @@ abstract contract WithdrawMaxAndTransfer_Integration_Concrete_Test is
         lockup.withdrawMaxAndTransfer({ streamId: defaultStreamId, newRecipient: users.eve });
     }
 
+    function test_WhenCallerApprovedThirdParty()
+        external
+        whenNoDelegateCall
+        givenNotNull
+        givenTransferableStream
+        givenNotBurnedNFT
+        givenNonZeroWithdrawableAmount
+    {
+        // Approve the operator to handle the stream.
+        lockup.approve({ to: users.operator, tokenId: defaultStreamId });
+
+        // Make the operator the caller in this test.
+        resetPrank({ msgSender: users.operator });
+
+        // Simulate the passage of time.
+        vm.warp({ newTimestamp: defaults.WARP_26_PERCENT() });
+
+        // Get the withdraw amount.
+        uint128 expectedWithdrawnAmount = lockup.withdrawableAmountOf(defaultStreamId);
+
+        // Expect the assets to be transferred to the Recipient.
+        expectCallToTransfer({ to: users.recipient, value: expectedWithdrawnAmount });
+
+        // It should emit {Transfer} and {WithdrawFromLockupStream} events.
+        vm.expectEmit({ emitter: address(lockup) });
+        emit WithdrawFromLockupStream({
+            streamId: defaultStreamId,
+            to: users.recipient,
+            amount: expectedWithdrawnAmount,
+            asset: dai
+        });
+        vm.expectEmit({ emitter: address(lockup) });
+        emit Transfer({ from: users.recipient, to: users.alice, tokenId: defaultStreamId });
+
+        // Make the max withdrawal and transfer the NFT.
+        uint128 actualWithdrawnAmount =
+            lockup.withdrawMaxAndTransfer({ streamId: defaultStreamId, newRecipient: users.alice });
+
+        // Assert that the withdrawn amount has been updated.
+        assertEq(actualWithdrawnAmount, expectedWithdrawnAmount, "withdrawnAmount");
+
+        // Assert that the operator is the new stream recipient (and NFT owner).
+        address actualRecipient = lockup.getRecipient(defaultStreamId);
+        address expectedRecipient = users.alice;
+        assertEq(actualRecipient, expectedRecipient, "recipient");
+    }
+
     function test_WhenCallerCurrentRecipient()
         external
         whenNoDelegateCall

diff --git a/test/core/integration/concrete/lockup/withdraw-max-and-transfer/withdrawMaxAndTransfer.tree b/test/core/integration/concrete/lockup/withdraw-max-and-transfer/withdrawMaxAndTransfer.tree
@@ -17,6 +17,9 @@ WithdrawMaxAndTransfer_Integration_Concrete_Test
             └── given non zero withdrawable amount
                ├── when caller not current recipient
                │  └── it should revert
+               ├── when caller approved third party
+               │  ├── it should update the withdrawn amount
+               │  └── it should transfer the NFT
                └── when caller current recipient
                   ├── it should update the withdrawn amount
                   ├── it should transfer the NFT