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

If the recipient doesn't implement the relevant hooks for onLockupStreamCanceled and onLockupStreamWithdrawn, try/catch will fail to catch the error, leading to tx reversion.

golanger85

Summary

The try/catch blocks in the SablierV2Lockup contract may fail to handle scenarios where the target contract does not implement the onLockupStreamCanceled(), onLockupStreamWithdrawn(), and onLockupStreamRenounced() hooks.
This can cause the transaction to revert unexpectedly, leading to a cascading failure in the contract.

Vulnerability Details

The use of try/catch blocks in the SablierV2Lockup contract is intended to gracefully handle errors when calling external contracts. However, these blocks only catch errors that occur within the target contract's execution. If the target contract does not implement the expected hooks, such as onLockupStreamCanceled(), onLockupStreamWithdrawn(), and onLockupStreamRenounced(), the transaction will revert due to decoding errors rather than being caught by the catch block. This can lead to unexpected crashes in the contract.

The sender initiates the cancellation of a stream by calling the cancel function.
The function begins execution and performs initial checks to ensure the stream is neither depleted
nor already canceled and verifies that the sender is authorized to cancel the stream.

After marking the stream as canceled and transferring the funds, the cancel function attempts to call
the onLockupStreamCanceled() hook on the recipient contract.
If the recipient contract does not implement the hook, the transaction will revert due to decoding errors,
causing the entire cancellation process to fail.

function cancel(uint256 streamId) public override noDelegateCall notNull(streamId) {
// Check: the stream is neither depleted nor canceled.
if (_streams[streamId].isDepleted) {
revert Errors.SablierV2Lockup_StreamDepleted(streamId);
} else if (_streams[streamId].wasCanceled) {
revert Errors.SablierV2Lockup_StreamCanceled(streamId);
}
​
// Check: `msg.sender` is the stream's sender.
if (!_isCallerStreamSender(streamId)) {
revert Errors.SablierV2Lockup_Unauthorized(streamId, msg.sender);
}
​
// Checks, Effects and Interactions: cancel the stream.
_cancel(streamId);
}

_cancel Function:

function _cancel(uint256 streamId) internal {
// Calculate the streamed amount.
uint128 streamedAmount = _calculateStreamedAmount(streamId);
​
// Retrieve the amounts from storage.
Lockup.Amounts memory amounts = _streams[streamId].amounts;
​
// Check: the stream is not settled.
if (streamedAmount >= amounts.deposited) {
revert Errors.SablierV2Lockup_StreamSettled(streamId);
}
​
// Check: the stream is cancelable.
if (!_streams[streamId].isCancelable) {
revert Errors.SablierV2Lockup_StreamNotCancelable(streamId);
}
​
// Calculate the sender's amount.
uint128 senderAmount;
unchecked {
senderAmount = amounts.deposited - streamedAmount;
}
​
// Calculate the recipient's amount.
uint128 recipientAmount = streamedAmount - amounts.withdrawn;
​
// Effect: mark the stream as canceled.
_streams[streamId].wasCanceled = true;
​
// Effect: make the stream not cancelable anymore, because a stream can only be canceled once.
_streams[streamId].isCancelable = false;
​
// Effect: if there are no assets left for the recipient to withdraw, mark the stream as depleted.
if (recipientAmount == 0) {
_streams[streamId].isDepleted = true;
}
​
// Effect: set the refunded amount.
_streams[streamId].amounts.refunded = senderAmount;
​
// Retrieve the sender and the recipient from storage.
address sender = _streams[streamId].sender;
address recipient = _ownerOf(streamId);
​
// Retrieve the ERC-20 asset from storage.
IERC20 asset = _streams[streamId].asset;
​
// Interaction: refund the sender.
asset.safeTransfer({ to: sender, value: senderAmount });
​
// Log the cancellation.
emit ISablierV2Lockup.CancelLockupStream(streamId, sender, recipient, asset, senderAmount, recipientAmount);
​
// Emits an ERC-4906 event to trigger an update of the NFT metadata.
emit MetadataUpdate({ _tokenId: streamId });
​
// Interaction: if the recipient is a contract, try to invoke the cancel hook on the recipient without
// reverting if the hook is not implemented, and without bubbling up any potential revert.
if (recipient.code.length > 0) {
try ISablierV2Recipient(recipient).onLockupStreamCanceled({
streamId: streamId,
sender: sender,
senderAmount: senderAmount,
recipientAmount: recipientAmount
}) { } catch { }
}
}

Impact

If the recipient contract does not implement the expected hooks, the try/catch block will fail to catch the error, leading to a crash of the entire transaction and causing a cascading failure upon the rest of the onLockupStreamCanceled() logic.
(and onLockupStreamWithdrawn())

Tools Used

Manual Review/Solidity Docs

Recommendations

This thread explains the solutions in more detail regarding circumventing try/catch blocks.

https://ethereum.stackexchange.com/questions/129150/solidity-try-catch-call-to-external-non-existent-address-method
https://docs.soliditylang.org/en/v0.8.14/control-structures.html?highlight=try catch#try-catch

Implement a check for the presence of contract code before attempting to call the hooks.
This can be done using extcodesize(assembly) or use a wrapper contract which is internally calling the onLockupStreamCanceled
on the untrusted target.

Updates

Lead Judging Commences

inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement
golanger85 Submitter
about 1 year ago
golanger85 Submitter
about 1 year ago
inallhonesty Lead Judge
about 1 year ago
golanger85 Submitter
about 1 year ago
golanger85 Submitter
about 1 year ago
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.