Flow
Sablier
FoundryDeFi
20,000 USDC
View results
Previous Next
Submission Details
Severity: medium
Invalid

Deposited funds can be stuck to the contract forever

mrmorningstar

Summary

When someone create stream using create or creadeAndDeposit it eventually create stream via _create function that looks like this:

/// @dev See the documentation for the user-facing functions that call this internal function.
function _create(
address sender,
address recipient,
UD21x18 ratePerSecond,
IERC20 token,
bool transferable
)
internal
returns (uint256 streamId)
{
// Check: the sender is not the zero address.
if (sender == address(0)) {
revert Errors.SablierFlow_SenderZeroAddress();
}
​
uint8 tokenDecimals = IERC20Metadata(address(token)).decimals();
​
// Check: the token decimals are not greater than 18.
if (tokenDecimals > 18) {
revert Errors.SablierFlow_InvalidTokenDecimals(address(token));
}
​
// Load the stream ID.
streamId = nextStreamId;
​
// Effect: create the stream.
_streams[streamId] = Flow.Stream({
balance: 0,
isStream: true,
isTransferable: transferable,
isVoided: false,
ratePerSecond: ratePerSecond,
sender: sender,
snapshotDebtScaled: 0,
snapshotTime: uint40(block.timestamp),
token: token,
tokenDecimals: tokenDecimals
});
​
// Using unchecked arithmetic because this calculation can never realistically overflow.
unchecked {
// Effect: bump the next stream ID.
nextStreamId = streamId + 1;
}
​
// Effect: mint the NFT to the recipient.
_mint({ to: recipient, tokenId: streamId });
​
// Log the newly created stream.
emit ISablierFlow.CreateFlowStream({
streamId: streamId,
sender: sender,
recipient: recipient,
ratePerSecond: ratePerSecond,
token: token,
transferable: transferable
});
}

As we can see function after necessary check create stream and mint NFT to the recipient via _mint function that looks like this:

/**
* @dev Mints `tokenId` and transfers it to `to`.
*
* WARNING: Usage of this method is discouraged, use {_safeMint} whenever possible
*
* Requirements:
*
* - `tokenId` must not exist.
* - `to` cannot be the zero address.
*
* Emits a {Transfer} event.
*/
function _mint(address to, uint256 tokenId) internal {
if (to == address(0)) {
revert ERC721InvalidReceiver(address(0));
}
address previousOwner = _update(to, tokenId, address(0));
if (previousOwner != address(0)) {
revert ERC721InvalidSender(address(0));
}
}

Vulnerability Details

The issue arise because _mint function only check if recipient address is not address(0), but it does not check if the recepient address is able to receive NFT.
This can cause the stream depositors to stuck their deposit forever in which case they will incur loss of funds.(especially the more time passes before they discover this and if stream is not transferable).
This can happen even if recipient is not malicious(althrough it can be also exploited by malicious user) as we can see in the examples below:

Example 1 non-malicious recipient:

There is a project that is 1 year long and deposited and covered amount in stream is 10 ETH. So after 1 year recipient should be able to withdraw deposited amount (it will not be full 10 ETH because of protocol fee, but for the sake of simplicity we can put protocol fee to 0 for now). Also anytme before 1 year passes recipient could withdraw some of the amount that is deposited based on how much time has passed, for example after 6 months he could withdraw 5 ETH (half of the deposited amount).
So stream owner creates stream with recipient address that is not able to receive NFT and make stream non transferable. If none of them (stream owner or recipient) does not find this immediatelly (which is usually the case unfortunatelly) and they find out for example 6 months after. Then in that case recipient would not be able to withdraw his promised funds, the depositors can refund their remaining amount but the half of their deposited amount would be stuck forever in contract since stream is not transferable, so in that case both parties incured loss of funds.

Example 2 malicious recipient:

Imagine same project as in example 1 but the recipient gave purposely wrong address that could not receive NFT and does not have intention to complete project task that he is payed for. The malicious recipient would not benefit this monetarly but the depositors will be the one that incured loss of funds especially if they found out they are scammed as late as possible during stream duration. They could refund some of the amount before duration ends but if duration ends they they will lose full deposit and it will be stucked.

In example I gave is small to medium project but the bigger the project and stream is the bigger losses can be (possibly in the range of millions of dollars).

Impact

Users can incur loss of funds and their funds stuck in contract forever.

Tools Used

Manual Review

Recommendations

In order to prevent this I recommend to use _safeMint instead of regular _mint because it checks if recipient received the NFT and revert if receiver is invalid address.

Further more I would recommend to have recover function in SablierFlow also that is specifically designed to recover deposited amount by the depositor in case funds got stuck in the contract.

Updates

Lead Judging Commences

inallhonesty Lead Judge 8 months ago
Submission Judgement Published
Invalidated
Reason: Known issue

Support

FAQs

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