diff --git a/src/core/abstracts/SablierLockup.sol b/src/core/abstracts/SablierLockup.sol index 74a35d287..64fccd23f 100644 --- 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 index 5c2471161..123858a64 100644 --- 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 index 6274f9202..5af4f445a 100644 --- 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 index 57250a641..4fc64a99d 100644 --- 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