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

Overridden functions `_calculateStreamedAmount` fails to implement safety mechanism for non-cancellable steams.

n0kto

Description

_calculateStreamedAmount is an overridden function that returns the amount already streamed to the recipient. This function includes a safety mechanism to prevent a specific bug where the streamedAmount could exceed the depositedAmount, in which case it returns the withdrawn amount. This mechanism allows the sender to cancel the stream and retrieve all the stuck funds if such a bug occurs. However, if the stream is non-cancellable due to any of the following reasons, this mechanism will not function:

  • isCancellable = false was set at creation.

  • The sender address used the renounce function.

  • The sender is not a valid sender (it could be set to anything at the creation if the stream creator does not plan to cancel the stream).

`SablierV2LockupDynamic.sol` vulnerable function
function _calculateStreamedAmount(uint256 streamId) internal view override returns (uint128) {
...
if (_segments[streamId].length > 1) {
// If there is more than one segment, it may be required to iterate over all of them.
return _calculateStreamedAmountForMultipleSegments(streamId);
} else {
// Otherwise, there is only one segment, and the calculation is simpler.
@> return _calculateStreamedAmountForOneSegment(streamId);
}
}
​
function _calculateStreamedAmountForOneSegment(uint256 streamId) internal view returns (uint128) {
unchecked {
...
​
if (streamedAmount.gt(depositedAmount)) {
@> return _streams[streamId].amounts.withdrawn;
}
​
// Cast the streamed amount to uint128. This is safe due to the check above.
return uint128(streamedAmount.intoUint256());
}
}
`SablierV2LockupLinear.sol` vulnerable function
function _calculateStreamedAmount(uint256 streamId) internal view override returns (uint128) {
...
unchecked {
...
if (streamedAmount.gt(depositedAmount)) {
@> return _streams[streamId].amounts.withdrawn;
}
​
// Cast the streamed amount to uint128. This is safe due to the check above.
return uint128(streamedAmount.intoUint256());
}
}

Risk

Even though this safety mechanism is not intended to be used, since it exists for a purpose and it fails to fulfill it, I consider this finding to be at least of Low severity.

Likelyhood: Low

  • The safety mechanism does not work if the stream is non-cancellable.

Impact: High

  • The safety mechanism will not prevent funds from being stuck in the contract.

Recommended Mitigation

Return the deposited amount instead of the withdrawn amount to ensure that the recipient can retrieve the funds under any circumstances. These changes will not break any parent functions that call them.

`SablierV2LockupDynamic.sol`
function _calculateStreamedAmountForOneSegment(uint256 streamId) internal view returns (uint128) {
unchecked {
...
​
if (streamedAmount.gt(depositedAmount)) {
- return _streams[streamId].amounts.withdrawn;
+ return _streams[streamId].amounts.deposited;
}
​
// Cast the streamed amount to uint128. This is safe due to the check above.
return uint128(streamedAmount.intoUint256());
}
}
`SablierV2LockupLinear.sol`
function _calculateStreamedAmount(uint256 streamId) internal view override returns (uint128) {
...
unchecked {
...
if (streamedAmount.gt(depositedAmount)) {
- return _streams[streamId].amounts.withdrawn;
+ return _streams[streamId].amounts.deposited;
}
​
// Cast the streamed amount to uint128. This is safe due to the check above.
return uint128(streamedAmount.intoUint256());
}
}
Updates

Lead Judging Commences

inallhonesty Lead Judge over 1 year ago
Submission Judgement Published
Invalidated
Reason: Design choice

Support

FAQs

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