Skip to content

Commit

Permalink
feat: withdraw callable by any account (#785)
Browse files Browse the repository at this point in the history 
* feat: `withdraw` function callable by any account

feat: `withdrawMultiple` calls withdraw on streams recipient
test: fuzz test for with when the caller is an unknown address
refactor: rename custom error
chore: improve explanatory comments
test: undo authorized test branches
test: say uknown address instead of public caller
test: make withdraw fuzz test more minimalist

* refactor: withdraw related notes (#791)

* docs: improve writing in NatSpec comments

refactor: reorder errors
test: improve names for functions and variables

* refactor: withdraw tree

* refactor: order of checks in "withdraw"

test: polish withdraw tree and tests

---------

Co-authored-by: Paul Razvan Berg <[email protected]>
  • Loading branch information
@smol-ninja @PaulRBerg
smol-ninja and PaulRBerg authored Jan 23, 2024
1 parent 2487071 commit 3d30aaf
Show file tree
Hide file tree
Showing 13 changed files with 342 additions and 366 deletions.
27 changes: 10 additions & 17 deletions src/abstracts/SablierV2Lockup.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -241,21 +241,6 @@ abstract contract SablierV2Lockup is
revert Errors.SablierV2Lockup_StreamDepleted(streamId);
}

bool isCallerStreamSender = _isCallerStreamSender(streamId);

// Checks: `msg.sender` is the stream's sender, the stream's recipient, or an approved third party.
if (!isCallerStreamSender && !_isCallerStreamRecipientOrApproved(streamId)) {
revert Errors.SablierV2Lockup_Unauthorized(streamId, msg.sender);
}

// Retrieve the recipient from storage.
address recipient = _ownerOf(streamId);

// Checks: if `msg.sender` is the stream's sender, the withdrawal address must be the recipient.
if (isCallerStreamSender && to != recipient) {
revert Errors.SablierV2Lockup_InvalidSenderWithdrawal(streamId, msg.sender, to);
}

// Checks: the withdrawal address is not zero.
if (to == address(0)) {
revert Errors.SablierV2Lockup_WithdrawToZeroAddress();
Expand All @@ -266,6 +251,15 @@ abstract contract SablierV2Lockup is
revert Errors.SablierV2Lockup_WithdrawAmountZero(streamId);
}

// Retrieve the recipient from storage.
address recipient = _ownerOf(streamId);

// Checks: if `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)) {
revert Errors.SablierV2Lockup_WithdrawalAddressNotRecipient(streamId, msg.sender, to);
}

// Checks: the withdraw amount is not greater than the withdrawable amount.
uint128 withdrawableAmount = _withdrawableAmountOf(streamId);
if (amount > withdrawableAmount) {
Expand Down Expand Up @@ -337,7 +331,6 @@ abstract contract SablierV2Lockup is
/// @inheritdoc ISablierV2Lockup
function withdrawMultiple(
uint256[] calldata streamIds,
address to,
uint128[] calldata amounts
)
external
Expand All @@ -354,7 +347,7 @@ abstract contract SablierV2Lockup is
// Iterate over the provided array of stream ids and withdraw from each stream.
for (uint256 i = 0; i < streamIdsCount; ++i) {
// Checks, Effects and Interactions: check the parameters and make the withdrawal.
withdraw(streamIds[i], to, amounts[i]);
withdraw({ streamId: streamIds[i], to: _ownerOf(streamIds[i]), amount: amounts[i] });
}
}

Expand Down
17 changes: 9 additions & 8 deletions src/interfaces/ISablierV2Lockup.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -245,16 +245,15 @@ interface ISablierV2Lockup is
/// @dev Emits a {Transfer}, {WithdrawFromLockupStream}, and {MetadataUpdate} event.
///
/// Notes:
/// - This function attempts to invoke a hook on the stream's recipient, provided that the recipient is a contract
/// and `msg.sender` is either the sender or an approved operator.
/// - This function attempts to call a hook on the recipient of the stream, unless `msg.sender` is the recipient.
/// - This function attempts to call a hook on the sender of the stream, unless `msg.sender` is the sender.
///
/// Requirements:
/// - Must not be delegate called.
/// - `streamId` must not reference a null or depleted stream.
/// - `msg.sender` must be the stream's sender, the stream's recipient or an approved third party.
/// - `to` must be the recipient if `msg.sender` is the stream's sender.
/// - `to` must not be the zero address.
/// - `amount` must be greater than zero and must not exceed the withdrawable amount.
/// - `to` must be the recipient if `msg.sender` is not the stream's recipient or an approved third party.
///
/// @param streamId The id of the stream to withdraw from.
/// @param to The address receiving the withdrawn assets.
Expand Down Expand Up @@ -293,19 +292,21 @@ interface ISablierV2Lockup is
/// @param newRecipient The address of the new owner of the stream NFT.
function withdrawMaxAndTransfer(uint256 streamId, address newRecipient) external;

/// @notice Withdraws assets from streams to the provided address `to`.
/// @notice Withdraws assets from streams to the recipient of each stream.
///
/// @dev Emits multiple {Transfer}, {WithdrawFromLockupStream}, and {MetadataUpdate} events.
///
/// Notes:
/// - This function attempts to call a hook on the recipient of each stream, unless `msg.sender` is the recipient.
/// - This function attempts to call a hook on the sender of each stream, unless `msg.sender` is the sender.
///
/// Requirements:
/// - All requirements from {withdraw} must be met for each stream.
/// - Must not be delegate called.
/// - There must be an equal number of `streamIds` and `amounts`.
/// - Each stream id in the array must not reference a null or depleted stream.
/// - Each amount in the array must be greater than zero and must not exceed the withdrawable amount.
///
/// @param streamIds The ids of the streams to withdraw from.
/// @param to The address receiving the withdrawn assets.
/// @param amounts The amounts to withdraw, denoted in units of the asset's decimals.
function withdrawMultiple(uint256[] calldata streamIds, address to, uint128[] calldata amounts) external;
function withdrawMultiple(uint256[] calldata streamIds, uint128[] calldata amounts) external;
}
2 changes: 1 addition & 1 deletion src/interfaces/hooks/ISablierV2Recipient.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ interface ISablierV2Recipient {
/// @param streamId The id of the renounced stream.
function onLockupStreamRenounced(uint256 streamId) external;

/// @notice Responds to withdrawals triggered by either the stream's sender or an approved third party.
/// @notice Responds to withdrawals triggered by any address except the contract implementing this interface.
///
/// @dev Notes:
/// - This function may revert, but the Sablier contract will ignore the revert.
Expand Down
2 changes: 1 addition & 1 deletion src/interfaces/hooks/ISablierV2Sender.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ pragma solidity >=0.8.22;
/// @dev Implementation of this interface is optional. If a sender contract doesn't implement this
/// interface or implements it partially, function execution will not revert.
interface ISablierV2Sender {
/// @notice Responds to withdrawals triggered by either the stream's recipient or an approved third party.
/// @notice Responds to withdrawals triggered by any address except the contract implementing this interface.
///
/// @dev Notes:
/// - This function may revert, but the Sablier contract will ignore the revert.
Expand Down
6 changes: 3 additions & 3 deletions src/libraries/Errors.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,6 @@ library Errors {
/// @notice Thrown when trying to create a stream with an end time not in the future.
error SablierV2Lockup_EndTimeNotInTheFuture(uint40 currentTime, uint40 endTime);

/// @notice Thrown when the stream's sender tries to withdraw to an address other than the recipient's.
error SablierV2Lockup_InvalidSenderWithdrawal(uint256 streamId, address sender, address to);

/// @notice Thrown when trying to transfer Stream NFT when transferability is disabled.
error SablierV2Lockup_NotTransferable(uint256 tokenId);

Expand Down Expand Up @@ -71,6 +68,9 @@ library Errors {
/// @notice Thrown when `msg.sender` lacks authorization to perform an action.
error SablierV2Lockup_Unauthorized(uint256 streamId, address caller);

/// @notice Thrown when trying to withdraw to an address other than the recipient's.
error SablierV2Lockup_WithdrawalAddressNotRecipient(uint256 streamId, address caller, address to);

/// @notice Thrown when trying to withdraw zero assets from a stream.
error SablierV2Lockup_WithdrawAmountZero(uint256 streamId);

Expand Down
129 changes: 9 additions & 120 deletions test/integration/concrete/lockup/withdraw-multiple/withdrawMultiple.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,7 @@ abstract contract WithdrawMultiple_Integration_Concrete_Test is
}

function test_RevertWhen_DelegateCalled() external {
bytes memory callData =
abi.encodeCall(ISablierV2Lockup.withdrawMultiple, (testStreamIds, users.recipient, testAmounts));
bytes memory callData = abi.encodeCall(ISablierV2Lockup.withdrawMultiple, (testStreamIds, testAmounts));
(bool success, bytes memory returnData) = address(lockup).delegatecall(callData);
expectRevertDueToDelegateCall(success, returnData);
}
Expand All @@ -33,7 +32,7 @@ abstract contract WithdrawMultiple_Integration_Concrete_Test is
Errors.SablierV2Lockup_WithdrawArrayCountsNotEqual.selector, streamIds.length, amounts.length
)
);
lockup.withdrawMultiple({ streamIds: streamIds, to: users.recipient, amounts: amounts });
lockup.withdrawMultiple(streamIds, amounts);
}

modifier whenArrayCountsAreEqual() {
Expand All @@ -43,7 +42,7 @@ abstract contract WithdrawMultiple_Integration_Concrete_Test is
function test_WithdrawMultiple_ArrayCountsZero() external whenNotDelegateCalled whenArrayCountsAreEqual {
uint256[] memory streamIds = new uint256[](0);
uint128[] memory amounts = new uint128[](0);
lockup.withdrawMultiple({ streamIds: streamIds, to: users.recipient, amounts: amounts });
lockup.withdrawMultiple(streamIds, amounts);
}

modifier whenArrayCountsNotZero() {
Expand All @@ -61,7 +60,6 @@ abstract contract WithdrawMultiple_Integration_Concrete_Test is
vm.expectRevert(abi.encodeWithSelector(Errors.SablierV2Lockup_Null.selector, nullStreamId));
lockup.withdrawMultiple({
streamIds: Solarray.uint256s(nullStreamId),
to: users.recipient,
amounts: Solarray.uint128s(withdrawAmount)
});
}
Expand All @@ -82,7 +80,7 @@ abstract contract WithdrawMultiple_Integration_Concrete_Test is
vm.expectRevert(abi.encodeWithSelector(Errors.SablierV2Lockup_Null.selector, nullStreamId));

// Withdraw from multiple streams.
lockup.withdrawMultiple({ streamIds: streamIds, to: users.recipient, amounts: testAmounts });
lockup.withdrawMultiple({ streamIds: streamIds, amounts: testAmounts });
}

function test_RevertGiven_AllStatusesDepleted()
Expand All @@ -105,7 +103,7 @@ abstract contract WithdrawMultiple_Integration_Concrete_Test is
vm.expectRevert(abi.encodeWithSelector(Errors.SablierV2Lockup_StreamDepleted.selector, testStreamIds[0]));

// Withdraw from multiple streams.
lockup.withdrawMultiple({ streamIds: streamIds, to: users.recipient, amounts: amounts });
lockup.withdrawMultiple(streamIds, amounts);
}

function test_RevertGiven_SomeStatusesDepleted()
Expand All @@ -125,113 +123,7 @@ abstract contract WithdrawMultiple_Integration_Concrete_Test is
vm.expectRevert(abi.encodeWithSelector(Errors.SablierV2Lockup_StreamDepleted.selector, testStreamIds[0]));

// Withdraw from multiple streams.
lockup.withdrawMultiple({ streamIds: testStreamIds, to: users.recipient, amounts: testAmounts });
}

function test_RevertWhen_CallerUnauthorizedAllStreams_MaliciousThirdParty()
external
whenNotDelegateCalled
whenArrayCountsAreEqual
whenArrayCountsNotZero
givenNoNull
givenNoDepletedStream
whenCallerUnauthorized
{
// Make Eve the caller in this test.
changePrank({ msgSender: users.eve });

// Run the test.
vm.expectRevert(
abi.encodeWithSelector(Errors.SablierV2Lockup_Unauthorized.selector, testStreamIds[0], users.eve)
);
lockup.withdrawMultiple({ streamIds: testStreamIds, to: users.recipient, amounts: testAmounts });
}

function test_RevertWhen_CallerUnauthorizedAllStreams_FormerRecipient()
external
whenNotDelegateCalled
whenArrayCountsAreEqual
whenArrayCountsNotZero
givenNoNull
givenNoDepletedStream
whenCallerUnauthorized
{
// Transfer all streams to Alice.
changePrank({ msgSender: users.recipient });
lockup.transferFrom({ from: users.recipient, to: users.alice, tokenId: testStreamIds[0] });
lockup.transferFrom({ from: users.recipient, to: users.alice, tokenId: testStreamIds[1] });
lockup.transferFrom({ from: users.recipient, to: users.alice, tokenId: testStreamIds[2] });

// Run the test.
vm.expectRevert(
abi.encodeWithSelector(Errors.SablierV2Lockup_Unauthorized.selector, testStreamIds[0], users.recipient)
);
lockup.withdrawMultiple({ streamIds: testStreamIds, to: users.recipient, amounts: testAmounts });
}

function test_RevertWhen_CallerUnauthorizedSomeStreams_MaliciousThirdParty()
external
whenNotDelegateCalled
whenArrayCountsAreEqual
whenArrayCountsNotZero
givenNoNull
givenNoDepletedStream
whenCallerUnauthorized
{
// Create a stream with Eve as the stream's recipient.
uint256 eveStreamId = createDefaultStreamWithRecipient(users.eve);

// Make Eve the caller in this test.
changePrank({ msgSender: users.eve });

// Simulate the passage of time.
vm.warp({ timestamp: defaults.WARP_26_PERCENT() });

// Run the test.
uint256[] memory streamIds = Solarray.uint256s(eveStreamId, testStreamIds[0], testStreamIds[1]);
vm.expectRevert(
abi.encodeWithSelector(Errors.SablierV2Lockup_Unauthorized.selector, testStreamIds[0], users.eve)
);
lockup.withdrawMultiple({ streamIds: streamIds, to: users.recipient, amounts: testAmounts });
}

function test_RevertWhen_CallerUnauthorizedSomeStreams_FormerRecipient()
external
whenNotDelegateCalled
whenArrayCountsAreEqual
whenArrayCountsNotZero
givenNoNull
givenNoDepletedStream
whenCallerUnauthorized
{
// Transfer one of the streams to Eve.
changePrank({ msgSender: users.recipient });
lockup.transferFrom({ from: users.recipient, to: users.alice, tokenId: testStreamIds[0] });

// Simulate the passage of time.
vm.warp({ timestamp: defaults.WARP_26_PERCENT() });

// Run the test.
vm.expectRevert(
abi.encodeWithSelector(Errors.SablierV2Lockup_Unauthorized.selector, testStreamIds[0], users.recipient)
);
lockup.withdrawMultiple({ streamIds: testStreamIds, to: users.recipient, amounts: testAmounts });
}

function test_RevertWhen_ToZeroAddress()
external
whenNotDelegateCalled
whenArrayCountsAreEqual
whenArrayCountsNotZero
givenNoNull
givenNoDepletedStream
whenCallerAuthorizedAllStreams
{
if (caller == users.sender) {
return;
}
vm.expectRevert(Errors.SablierV2Lockup_WithdrawToZeroAddress.selector);
lockup.withdrawMultiple({ streamIds: testStreamIds, to: address(0), amounts: testAmounts });
lockup.withdrawMultiple({ streamIds: testStreamIds, amounts: testAmounts });
}

function test_RevertWhen_SomeAmountsZero()
Expand All @@ -241,7 +133,6 @@ abstract contract WithdrawMultiple_Integration_Concrete_Test is
whenArrayCountsNotZero
givenNoNull
givenNoDepletedStream
whenCallerAuthorizedAllStreams
whenToNonZeroAddress
{
// Simulate the passage of time.
Expand All @@ -250,7 +141,7 @@ abstract contract WithdrawMultiple_Integration_Concrete_Test is
// Run the test.
uint128[] memory amounts = Solarray.uint128s(defaults.WITHDRAW_AMOUNT(), 0, 0);
vm.expectRevert(abi.encodeWithSelector(Errors.SablierV2Lockup_WithdrawAmountZero.selector, testStreamIds[1]));
lockup.withdrawMultiple({ streamIds: testStreamIds, to: users.recipient, amounts: amounts });
lockup.withdrawMultiple({ streamIds: testStreamIds, amounts: amounts });
}

function test_RevertWhen_SomeAmountsOverdraw()
Expand All @@ -260,7 +151,6 @@ abstract contract WithdrawMultiple_Integration_Concrete_Test is
whenArrayCountsNotZero
givenNoNull
givenNoDepletedStream
whenCallerAuthorizedAllStreams
whenToNonZeroAddress
whenNoAmountZero
{
Expand All @@ -275,7 +165,7 @@ abstract contract WithdrawMultiple_Integration_Concrete_Test is
Errors.SablierV2Lockup_Overdraw.selector, testStreamIds[2], MAX_UINT128, withdrawableAmount
)
);
lockup.withdrawMultiple({ streamIds: testStreamIds, to: users.recipient, amounts: amounts });
lockup.withdrawMultiple({ streamIds: testStreamIds, amounts: amounts });
}

function test_WithdrawMultiple()
Expand All @@ -285,7 +175,6 @@ abstract contract WithdrawMultiple_Integration_Concrete_Test is
whenArrayCountsNotZero
givenNoNull
givenNoDepletedStream
whenCallerAuthorizedAllStreams
whenToNonZeroAddress
whenNoAmountZero
whenNoAmountOverdraws
Expand Down Expand Up @@ -329,7 +218,7 @@ abstract contract WithdrawMultiple_Integration_Concrete_Test is
});

// Make the withdrawals.
lockup.withdrawMultiple({ streamIds: testStreamIds, to: users.recipient, amounts: testAmounts });
lockup.withdrawMultiple({ streamIds: testStreamIds, amounts: testAmounts });

// Assert that the statuses have been updated.
assertEq(lockup.statusOf(testStreamIds[0]), Lockup.Status.STREAMING, "status0");
Expand Down
32 changes: 9 additions & 23 deletions test/integration/concrete/lockup/withdraw-multiple/withdrawMultiple.tree
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,27 +18,13 @@ withdrawMultiple.t.sol
├── given some streams' statuses are "DEPLETED"
│ └── it should revert
└── given no stream's status is "DEPLETED"
├── when the caller is unauthorized for all streams
│ ├── when the caller is a malicious third party
│ │ └── it should revert
│ └── when the caller is a former recipient
│ └── it should revert
├── when the caller is unauthorized for some streams
│ ├── when the caller is a malicious third party
│ │ └── it should revert
│ └── when the caller is a former recipient
│ └── it should revert
└── when the caller is authorized for all streams
├── when the provided address is zero
├── when some amounts are zero
│ └── it should revert
└── when none of the amounts are zero
├── when some amounts overdraw
│ └── it should revert
└── when the provided address is not zero
├── when some amounts are zero
│ └── it should revert
└── when none of the amounts are zero
├── when some amounts overdraw
│ └── it should revert
└── when no amount overdraws
├── it should make the withdrawals
├── it should update the statuses
├── it should update the withdrawn amounts
└── it should emit multiple {WithdrawFromLockupStream} events
└── when no amount overdraws
├── it should make the withdrawals
├── it should update the statuses
├── it should update the withdrawn amounts
└── it should emit multiple {WithdrawFromLockupStream} events
Loading

0 comments on commit 3d30aaf

Please sign in to comment.