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

Loss of funds to users through unsafe and unchecked withdrawal process

stackbuster23

Summary

Streams could be withdrawn and stolen from contract by malicious actors.

Vulnerability Details

In the SablierV2Lockup.sol (which Handles common logic between all Sablier V2 Lockup contracts), and is inherited by the SablierV2LockupDynamic.sol, SablierV2LockupLinear.sol and SablierV2LockupTranched.sol smart contracts. The contract has functions to enable the processing of withdraws by calling the following exposed withdrawal methods function withdraw (uint256 streamId, address to, uint128 amount) public, function withdrawMax(uint256 streamId, address to) external,function withdrawMaxAndTransfer(uint256 streamId, address newRecipient) external and function withdrawMultiple(uint256[] calldata streamIds,uint128[] calldata amounts) external, however most of the logic that ensures the integrity of the process takes place in the public withdraw function and the transfer proper done in the contract's internal withdraw function function _withdraw(uint256 streamId, address to, uint128 amount) internal. The issue is the internal function does not carry out the necessary checks to ensure the integrity of the withdrawal process like the public withdraw function( function withdraw (uint256 streamId, address to, uint128 amount) public) does. As Internal functions are accessible to any derived contract, this could lead to malicious users withdrawing tokens with valid streamid meant for a different recipient or recipients also withdrawing before stream expires, basically it exposes the contract to serious abuse. Internal functions can be called within the contract that defines them and in derived contracts. Since internal Function is marked as internal, it cannot be called by external contracts or users, but it can be called within the contract itself and in any derived contracts. Also, even if the contracts are already deployed on the blockchain, the internal functions can still be called through inheritance. When you deploy a new contract that inherits from another contract, the new contract's bytecode includes the inherited functions, making them available to the inheriting contract.

Impact

There is potential for loss of funds to users of the protocol, the impact is high although the likelihood is medium as a valid streamid would be required to ensure it succeeds. Moreover, Valid streamids shared between actors are susceptible to been abused.

Tools Used

manual review

Recommendations

The major logics used in the function withdraw (uint256 streamId, address to, uint128 amount) public should instead be moved to the internal function where the transfer of tokens to the recipient address is actually carried out.

​
- function withdraw(
uint256 streamId,
address to,
uint128 amount
)
public
override
noDelegateCall
notNull(streamId)
updateMetadata(streamId)
{
// Check: the stream is not depleted.
if (_streams[streamId].isDepleted) {
revert Errors.SablierV2Lockup_StreamDepleted(streamId);
}
​
// Check: the withdrawal address is not zero.
if (to == address(0)) {
revert Errors.SablierV2Lockup_WithdrawToZeroAddress(streamId);
}
​
// Check: the withdraw amount is not zero.
if (amount == 0) {
revert Errors.SablierV2Lockup_WithdrawAmountZero(streamId);
}
​
// 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
// must be the recipient.
if (to != recipient && !_isCallerStreamRecipientOrApproved(streamId)) {
revert Errors.SablierV2Lockup_WithdrawalAddressNotRecipient(streamId, msg.sender, to);
}
​
// Check: the withdraw amount is not greater than the withdrawable amount.
uint128 withdrawableAmount = _withdrawableAmountOf(streamId);
if (amount > withdrawableAmount) {
revert Errors.SablierV2Lockup_Overdraw(streamId, amount, withdrawableAmount);
}
​
// Retrieve the sender from storage.
address sender = _streams[streamId].sender;
​
// Effects and Interactions: make the withdrawal.
_withdraw(streamId, to, amount);
​
// Interaction: if `msg.sender` is not the recipient and the recipient is a contract, try to invoke the
// withdraw hook on it without reverting if the hook is not implemented, and also without bubbling up
// any potential revert.
if (msg.sender != recipient && recipient.code.length > 0) {
try ISablierV2Recipient(recipient).onLockupStreamWithdrawn({
streamId: streamId,
caller: msg.sender,
to: to,
amount: amount
}) { } catch { }
}
​
// Interaction: if `msg.sender` is not the sender, the sender is a contract and is different from the
// recipient, try to invoke the withdraw hook on it without reverting if the hook is not implemented, and also
// without bubbling up any potential revert.
if (msg.sender != sender && sender.code.length > 0 && sender != recipient) {
try ISablierV2Sender(sender).onLockupStreamWithdrawn({
streamId: streamId,
caller: msg.sender,
to: to,
amount: amount
}) { } catch { }
}
}
​
+ function withdraw(
uint256 streamId,
address to,
uint128 amount
)
public
override
noDelegateCall
notNull(streamId)
updateMetadata(streamId)
{
_withdraw(uint256 streamId, address to, uint128 amount);
}
​

The logic should be in the internal function to avoid bypass.

+ /// @dev See the documentation for the user-facing functions that call this internal function.
function _withdraw(uint256 streamId, address to, uint128 amount) internal {
// Check: the stream is not depleted.
if (_streams[streamId].isDepleted) {
revert Errors.SablierV2Lockup_StreamDepleted(streamId);
}
​
// Check: the withdrawal address is not zero.
if (to == address(0)) {
revert Errors.SablierV2Lockup_WithdrawToZeroAddress(streamId);
}
​
// Check: the withdraw amount is not zero.
if (amount == 0) {
revert Errors.SablierV2Lockup_WithdrawAmountZero(streamId);
}
​
// 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
// must be the recipient.
if (to != recipient && !_isCallerStreamRecipientOrApproved(streamId)) {
revert Errors.SablierV2Lockup_WithdrawalAddressNotRecipient(streamId, msg.sender, to);
}
​
// Check: the withdraw amount is not greater than the withdrawable amount.
uint128 withdrawableAmount = _withdrawableAmountOf(streamId);
if (amount > withdrawableAmount) {
revert Errors.SablierV2Lockup_Overdraw(streamId, amount, withdrawableAmount);
}
​
// Retrieve the sender from storage.
address sender = _streams[streamId].sender;
​
// Effects and Interactions: make the withdrawal.
_withdraw(streamId, to, amount);
​
// Interaction: if `msg.sender` is not the recipient and the recipient is a contract, try to invoke the
// withdraw hook on it without reverting if the hook is not implemented, and also without bubbling up
// any potential revert.
if (msg.sender != recipient && recipient.code.length > 0) {
try ISablierV2Recipient(recipient).onLockupStreamWithdrawn({
streamId: streamId,
caller: msg.sender,
to: to,
amount: amount
}) { } catch { }
}
​
// Interaction: if `msg.sender` is not the sender, the sender is a contract and is different from the
// recipient, try to invoke the withdraw hook on it without reverting if the hook is not implemented, and also
// without bubbling up any potential revert.
if (msg.sender != sender && sender.code.length > 0 && sender != recipient) {
try ISablierV2Sender(sender).onLockupStreamWithdrawn({
streamId: streamId,
caller: msg.sender,
to: to,
amount: amount
}) { } catch { }
}
// Effect: update the withdrawn amount.
_streams[streamId].amounts.withdrawn = _streams[streamId].amounts.withdrawn + amount;
​
// Retrieve the amounts from storage.
Lockup.Amounts memory amounts = _streams[streamId].amounts;
​
// Using ">=" instead of "==" for additional safety reasons. In the event of an unforeseen increase in the
// withdrawn amount, the stream will still be marked as depleted.
if (amounts.withdrawn >= amounts.deposited - amounts.refunded) {
// Effect: mark the stream as depleted.
_streams[streamId].isDepleted = true;
​
// Effect: make the stream not cancelable anymore, because a depleted stream cannot be canceled.
_streams[streamId].isCancelable = false;
}
​
// Retrieve the ERC-20 asset from storage.
IERC20 asset = _streams[streamId].asset;
​
// Interaction: perform the ERC-20 transfer.
asset.safeTransfer({ to: to, value: amount });
​
// Log the withdrawal.
emit ISablierV2Lockup.WithdrawFromLockupStream(streamId, to, asset, amount);
}
​
Updates

Lead Judging Commences

inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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