Sablier

Summary

The _calculateStreamedAmountForMultipleSegments function has an issue that could lead to an "index out of bounds" error. This issue exists becaus of the way the function iterates over the segments array without properly checking the boundaries, potentially accessing an index that does not exist in the array.

Vulnerability Details

Look at this part of the code:

File: SablierV2LockupDynamic.sol

In this loop, index is incremented without ensuring it remains within the bounds of the segments array. If blockTimestamp is beyond the last segment's timestamp, the loop will attempt to access segments[index] where index equals the length of the array, leading to an "index out of bounds" error.

As per the Natspec,

Albeit, the implementation does not enforce this requirement, allowing for scenarios where blockTimestamp exceeds the last segment's timestamp, contradicting the NatSpec's guidance.

Detailed Explanation:

Index Out of Bounds

The function iterates over the segments array using a while loop. The loop condition is while (currentSegmentTimestamp < blockTimestamp), and within the loop, the index is incremented. However, there is no check to ensure that index does not exceed the length of the segments array. If blockTimestamp is beyond the last segment's timestamp, the loop will continue to increment index until it tries to access an element outside the bounds of the array, causing an "index out of bounds" error.

Now the loop condition while (currentSegmentTimestamp < blockTimestamp) does not account for the possibility that blockTimestamp might be beyond the last segment's timestamp. This can lead to an out-of-bounds access when index exceeds the length of the segments array. Additionally, the loop does not handle the case where blockTimestamp is exactly equal to the last segment's timestamp.

Walkthrough:

1. Initialization:
   https://github.com/Cyfrin/2024-05-Sablier/blob/43d7e752a68bba2a1d73d3d6466c3059079ed0c6/v2-core/src/SablierV2LockupDynamic.sol#L229-L235

   uint40 currentSegmentTimestamp = segments[0].timestamp;

   uint256 index = 0;

   • currentSegmentTimestamp is set to the timestamp of the first segment.

   • index is initialized to 0.

2. Loop Execution:
   https://github.com/Cyfrin/2024-05-Sablier/blob/43d7e752a68bba2a1d73d3d6466c3059079ed0c6/v2-core/src/SablierV2LockupDynamic.sol#L231-L234

   while (currentSegmentTimestamp < blockTimestamp) {

   previousSegmentAmounts += segments[index].amount;

   index += 1;

   currentSegmentTimestamp = segments[index].timestamp;

   }

   • The loop continues as long as currentSegmentTimestamp is less than blockTimestamp.

   • Inside the loop, index is incremented, and currentSegmentTimestamp is updated to the timestamp of the next segment.

If blockTimestamp is beyond the last segment's timestamp, index will eventually exceed the length of the segments array, leading to an out-of-bounds access when currentSegmentTimestamp = segments[index].timestamp is executed.

Let us consder a scenario:

• Given segments with timestamps [100, 200, 300] and block.timestamp is 400:

  • Initial values: currentSegmentTimestamp = 100, index = 0.

  • First iteration: index = 1, currentSegmentTimestamp = 200.

  • Second iteration: index = 2, currentSegmentTimestamp = 300.

  • Third iteration: index = 3, which is out of bounds for the segments array.

Impact

If exploited, this vulnerability could cause the contract to revert unexpectedly during execution of the _calculateStreamedAmountForMultipleSegments function. This could lead to inccorrect calculation of the streamed amount for a stream with multiple segments, potentially affecting the functionality of the entire contract.

Tools Used

Manual review

Recommended Mitigation Steps:

Explicitly ensure that the stream's endtime is in the future. i.e (Before entering the loop, add a check to ensure that the stream's end time is later than the current block timestamp)
Also modify the loop condition to also check that index is within the bounds of the segments array. This will prevent attempts to access an index that does not exist. And lastly, when updating currentSegmentTimestamp, ensure that index is still within the bounds of the segments array to avoid out-of-bounds access.