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

Logic Error in Stream Calculation within SablierV2LockupDynamic.sol

bauchibred

Summary

This report addresses a logic error in the _calculateStreamedAmountForMultipleSegments function within the SablierV2LockupDynamic smart contract. The issue pertains to the calculation of the streamed amount for multiple segments based on timestamps and amounts stored in the contract. The flaw leads to incorrect calculations of the streamed amount, affecting the functionality of the lockup mechanism and potentially causing financial discrepancies.

Vulnerability Details

The core of the vulnerability lies in the handling of timestamps and the calculation of elapsed time and segment durations. Specifically, the method calculates the elapsed time and segment duration incorrectly under certain conditions, particularly when dealing with segments that are not the first in the sequence. This miscalculation can result in an overestimation of the elapsed time percentage and subsequently an incorrect calculation of the streamed amount for the current segment.

Take a look at https://github.com/Cyfrin/2024-05-Sablier/blob/43d7e752a68bba2a1d73d3d6466c3059079ed0c6/v2-core/src/SablierV2LockupDynamic.sol#L221-L279

function _calculateStreamedAmountForMultipleSegments(uint256 streamId) internal view returns (uint128) {
unchecked {
uint40 blockTimestamp = uint40(block.timestamp);
Lockup.Stream memory stream = _streams[streamId];
LockupDynamic.Segment[] memory segments = _segments[streamId];
​
// Sum the amounts in all segments that precede the block timestamp.
uint128 previousSegmentAmounts;
uint40 currentSegmentTimestamp = segments[0].timestamp;
uint256 index = 0;
while (currentSegmentTimestamp < blockTimestamp) {
previousSegmentAmounts += segments[index].amount;
index += 1;
currentSegmentTimestamp = segments[index].timestamp;
}
​
// After exiting the loop, the current segment is at `index`.
SD59x18 currentSegmentAmount = segments[index].amount.intoSD59x18();
SD59x18 currentSegmentExponent = segments[index].exponent.intoSD59x18();
currentSegmentTimestamp = segments[index].timestamp;
​
uint40 previousTimestamp;
if (index == 0) {
//@audit correct implementation of having it set at the start timestamp
// When the current segment's index is equal to 0, the current segment is the first, so use the start
// time as the previous timestamp.
previousTimestamp = stream.startTime;
} else {
// Otherwise, when the current segment's index is greater than zero, it means that the segment is not
// the first. In this case, use the previous segment's timestamp.
previousTimestamp = segments[index - 1].timestamp;
}
​
|>@audit // Calculate how much time has passed since the segment started, and the total duration of the segment.
​
// Divide the elapsed time by the total duration of the segment.
SD59x18 elapsedTimePercentage = elapsedTime.div(segmentDuration);
​
// Calculate the streamed amount using the special formula.
SD59x18 multiplier = elapsedTimePercentage.pow(currentSegmentExponent);
SD59x18 segmentStreamedAmount = multiplier.mul(currentSegmentAmount);
​
// Although the segment streamed amount should never exceed the total segment amount, this condition is
// checked without asserting to avoid locking funds in case of a bug. If this situation occurs, the
// amount streamed in the segment is considered zero (except for past withdrawals), and the segment is
// effectively voided.
if (segmentStreamedAmount.gt(currentSegmentAmount)) {
return previousSegmentAmounts > stream.amounts.withdrawn
? previousSegmentAmounts
: stream.amounts.withdrawn;
}
​
// Calculate the total streamed amount by adding the previous segment amounts and the amount streamed in
// the current segment. Casting to uint128 is safe due to the if statement above.
return previousSegmentAmounts + uint128(segmentStreamedAmount.intoUint256());
}
}
​

The function iterates through segments until it finds one whose timestamp is less than the current block timestamp. It then calculates the elapsed time and segment duration based on these timestamps and uses them to calculate the streamed amount for the current segment.

The issue arises from the way the previousTimestamp is determined. When the current segment is not the first (index!= 0), the amount of time that has passed since the segment started would be wrong in the case, cause we use the previous segment's timestamp, and not the overall start timestamp as used when the index == 0, hinted above, essentially leading to a flawed data for the segment's duration(making it smaller than it should be) which would cause for elapsedTimePercentage to be bigger than it should be and also the segmentStreamedAmount this then causes some segments to be unfairly/wrongly voided considering the if (segmentStreamedAmount.gt(currentSegmentAmount)) check might be triggerred depending on how massive the segmentStreamedAmount gets incremented, if the if block isn't triggered this still leads to a wrong evaluation of the total streamed amount since it's gotten by adding the previous segment amounts and the amount streamed in the current segment (segmentStreamedAmount, which in our case would be flawed).

Impact

As already hinted in the Proof of Concept, this leads to multiple issues as all the calculations done in the snippet shown below would now be done with a flawed elapsedTime & segmentDuration data.

https://github.com/Cyfrin/2024-05-Sablier/blob/43d7e752a68bba2a1d73d3d6466c3059079ed0c6/v2-core/src/SablierV2LockupDynamic.sol#L254-L276

SD59x18 elapsedTime = (blockTimestamp - previousTimestamp).intoSD59x18();
SD59x18 segmentDuration = (currentSegmentTimestamp - previousTimestamp).intoSD59x18();
SD59x18 elapsedTimePercentage = elapsedTime.div(segmentDuration);
SD59x18 multiplier = elapsedTimePercentage.pow(currentSegmentExponent);
SD59x18 segmentStreamedAmount = multiplier.mul(currentSegmentAmount);
if (segmentStreamedAmount.gt(currentSegmentAmount)) {
return previousSegmentAmounts > stream.amounts.withdrawn? previousSegmentAmounts : stream.amounts.withdrawn; }
return previousSegmentAmounts + uint128(segmentStreamedAmount.intoUint256());

Tools Used

Manual review

Recommendations

The check if the index is > 1 when calculating the previousTimestamp , if true should then cache in the startTime from index == 0 to get the current duration length.

Updates

Lead Judging Commences

inallhonesty Lead Judge 12 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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