Sablier
Sablier
DeFiFoundry
53,440 USDC
View results
Previous Next
Submission Details
Severity: low
Invalid

SablierV2Lockup.sol:withdrawMaxAndTransfer does not need to require the msg.sender is stream's recipient

lilizhu

Summary

The SablierV2Lockup.sol:withdrawMaxAndTransfer function requires the caller to be the recipient. This requirement is unnecessary because an approved user can perform the withdraw the max amount from the stream And transfer the NFT in a step-by-step process to implementing the same function. So there is no need to require the caller of the withdrawMaxAndTransfer must be the user of stream's recipient.

Vulnerability Details

In the v2-core/src/abstracts/SablierV2Lockup.sol:withdrawMaxAndTransfer function documentation, it states that msg.sender must be the stream's recipient. This restriction is too strict. In reality, msg.sender can be either the recipient or an authorized user of the recipient to perform this function.

The function's purpose is: Withdraws the maximum withdrawable amount from the stream to the current recipient, and transfers the NFT to newRecipient.

However, in practice, if the stream's recipient is Alice and Alice has authorized Bob to access the stream, then Bob can withdraw the stream to the newRecipient and also transfer the NFT of the stream ID. Therefore, there is no necessity to restrict caller si Alice, because Bob CAN perform the same thing.

https://github.com/Cyfrin/2024-05-Sablier/blob/43d7e752a68bba2a1d73d3d6466c3059079ed0c6/v2-core/src/abstracts/SablierV2Lockup.sol#L409-L433

/// @inheritdoc ISablierV2Lockup
function withdrawMaxAndTransfer(
uint256 streamId,
address newRecipient
)
external
override
noDelegateCall
notNull(streamId)
{
// Check: the caller is the current recipient. This also checks that the NFT was not burned.
address currentRecipient = _ownerOf(streamId);
if (msg.sender != currentRecipient) {
revert Errors.SablierV2Lockup_Unauthorized(streamId, msg.sender);
}
​
// Skip the withdrawal if the withdrawable amount is zero.
uint128 withdrawableAmount = _withdrawableAmountOf(streamId);
if (withdrawableAmount > 0) {
withdraw({ streamId: streamId, to: currentRecipient, amount: withdrawableAmount });
}
​
// Checks and Effects: transfer the NFT.
_transfer({ from: currentRecipient, to: newRecipient, tokenId: streamId });
}
​

Impact

It is useless and misleading to require the caller is stream's recipient, but it should also work for approved users.

Tools Used

Manual Review

Recommendations

Change the validation logic for msg.sender to _isCallerStreamRecipientOrApproved.

/// @inheritdoc ISablierV2Lockup
function withdrawMaxAndTransfer(
uint256 streamId,
address newRecipient
)
external
override
noDelegateCall
notNull(streamId)
{
// Check: the caller is the current recipient. This also checks that the NFT was not burned.
address currentRecipient = _ownerOf(streamId);
- if (msg.sender != currentRecipient) {
+ if (!_isCallerStreamRecipientOrApproved(streamId)) {
revert Errors.SablierV2Lockup_Unauthorized(streamId, msg.sender);
}
​
// Skip the withdrawal if the withdrawable amount is zero.
uint128 withdrawableAmount = _withdrawableAmountOf(streamId);
if (withdrawableAmount > 0) {
withdraw({ streamId: streamId, to: currentRecipient, amount: withdrawableAmount });
}
​
// Checks and Effects: transfer the NFT.
_transfer({ from: currentRecipient, to: newRecipient, tokenId: streamId });
}
​
Updates

Lead Judging Commences

inallhonesty Lead Judge over 1 year ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

Info/Gas/Invalid as per Docs

https://docs.codehawks.com/hawks-auditors/how-to-determine-a-finding-validity

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.