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

In `try/catch` block `catch` never executaed as Tx flow always result in `revert` when `recipient contract` doesn't have any `onLockupStreamCanceled()` implementation

0xhacksmithh

Summary

If recipient of a Stream is a contract & that contract doesn't implement any onLockupStreamCanceled(), so there is no sence for try/catch block as transaction flow never goes inside catch its always result is revert

Vulnerability Details

try/catch block used in multiple part of the code, i'm show casing only one below and later section i will mention other infected parts

function _cancel(uint256 streamId) internal {
// Calculate the streamed amount.
...
...
uint128 streamedAmount = _calculateStreamedAmount(streamId);
if (recipient.code.length > 0) {
try ISablierV2Recipient(recipient).onLockupStreamCanceled({
streamId: streamId,
sender: sender,
senderAmount: senderAmount,
recipientAmount: recipientAmount
}) { } catch { }
}

https://github.com/Cyfrin/2024-05-Sablier/blob/main/v2-core/src/abstracts/SablierV2Lockup.sol#L609-L615

The above code segment is from SablierV2LockUp._cancel()
Here if recipient of a Stream is a contract which doesn't implement onLockupStreamCanceled() then Tx will revert and catch block wouldn't helpfull anymore.
This thing is severe problamatic here as in Code base cancel() used to Cancels the stream and refunds any remaining assets to the sender

Due to above reason cancel() lead to an DoS (Direct loss of Sender & there is no profit for recipient as well explaining below)

Other Infected Codes Part

withdraw() has below code segment

// Effects and Interactions: make the withdrawal.
_withdraw(streamId, to, amount);
​
if (msg.sender != recipient && recipient.code.length > 0) {
try ISablierV2Recipient(recipient).onLockupStreamWithdrawn({
streamId: streamId,
caller: msg.sender,
to: to,
amount: amount
}) { } catch { }
}
​
if (msg.sender != sender && sender.code.length > 0 && sender != recipient) {
try ISablierV2Sender(sender).onLockupStreamWithdrawn({
streamId: streamId,
caller: msg.sender,
to: to,
amount: amount
}) { } catch { }
}

https://github.com/Cyfrin/2024-05-Sablier/blob/main/v2-core/src/abstracts/SablierV2Lockup.sol#L382-L402
Here

  • If Sender intentionally didn't implement onLockupStreamWithdrawn() then all user withdrawable fund get locked.

  • If recepient didn't implement onLockupStreamWithdrawn() all of its fund get locked.

renounce() also affected due to this type of code styling

address recipient = _ownerOf(streamId);
if (recipient.code.length > 0) {
try ISablierV2Recipient(recipient).onLockupStreamRenounced(streamId) { } catch { }
}

https://github.com/Cyfrin/2024-05-Sablier/blob/main/v2-core/src/abstracts/SablierV2Lockup.sol#L309-L311

Impact

Lead to DoS

Tools Used

Manual review

Recommendations

Maybe use a wrapper contract, which is trusted for both parties and is internally calling to those functions (which are intended to be implemented) on the untrusted target.

This issue was previously found in Sherlock's taller-protocol-v2-audit contest,
This is how they mitigate this.

Updates

Lead Judging Commences

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