Sablier

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

Lack of security Mechanism.

Summary

Lack of security Mechanism.

Vulnerability Details

In the comment it says:
comment "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"
So there is a chance the amounts.withdrawn will be more than amounts.deposited due to a hack for example.

The developers considered the possibility that amounts.withdrawn would be greater than amounts.deposited, but did not consider the possibility
that the excess amount was coming from amount parameter because the only place where amounts.withdrawn is increased is in _withdraw,
which is done through _streams[streamId].amounts.withdrawn = _streams[streamId].amounts.withdrawn + amount;
The amount is the reason for the increase in amounts.withdrawn, and in this case funds will be taken from other users, because amount passed
directly into transferFrom without checking amount value in case of amounts.withdrawn > amounts.deposited
and this will cause the loss of users’ funds.

Lets take this scenario:

  1. funder deposited 1000 DAI to recipient X so the amounts.deposited is 1000 DAI.

  2. The last withdraw: amounts.withdrawn = amounts.withdrawn + amount
    Eg: amounts.withdrawn(900) + amount(300) -> 1200 now the withdrawn is more than deposited amount.

  3. then if statement is executed and set isDepleted.

  4. now recipient will get 200 extra tokens, these 200 tokens is taken from the contract which is another recipient funds.

  5. If the last recipient deposited 200 his funds will be lost.

NOTE:
since if statement execute only at the last withdrawn the amounts.withdrawn will be ">=" when we add amount
because amounts.withdrawn increase only in _withdraw so for amounts.withdrawn to be ">=" amount should be more
than it should be in this case if (amounts.withdrawn > amounts.deposited - amounts.refunded) -> true
and if there is no mechanism to correct the amount it will send tokens to recipient more than it should be.
Amount of tokens could be 1000 as it could be millions.

And since the protocol is non-upgradeable such mistake could make the protocol lose millions.

Impact

Lack of security mechanism could lead protocol to lose millions better safe than sorry.

Tools Used

Manual

Recommendations

Since if statement execute only at the last withdraw add a Mechanism to send to the recipient only to proper amount of tokens, add this inside if statement.

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;
// execute only if withdrawn is > (deposit - refunded)
+ if (amounts.withdrawn > amounts.deposited - amounts.refunded) {
+ uint128 remain = amounts.deposited - amounts.refunded;
+ amount = remain - (amounts.withdrawn - amount);
+ }
}
Updates

Lead Judging Commences

inallhonesty Lead Judge over 1 year ago
Submission Judgement Published
Invalidated
Reason: Too generic
0xgenaudits Submitter
over 1 year ago
0xgenaudits Submitter
over 1 year ago
inallhonesty Lead Judge
about 1 year ago
0xgenaudits Submitter
about 1 year ago
inallhonesty Lead Judge about 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.