Skip to content

Commit

Permalink
refactor: allow anyone to call withdraw function
Browse files Browse the repository at this point in the history 
refactor: improve custom error names
docs: udpate natspec in withdraw function
chore: improve wording in explanatory comments
test: update withdraw tree and tests accordingly
  • Loading branch information
@andreivladbrg
andreivladbrg committed Apr 10, 2024
1 parent 1391fe9 commit 229987f
Show file tree
Hide file tree
Showing 7 changed files with 154 additions and 102 deletions.
29 changes: 12 additions & 17 deletions src/SablierV2OpenEnded.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
8 changes: 3 additions & 5 deletions src/interfaces/ISablierV2OpenEnded.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
16 changes: 9 additions & 7 deletions src/libraries/Errors.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
122 changes: 82 additions & 40 deletions test/integration/withdraw/withdraw.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,80 +33,88 @@ 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)
)
);
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 });
Expand All @@ -117,47 +125,81 @@ 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()
external
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 });
uint256 streamId = createDefaultStreamWithAsset(IERC20(address(usdt)));
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()
external
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);
Expand Down
63 changes: 33 additions & 30 deletions test/integration/withdraw/withdraw.tree
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
├── 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
2 changes: 1 addition & 1 deletion test/integration/withdrawable-amount-of/withdrawableAmountOf.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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");
}
Expand Down
Loading

0 comments on commit 229987f

Please sign in to comment.