Sablier

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

Missing pause modifier on withdraw function degrades security

Summary

The withdraw function does not have a pause modifier that would be critical in emergency situations

Vulnerability Details

The developers of the Sablier protocol have taken all measures to ensure that the codebase is bullet-proof. This is commendable and very evident. However, it seems that they have not taken into consideration external factors that also affect the security of Dapps. Of particular interest, is Solidity compiler bugs. While the code might be impenetrateable, a nasty bug in the Solidity compiler can suddenly expose all user funds currently held in the protocol. In July 2023, a vulnerability was discovered in the Vyper compiler that allowed attackers to steal an estimated $70 Million from different projects including Curve and Alchemix. The root cause was a failing lock in the re-entrancy feature. If such a bug is found in the Solidity compiler, then all user funds will be at risk, leaving Sablier beneficiaries at the mercy of blackhats.

The Sablier developers take pride in providing a censorship-resistant application. Adding such a functionality will not make it any less censorship-resistant since it is a security measure. The withdraw function in question is presented here:

/// @inheritdoc ISablierV2Lockup
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 { }
}
}

Impact

All user funds currently held in the protocol can be drained

Tools Used

Manual review

Recommendations

Add a pause modifier to the withdraw function to be used by admin at emergency times.

Updates

Lead Judging Commences

inallhonesty Lead Judge over 1 year ago
Submission Judgement Published
Invalidated
Reason: Too generic

Support

FAQs

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