From 229987f0d0a4f88fe895e295aaf47fef5ec5977b Mon Sep 17 00:00:00 2001 From: andreivladbrg Date: Wed, 10 Apr 2024 16:36:22 +0300 Subject: [PATCH] refactor: allow anyone to call withdraw function refactor: improve custom error names docs: udpate natspec in withdraw function chore: improve wording in explanatory comments test: update withdraw tree and tests accordingly --- src/SablierV2OpenEnded.sol | 29 ++--- src/interfaces/ISablierV2OpenEnded.sol | 8 +- src/libraries/Errors.sol | 16 ++- test/integration/withdraw/withdraw.t.sol | 122 ++++++++++++------ test/integration/withdraw/withdraw.tree | 63 ++++----- .../withdrawableAmountOf.t.sol | 2 +- test/utils/Modifiers.sol | 16 ++- 7 files changed, 154 insertions(+), 102 deletions(-) diff --git a/src/SablierV2OpenEnded.sol b/src/SablierV2OpenEnded.sol index d8fe51f0..b03a034d 100644 --- a/src/SablierV2OpenEnded.sol +++ b/src/SablierV2OpenEnded.sol @@ -568,34 +568,29 @@ contract SablierV2OpenEnded is ISablierV2OpenEnded, NoDelegateCall, SablierV2Ope /// @dev See the documentation for the user-facing functions that call this internal function. function _withdraw(uint256 streamId, address to, uint40 time) internal noDelegateCall notCanceled(streamId) { - bool isCallerStreamSender = _isCallerStreamSender(streamId); - address recipient = _streams[streamId].recipient; - - // Checks: `msg.sender` is the stream's sender or the stream's recipient. - if (!isCallerStreamSender && msg.sender != recipient) { - revert Errors.SablierV2OpenEnded_Unauthorized(streamId, msg.sender); - } - - // Checks: the provided address is the recipient if `msg.sender` is the sender of the stream. - if (isCallerStreamSender && to != recipient) { - revert Errors.SablierV2OpenEnded_Unauthorized(streamId, msg.sender); - } - // Checks: the withdrawal address is not zero. if (to == address(0)) { revert Errors.SablierV2OpenEnded_WithdrawToZeroAddress(); } + // Retrieve the recipient from storage. + address recipient = _streams[streamId].recipient; + + // Check: if `msg.sender` is neither the stream's recipient, the withdrawal address must be the recipient. + if (to != recipient && !(msg.sender == recipient)) { + revert Errors.SablierV2OpenEnded_WithdrawalAddressNotRecipient(streamId, msg.sender, to); + } + uint40 lastTimeUpdate = _streams[streamId].lastTimeUpdate; - // Checks: the time reference is greater than the `lastTimeUpdate`. + // Checks: the withdrawal time is greater than the `lastTimeUpdate`. if (time <= lastTimeUpdate) { - revert Errors.SablierV2OpenEnded_TimeNotGreaterThanLastUpdate(time, lastTimeUpdate); + revert Errors.SablierV2OpenEnded_WithdrawalTimeNotGreaterThanLastUpdate(time, lastTimeUpdate); } - // Checks: the time reference is less than or equal to the current time. + // Checks: the time reference is not in the future. if (time > uint40(block.timestamp)) { - revert Errors.SablierV2OpenEnded_TimeNotLessOrEqualToCurrentTime(time, uint40(block.timestamp)); + revert Errors.SablierV2OpenEnded_WithdrawalTimeInTheFuture(time, block.timestamp); } // Calculate how much to withdraw based on the time reference. diff --git a/src/interfaces/ISablierV2OpenEnded.sol b/src/interfaces/ISablierV2OpenEnded.sol index 6d3293ff..51ab2df8 100644 --- a/src/interfaces/ISablierV2OpenEnded.sol +++ b/src/interfaces/ISablierV2OpenEnded.sol @@ -305,13 +305,11 @@ interface ISablierV2OpenEnded is ISablierV2OpenEndedState { /// /// Requirements: /// - Must not be delegate called. - /// - `streamId` must not reference a null stream. - /// - `streamId` must not reference a canceled stream. - /// - `msg.sender` must be the stream's sender or the stream's recipient. - /// - `to` must be the recipient if `msg.sender` is the stream's sender. + /// - `streamId` must not reference a null or canceled stream. /// - `to` must not be the zero address. + /// - `to` must be the recipient if `msg.sender` is not the stream's recipient. /// - `time` must be greater than the stream's `lastTimeUpdate`. - /// - `time` must be less than or equal to the current `block.timestamp`. + /// - `time` must not be greater than the `block.timestamp`. /// /// @param streamId The id of the stream to withdraw from. /// @param to The address receiving the withdrawn assets. diff --git a/src/libraries/Errors.sol b/src/libraries/Errors.sol index 9aff115b..3fce83e1 100644 --- a/src/libraries/Errors.sol +++ b/src/libraries/Errors.sol @@ -57,16 +57,18 @@ library Errors { /// @notice Thrown when trying to restart a stream that is not canceled. error SablierV2OpenEnded_StreamNotCanceled(uint256 streamId); - /// @notice Thrown when trying to withdraw assets with a time reference less than or equal to stream's - /// `lastTimeUpdate`. - error SablierV2OpenEnded_TimeNotGreaterThanLastUpdate(uint40 time, uint40 lastUpdate); - - /// @notice Thrown when trying to withdraw assets with a time reference greater than the current time. - error SablierV2OpenEnded_TimeNotLessOrEqualToCurrentTime(uint40 time, uint40 currentTime); - /// @notice Thrown when `msg.sender` lacks authorization to perform an action. error SablierV2OpenEnded_Unauthorized(uint256 streamId, address caller); + /// @notice Thrown when trying to withdraw to an address other than the recipient's. + error SablierV2OpenEnded_WithdrawalAddressNotRecipient(uint256 streamId, address caller, address to); + + /// @notice Thrown when trying to withdraw assets with a withdrawal time in the future. + error SablierV2OpenEnded_WithdrawalTimeInTheFuture(uint40 time, uint256 currentTime); + + /// @notice Thrown when trying to withdraw assets with a withdrawal time not greater than `lastTimeUpdate`. + error SablierV2OpenEnded_WithdrawalTimeNotGreaterThanLastUpdate(uint40 time, uint40 lastUpdate); + /// @notice Thrown when trying to withdraw to the zero address. error SablierV2OpenEnded_WithdrawToZeroAddress(); } diff --git a/test/integration/withdraw/withdraw.t.sol b/test/integration/withdraw/withdraw.t.sol index 35bd134f..a4339039 100644 --- a/test/integration/withdraw/withdraw.t.sol +++ b/test/integration/withdraw/withdraw.t.sol @@ -33,59 +33,67 @@ contract Withdraw_Integration_Test is Integration_Test { openEnded.withdraw({ streamId: defaultStreamId, to: users.recipient, time: WITHDRAW_TIME }); } - function test_RevertWhen_CallerUnauthorized_Sender() + function test_RevertWhen_ToZeroAddress() external whenNotDelegateCalled givenNotNull givenNotCanceled { + vm.expectRevert(Errors.SablierV2OpenEnded_WithdrawToZeroAddress.selector); + openEnded.withdraw({ streamId: defaultStreamId, to: address(0), time: WITHDRAW_TIME }); + } + + function test_RevertWhen_CallerUnknown() external whenNotDelegateCalled givenNotNull givenNotCanceled - whenCallerUnauthorized + whenToNonZeroAddress + whenWithdrawalAddressNotRecipient { + address unknownCaller = address(0xCAFE); + resetPrank({ msgSender: unknownCaller }); + vm.expectRevert( - abi.encodeWithSelector(Errors.SablierV2OpenEnded_Unauthorized.selector, defaultStreamId, users.sender) + abi.encodeWithSelector( + Errors.SablierV2OpenEnded_WithdrawalAddressNotRecipient.selector, + defaultStreamId, + unknownCaller, + unknownCaller + ) ); - openEnded.withdraw({ streamId: defaultStreamId, to: users.sender, time: WITHDRAW_TIME }); + + openEnded.withdraw({ streamId: defaultStreamId, to: unknownCaller, time: WITHDRAW_TIME }); } - function test_RevertWhen_CallerUnauthorized_MaliciousThirdParty(address maliciousThirdParty) + function test_RevertWhen_CallerSender() external whenNotDelegateCalled givenNotNull givenNotCanceled - whenCallerUnauthorized + whenToNonZeroAddress + whenWithdrawalAddressNotRecipient { - vm.assume(maliciousThirdParty != users.sender && maliciousThirdParty != users.recipient); - resetPrank({ msgSender: maliciousThirdParty }); + resetPrank({ msgSender: users.sender }); + vm.expectRevert( abi.encodeWithSelector( - Errors.SablierV2OpenEnded_Unauthorized.selector, defaultStreamId, maliciousThirdParty + Errors.SablierV2OpenEnded_WithdrawalAddressNotRecipient.selector, + defaultStreamId, + users.sender, + users.sender ) ); - openEnded.withdraw({ streamId: defaultStreamId, to: maliciousThirdParty, time: WITHDRAW_TIME }); - } - function test_RevertWhen_ToZeroAddress() - external - whenNotDelegateCalled - givenNotNull - givenNotCanceled - whenCallerAuthorized - { - resetPrank({ msgSender: users.recipient }); - vm.expectRevert(Errors.SablierV2OpenEnded_WithdrawToZeroAddress.selector); - openEnded.withdraw({ streamId: defaultStreamId, to: address(0), time: WITHDRAW_TIME }); + openEnded.withdraw({ streamId: defaultStreamId, to: users.sender, time: WITHDRAW_TIME }); } - function test_RevertWhen_TimeNotGreaterThanLastUpdate() + function test_RevertWhen_WithdrawalTimeNotGreaterThanLastUpdate() external whenNotDelegateCalled givenNotNull givenNotCanceled - whenCallerAuthorized whenToNonZeroAddress + whenWithdrawalAddressIsRecipient { vm.expectRevert( abi.encodeWithSelector( - Errors.SablierV2OpenEnded_TimeNotGreaterThanLastUpdate.selector, + Errors.SablierV2OpenEnded_WithdrawalTimeNotGreaterThanLastUpdate.selector, 0, openEnded.getLastTimeUpdate(defaultStreamId) ) @@ -93,20 +101,20 @@ contract Withdraw_Integration_Test is Integration_Test { openEnded.withdraw({ streamId: defaultStreamId, to: users.recipient, time: 0 }); } - function test_RevertWhen_TimeNotLessOrEqualToCurrentTime() + function test_RevertWhen_WithdrawalTimeInTheFuture() external whenNotDelegateCalled givenNotNull givenNotCanceled - whenCallerAuthorized whenToNonZeroAddress - whenTimeNotLessThanLastUpdate + whenWithdrawalAddressIsRecipient + whenWithdrawalTimeGreaterThanLastUpdate { uint40 futureTime = uint40(block.timestamp + 1); vm.expectRevert( abi.encodeWithSelector( - Errors.SablierV2OpenEnded_TimeNotLessOrEqualToCurrentTime.selector, futureTime, uint40(block.timestamp) + Errors.SablierV2OpenEnded_WithdrawalTimeInTheFuture.selector, futureTime, uint40(block.timestamp) ) ); openEnded.withdraw({ streamId: defaultStreamId, to: users.recipient, time: futureTime }); @@ -117,12 +125,44 @@ contract Withdraw_Integration_Test is Integration_Test { whenNotDelegateCalled givenNotNull givenNotCanceled - whenCallerAuthorized whenToNonZeroAddress - whenTimeNotLessThanLastUpdate - whenTimeNotGreaterThanCurrentTime + whenWithdrawalAddressIsRecipient + whenWithdrawalTimeGreaterThanLastUpdate + whenWithdrawalTimeNotInTheFuture { openEnded.withdraw({ streamId: defaultStreamId, to: users.recipient, time: WITHDRAW_TIME }); + + uint40 actualLastTimeUpdate = openEnded.getLastTimeUpdate(defaultStreamId); + uint40 expectedLastTimeUpdate = WITHDRAW_TIME; + assertEq(actualLastTimeUpdate, expectedLastTimeUpdate, "last time updated"); + + uint128 actualStreamBalance = openEnded.getBalance(defaultStreamId); + uint128 expectedStreamBalance = DEPOSIT_AMOUNT - WITHDRAW_AMOUNT; + assertEq(actualStreamBalance, expectedStreamBalance, "stream balance"); + } + + function test_Withdraw_CallerUnknown() + external + whenNotDelegateCalled + givenNotNull + givenNotCanceled + whenToNonZeroAddress + whenWithdrawalAddressIsRecipient + whenWithdrawalTimeGreaterThanLastUpdate + whenWithdrawalTimeNotInTheFuture + { + address unknownCaller = address(0xCAFE); + resetPrank({ msgSender: unknownCaller }); + + openEnded.withdraw({ streamId: defaultStreamId, to: users.recipient, time: WITHDRAW_TIME }); + + uint40 actualLastTimeUpdate = openEnded.getLastTimeUpdate(defaultStreamId); + uint40 expectedLastTimeUpdate = WITHDRAW_TIME; + assertEq(actualLastTimeUpdate, expectedLastTimeUpdate, "last time updated"); + + uint128 actualStreamBalance = openEnded.getBalance(defaultStreamId); + uint128 expectedStreamBalance = DEPOSIT_AMOUNT - WITHDRAW_AMOUNT; + assertEq(actualStreamBalance, expectedStreamBalance, "stream balance"); } function test_Withdraw_AssetNot18Decimals() @@ -130,10 +170,11 @@ contract Withdraw_Integration_Test is Integration_Test { whenNotDelegateCalled givenNotNull givenNotCanceled - whenCallerAuthorized whenToNonZeroAddress - whenTimeNotLessThanLastUpdate - whenTimeNotGreaterThanCurrentTime + whenWithdrawalAddressIsRecipient + whenWithdrawalTimeGreaterThanLastUpdate + whenWithdrawalTimeNotInTheFuture + whenCallerRecipient { // Set the timestamp to 1 month ago to create the stream with the same `lastTimeUpdate` as `defaultStreamId`. vm.warp({ newTimestamp: WARP_ONE_MONTH - ONE_MONTH }); @@ -141,7 +182,7 @@ contract Withdraw_Integration_Test is Integration_Test { openEnded.deposit(streamId, DEPOSIT_AMOUNT); vm.warp({ newTimestamp: WARP_ONE_MONTH }); - test_Withdraw(streamId, IERC20(address(usdt))); + _test_Withdraw(streamId, IERC20(address(usdt))); } function test_Withdraw() @@ -149,15 +190,16 @@ contract Withdraw_Integration_Test is Integration_Test { whenNotDelegateCalled givenNotNull givenNotCanceled - whenCallerAuthorized whenToNonZeroAddress - whenTimeNotLessThanLastUpdate - whenTimeNotGreaterThanCurrentTime + whenWithdrawalAddressIsRecipient + whenWithdrawalTimeGreaterThanLastUpdate + whenWithdrawalTimeNotInTheFuture + whenCallerRecipient { - test_Withdraw(defaultStreamId, dai); + _test_Withdraw(defaultStreamId, dai); } - function test_Withdraw(uint256 streamId, IERC20 asset) internal { + function _test_Withdraw(uint256 streamId, IERC20 asset) internal { resetPrank({ msgSender: users.recipient }); uint40 actualLastTimeUpdate = openEnded.getLastTimeUpdate(streamId); diff --git a/test/integration/withdraw/withdraw.tree b/test/integration/withdraw/withdraw.tree index 5f053b3c..3ecfd6ba 100644 --- a/test/integration/withdraw/withdraw.tree +++ b/test/integration/withdraw/withdraw.tree @@ -8,33 +8,36 @@ withdraw.t.sol ├── given the id references a canceled stream │ └── it should revert └── given the id does not reference a canceled stream - ├── when the caller is unauthorized - │ ├── when the caller is the sender - │ │ └── it should revert - │ └── when the caller is a malicious third party - │ └── it should revert - └── when the caller is authorized - ├── when the provided address is zero - │ └── it should revert - └── when the provided address is not zero - ├── when the time is less than or equal to the stream's last time update - │ └── it should revert - └── when the time is greater than the stream's last time update - ├── when the time greater than the current time - │ └── it should revert - └── when the time is not greater than the current time - ├── when the caller is the sender - │ └── it should make the withdrawal - └── when the caller is the recipient - ├── given the asset does not have 18 decimals - │ ├── it should make the withdrawal - │ ├── it should update the time - │ ├── it should update the stream balance - │ ├── it should perform the ERC-20 transfer - │ └── it should emit a {Transfer} and {WithdrawFromOpenEndedStream} event - └── given the asset has 18 decimals - ├── it should make the withdrawal - ├── it should update the time - ├── it should update the stream balance - ├── it should perform the ERC-20 transfer - └── it should emit a {Transfer} and {WithdrawFromOpenEndedStream} event \ No newline at end of file + ├── when the provided address is zero + │ └── it should revert + └── when the provided address is not zero + ├── when the withdrawal address is not the stream recipient + │ ├── when the caller is the sender + │ │ └── it should revert + │ └── when the caller is unknown + │ └── it should revert + └── when the withdrawal address is the stream recipient + ├── when the withdrawal time is not strictly greater than last time update + │ └── it should revert + └── when the withdrawal time is strictly greater than last time update + ├── when the withdrawal time is in the future + │ └── it should revert + └── when the withdrawal time is not in the future + ├── when the caller is not the recipient + │ ├── when the caller is the sender + │ │ └── it should make the withdrawal + │ └── when the caller is unknown + │ └── it should make the withdrawal + └── when the caller is the recipient + ├── given the asset does not have 18 decimals + │ ├── it should make the withdrawal + │ ├── it should update the time + │ ├── it should update the stream balance + │ ├── it should perform the ERC-20 transfer + │ └── it should emit a {Transfer} and {WithdrawFromOpenEndedStream} event + └── given the asset has 18 decimals + ├── it should make the withdrawal + ├── it should update the time + ├── it should update the stream balance + ├── it should perform the ERC-20 transfer + └── it should emit a {Transfer} and {WithdrawFromOpenEndedStream} event \ No newline at end of file diff --git a/test/integration/withdrawable-amount-of/withdrawableAmountOf.t.sol b/test/integration/withdrawable-amount-of/withdrawableAmountOf.t.sol index 0ed7c14e..a9467f26 100644 --- a/test/integration/withdrawable-amount-of/withdrawableAmountOf.t.sol +++ b/test/integration/withdrawable-amount-of/withdrawableAmountOf.t.sol @@ -18,7 +18,7 @@ contract WithdrawableAmountOf_Integration_Test is Integration_Test { openEnded.withdrawableAmountOf(defaultStreamId); } - function test_WithdrawableAmountOf_BalanceZero() external givenNotNull givenNotCanceled { + function test_WithdrawableAmountOf_BalanceZero() external view givenNotNull givenNotCanceled { uint128 withdrawableAmount = openEnded.withdrawableAmountOf(defaultStreamId); assertEq(withdrawableAmount, 0, "withdrawable amount"); } diff --git a/test/utils/Modifiers.sol b/test/utils/Modifiers.sol index 040ef00d..40ec01d0 100644 --- a/test/utils/Modifiers.sol +++ b/test/utils/Modifiers.sol @@ -90,11 +90,23 @@ abstract contract Modifiers { _; } - modifier whenTimeNotGreaterThanCurrentTime() { + modifier whenCallerRecipient() { _; } - modifier whenTimeNotLessThanLastUpdate() { + modifier whenWithdrawalAddressIsRecipient() { + _; + } + + modifier whenWithdrawalAddressNotRecipient() { + _; + } + + modifier whenWithdrawalTimeNotInTheFuture() { + _; + } + + modifier whenWithdrawalTimeGreaterThanLastUpdate() { _; } }