refactor: reorder checks

chore: improve writing in comments
test: bring back test in "createWithDurations"
test: close #880
test: fuzz cliff time as zero
test: improve BTs and associated modifiers

src/SablierV2LockupDynamic.sol

@@ -245,7 +245,7 @@ contract SablierV2LockupDynamic is
                 // time as the previous timestamp.
                 previousTimestamp = stream.startTime;
             } else {
-                // Otherwise, when the current segment's index is greater than 0, it implies that the segment is not
+                // Otherwise, when the current segment's index is greater than zero, it implies that the segment is not
                 // the first. In this case, use the previous segment's timestamp.
                 previousTimestamp = segments[index - 1].timestamp;
             }
@@ -339,8 +339,8 @@ contract SablierV2LockupDynamic is
             uint256 segmentCount = params.segments.length;
             stream.endTime = params.segments[segmentCount - 1].timestamp;
 
-            // Effect: store the segments. Since Solidity lacks a syntax for copying arrays directly from memory
-            // to storage, a manual approach is necessary. See https://github.com/ethereum/solidity/issues/12783.
+            // Effect: store the tranches. Since Solidity lacks a syntax for copying arrays of structs directly from
+            // memory to storage, a manual approach is necessary. See https://github.com/ethereum/solidity/issues/12783.
             for (uint256 i = 0; i < segmentCount; ++i) {
                 _segments[streamId].push(params.segments[i]);
             }

src/SablierV2LockupLinear.sol

@@ -134,15 +134,13 @@ contract SablierV2LockupLinear is
         LockupLinear.Range memory range;
         range.start = uint40(block.timestamp);
 
-        // Calculate the cliff time and the end time. It is safe to use unchecked arithmetic because
-        // {_create} will nonetheless check that the end time is greater than the cliff time,
-        // and also that the cliff time, if set, is greater than or equal to the start time.
+        // Calculate the cliff time and the end time. It is safe to use unchecked arithmetic because {_create} will
+        // nonetheless check that the end time is greater than the cliff time, and also that the cliff time, if set,
+        // is greater than or equal to the start time.
         unchecked {
-            // If the cliff duration is greater than zero, calculate the cliff time.
             if (params.durations.cliff > 0) {
                 range.cliff = range.start + params.durations.cliff;
             }
-
             range.end = range.start + params.durations.total;
         }
 
@@ -261,7 +259,7 @@ contract SablierV2LockupLinear is
             wasCanceled: false
         });
 
-        // Effect: set the cliff time if it is greater than 0.
+        // Effect: set the cliff time if it is greater than zero.
         if (params.range.cliff > 0) {
             _cliffs[streamId] = params.range.cliff;
         }

src/SablierV2LockupTranched.sol

@@ -241,8 +241,8 @@ contract SablierV2LockupTranched is
             uint256 trancheCount = params.tranches.length;
             stream.endTime = params.tranches[trancheCount - 1].timestamp;
 
-            // Effect: store the tranches. Since Solidity lacks a syntax for copying arrays directly from memory
-            // to storage, a manual approach is necessary. See https://github.com/ethereum/solidity/issues/12783.
+            // Effect: store the tranches. Since Solidity lacks a syntax for copying arrays of structs directly from
+            // memory to storage, a manual approach is necessary. See https://github.com/ethereum/solidity/issues/12783.
             for (uint256 i = 0; i < trancheCount; ++i) {
                 _tranches[streamId].push(params.tranches[i]);
             }

src/abstracts/SablierV2Lockup.sol

@@ -516,8 +516,8 @@ abstract contract SablierV2Lockup is
     /// - If `to` is 0, then the update is a burn and is also allowed.
     /// @param to The address of the new recipient of the stream.
     /// @param streamId ID of the stream to update.
-    /// @param auth Optional parameter. If the value is non-zero, the upstream implementation of this function will
-    /// check that `auth` is either the recipient of the stream, or an approved third party.
+    /// @param auth Optional parameter. If the value is not zero, the overridden implementation will check that
+    /// `auth` is either the recipient of the stream, or an approved third party.
     /// @return The original recipient of the `streamId` before the update.
     function _update(
         address to,
@@ -575,7 +575,7 @@ abstract contract SablierV2Lockup is
         // Effect: make the stream not cancelable anymore, because a stream can only be canceled once.
         _streams[streamId].isCancelable = false;
 
-        // Effect: If there are no assets left for the recipient to withdraw, mark the stream as depleted.
+        // Effect: if there are no assets left for the recipient to withdraw, mark the stream as depleted.
         if (recipientAmount == 0) {
             _streams[streamId].isDepleted = true;
         }

src/libraries/Helpers.sol

@@ -92,12 +92,7 @@ library Helpers {
             revert Errors.SablierV2Lockup_StartTimeZero();
         }
 
-        // Check: the start time is strictly less than the end time.
-        if (range.start >= range.end) {
-            revert Errors.SablierV2LockupLinear_StartTimeNotLessThanEndTime(range.start, range.end);
-        }
-
-        // A cliff time of zero means there is no cliff, so we only perform the following checks if it is not zero.
+        // Since a cliff time of zero means there is no cliff, the following checks are performed only if it's not zero.
         if (range.cliff > 0) {
             // Check: the start time is strictly less than the cliff time.
             if (range.start >= range.cliff) {
@@ -110,6 +105,11 @@ library Helpers {
             }
         }
 
+        // Check: the start time is strictly less than the end time.
+        if (range.start >= range.end) {
+            revert Errors.SablierV2LockupLinear_StartTimeNotLessThanEndTime(range.start, range.end);
+        }
+
         // Check: the end time is in the future.
         uint40 currentTime = uint40(block.timestamp);
         if (currentTime >= range.end) {

test/fork/LockupLinear.t.sol

@@ -2,6 +2,7 @@
 pragma solidity >=0.8.22 <0.9.0;
 
 import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
+import { Math } from "@openzeppelin/contracts/utils/math/Math.sol";
 import { ud } from "@prb/math/src/UD60x18.sol";
 import { Solarray } from "solarray/src/Solarray.sol";
 
@@ -51,11 +52,14 @@ abstract contract LockupLinear_Fork_Test is Fork_Test {
         uint256 actualRecipientBalance;
         Lockup.Status actualStatus;
         uint256[] balances;
+        uint40 currentTime;
+        uint40 endTimeLowerBound;
         uint256 expectedLockupLinearBalance;
         uint256 expectedHolderBalance;
         address expectedNFTOwner;
         uint256 expectedRecipientBalance;
         Lockup.Status expectedStatus;
+        bool hasCliff;
         uint256 initialLockupLinearBalance;
         uint256 initialRecipientBalance;
         bool isDepleted;
@@ -98,30 +102,37 @@ abstract contract LockupLinear_Fork_Test is Fork_Test {
     ///
     /// - Multiple values for the sender, recipient, and broker
     /// - Multiple values for the total amount
-    /// - Multiple values for the cliff time and the end time
-    /// - Multiple values for the broker fee, including zero
     /// - Multiple values for the withdraw amount, including zero
     /// - Start time in the past
     /// - Start time in the present
     /// - Start time in the future
-    /// - Start time lower than and equal to cliff time
+    /// - Multiple values for the cliff time and the end time
+    /// - Cliff time zero and not zero
+    /// - Multiple values for the broker fee, including zero
     /// - The whole gamut of stream statuses
     function testForkFuzz_LockupLinear_CreateWithdrawCancel(Params memory params) external {
         checkUsers(params.sender, params.recipient, params.broker.account, address(lockupLinear));
 
         // Bound the parameters.
-        uint40 currentTime = getBlockTimestamp();
+        Vars memory vars;
+        vars.currentTime = getBlockTimestamp();
         params.broker.fee = _bound(params.broker.fee, 0, MAX_BROKER_FEE);
-        params.range.start = boundUint40(params.range.start, currentTime - 1000 seconds, currentTime + 10_000 seconds);
-        params.range.cliff =
-            boundUint40(params.range.cliff, params.range.start + 1 seconds, params.range.start + 52 weeks);
+        params.range.start =
+            boundUint40(params.range.start, vars.currentTime - 1000 seconds, vars.currentTime + 10_000 seconds);
         params.totalAmount = boundUint128(params.totalAmount, 1, uint128(initialHolderBalance));
 
-        // Bound the end time so that it is always greater than both the current time and the cliff time (this is
-        // a requirement of the protocol).
+        // The cliff time must be either zero or greater than the start time.
+        vars.hasCliff = params.range.cliff > 0;
+        if (vars.hasCliff) {
+            params.range.cliff =
+                boundUint40(params.range.cliff, params.range.start + 1 seconds, params.range.start + 52 weeks);
+        }
+        // Bound the end time so that it is always greater than both the current time and the cliff time (as this is
+        // a protocol requirement).
+        vars.endTimeLowerBound = maxUint40(params.range.start, params.range.cliff);
         params.range.end = boundUint40(
             params.range.end,
-            (params.range.cliff <= currentTime ? currentTime : params.range.cliff) + 1,
+            (vars.endTimeLowerBound <= vars.currentTime ? vars.currentTime : vars.endTimeLowerBound) + 1 seconds,
             MAX_UNIX_TIMESTAMP
         );
 
@@ -132,8 +143,6 @@ abstract contract LockupLinear_Fork_Test is Fork_Test {
                                             CREATE
         //////////////////////////////////////////////////////////////////////////*/
 
-        Vars memory vars;
-
         // Load the pre-create asset balances.
         vars.balances =
             getTokenBalances(address(ASSET), Solarray.addresses(address(lockupLinear), params.broker.account));
@@ -185,16 +194,16 @@ abstract contract LockupLinear_Fork_Test is Fork_Test {
         assertEq(actualStream.endTime, params.range.end, "endTime");
         assertEq(actualStream.isCancelable, true, "isCancelable");
         assertEq(actualStream.isDepleted, false, "isDepleted");
-        assertEq(actualStream.isTransferable, true, "isTransferable");
         assertEq(actualStream.isStream, true, "isStream");
+        assertEq(actualStream.isTransferable, true, "isTransferable");
         assertEq(actualStream.recipient, params.recipient, "recipient");
         assertEq(actualStream.sender, params.sender, "sender");
         assertEq(actualStream.startTime, params.range.start, "startTime");
         assertEq(actualStream.wasCanceled, false, "wasCanceled");
 
         // Assert that the stream's status is correct.
         vars.actualStatus = lockupLinear.statusOf(vars.streamId);
-        vars.expectedStatus = params.range.start > currentTime ? Lockup.Status.PENDING : Lockup.Status.STREAMING;
+        vars.expectedStatus = params.range.start > vars.currentTime ? Lockup.Status.PENDING : Lockup.Status.STREAMING;
         assertEq(vars.actualStatus, vars.expectedStatus, "post-create stream status");
 
         // Assert that the next stream ID has been bumped.
@@ -231,7 +240,11 @@ abstract contract LockupLinear_Fork_Test is Fork_Test {
         //////////////////////////////////////////////////////////////////////////*/
 
         // Simulate the passage of time.
-        params.warpTimestamp = boundUint40(params.warpTimestamp, params.range.cliff, params.range.end + 100 seconds);
+        params.warpTimestamp = boundUint40(
+            params.warpTimestamp,
+            vars.hasCliff ? params.range.cliff : params.range.start + 1 seconds,
+            params.range.end + 100 seconds
+        );
         vm.warp({ newTimestamp: params.warpTimestamp });
 
         // Bound the withdraw amount.

test/integration/concrete/lockup-linear/create-with-durations/createWithDurations.t.sol

@@ -28,6 +28,28 @@ contract CreateWithDurations_LockupLinear_Integration_Concrete_Test is
         expectRevertDueToDelegateCall(success, returnData);
     }
 
+    function test_RevertWhen_CliffDurationCalculationOverflows() external whenNotDelegateCalled {
+        uint40 startTime = getBlockTimestamp();
+        uint40 cliffDuration = MAX_UINT40 - startTime + 2 seconds;
+        uint40 totalDuration = defaults.TOTAL_DURATION();
+
+        // Calculate the end time. Needs to be "unchecked" to avoid an overflow.
+        uint40 cliffTime;
+        unchecked {
+            cliffTime = startTime + cliffDuration;
+        }
+
+        // Expect the relevant error to be thrown.
+        vm.expectRevert(
+            abi.encodeWithSelector(
+                Errors.SablierV2LockupLinear_StartTimeNotLessThanCliffTime.selector, startTime, cliffTime
+            )
+        );
+
+        // Create the stream.
+        createDefaultStreamWithDurations(LockupLinear.Durations({ cliff: cliffDuration, total: totalDuration }));
+    }
+
     function test_RevertWhen_TotalDurationCalculationOverflows()
         external
         whenNotDelegateCalled

test/integration/concrete/lockup-linear/create-with-durations/createWithDurations.tree

@@ -2,11 +2,14 @@ createWithDurations.t.sol
 ├── when delegate called
 │  └── it should revert
 └── when not delegate called
+   ├── when the cliff duration calculation overflows uint256
+   │  └── it should revert due to the start time being greater than the cliff time
+   └── when the cliff duration calculation does not overflow uint256
       ├── when the total duration calculation overflows uint256
-      │   └── it should revert
+      │  └── it should revert
       └── when the total duration calculation does not overflow uint256
-          ├── it should create the stream
-          ├── it should bump the next stream ID
-          ├── it should mint the NFT
-          ├── it should perform the ERC-20 transfers
-          └── it should emit a {CreateLockupLinearStream} event
+         ├── it should create the stream
+         ├── it should bump the next stream ID
+         ├── it should mint the NFT
+         ├── it should perform the ERC-20 transfers
+         └── it should emit a {CreateLockupLinearStream} event

test/integration/concrete/lockup-linear/create-with-timestamps/createWithTimestamps.t.sol

@@ -59,11 +59,7 @@ contract CreateWithTimestamps_LockupLinear_Integration_Concrete_Test is
         createDefaultStreamWithRange(LockupLinear.Range({ start: 0, cliff: cliffTime, end: endTime }));
     }
 
-    modifier whenCliffTimeZero() {
-        _;
-    }
-
-    function test_RevertWhen_StartTimeGreaterThanEndTime()
+    function test_RevertWhen_StartTimeNotLessThanEndTime()
         external
         whenNotDelegateCalled
         whenRecipientNonZeroAddress
@@ -82,7 +78,7 @@ contract CreateWithTimestamps_LockupLinear_Integration_Concrete_Test is
         createDefaultStreamWithRange(LockupLinear.Range({ start: startTime, cliff: 0, end: endTime }));
     }
 
-    function test_CreateWithTimestamps_CliffTimeZero()
+    function test_CreateWithTimestamps_StartTimeLessThanEndTime()
         external
         whenNotDelegateCalled
         whenRecipientNonZeroAddress
@@ -111,17 +107,14 @@ contract CreateWithTimestamps_LockupLinear_Integration_Concrete_Test is
         assertEq(actualNFTOwner, expectedNFTOwner, "NFT owner");
     }
 
-    modifier whenCliffTimeGreaterThanZero() {
-        _;
-    }
-
     function test_RevertWhen_StartTimeGreaterThanCliffTime()
         external
         whenNotDelegateCalled
         whenRecipientNonZeroAddress
         whenDepositAmountNotZero
         whenStartTimeNotZero
         whenCliffTimeGreaterThanZero
+        whenStartTimeLessThanEndTime
     {
         uint40 startTime = defaults.CLIFF_TIME();
         uint40 cliffTime = defaults.START_TIME();
@@ -141,7 +134,7 @@ contract CreateWithTimestamps_LockupLinear_Integration_Concrete_Test is
         whenDepositAmountNotZero
         whenStartTimeNotZero
         whenCliffTimeGreaterThanZero
-        whenStartTimeNotGreaterThanCliffTime
+        whenStartTimeLessThanEndTime
     {
         uint40 startTime = defaults.START_TIME();
         uint40 cliffTime = defaults.END_TIME();
@@ -161,7 +154,7 @@ contract CreateWithTimestamps_LockupLinear_Integration_Concrete_Test is
         whenDepositAmountNotZero
         whenStartTimeNotZero
         whenCliffTimeGreaterThanZero
-        whenStartTimeNotGreaterThanCliffTime
+        whenStartTimeLessThanEndTime
         whenCliffTimeLessThanEndTime
         whenEndTimeInTheFuture
     {
@@ -178,7 +171,7 @@ contract CreateWithTimestamps_LockupLinear_Integration_Concrete_Test is
         whenDepositAmountNotZero
         whenStartTimeNotZero
         whenCliffTimeGreaterThanZero
-        whenStartTimeNotGreaterThanCliffTime
+        whenStartTimeLessThanEndTime
         whenCliffTimeLessThanEndTime
         whenEndTimeInTheFuture
     {
@@ -196,7 +189,7 @@ contract CreateWithTimestamps_LockupLinear_Integration_Concrete_Test is
         whenDepositAmountNotZero
         whenStartTimeNotZero
         whenCliffTimeGreaterThanZero
-        whenStartTimeNotGreaterThanCliffTime
+        whenStartTimeLessThanEndTime
         whenCliffTimeLessThanEndTime
         whenEndTimeInTheFuture
         whenBrokerFeeNotTooHigh
@@ -213,7 +206,7 @@ contract CreateWithTimestamps_LockupLinear_Integration_Concrete_Test is
         whenDepositAmountNotZero
         whenStartTimeNotZero
         whenCliffTimeGreaterThanZero
-        whenStartTimeNotGreaterThanCliffTime
+        whenStartTimeLessThanEndTime
         whenCliffTimeLessThanEndTime
         whenEndTimeInTheFuture
         whenBrokerFeeNotTooHigh
@@ -228,7 +221,7 @@ contract CreateWithTimestamps_LockupLinear_Integration_Concrete_Test is
         whenDepositAmountNotZero
         whenStartTimeNotZero
         whenCliffTimeGreaterThanZero
-        whenStartTimeNotGreaterThanCliffTime
+        whenStartTimeLessThanEndTime
         whenCliffTimeLessThanEndTime
         whenEndTimeInTheFuture
         whenBrokerFeeNotTooHigh

test/integration/concrete/lockup-linear/create-with-timestamps/createWithTimestamps.tree

@@ -12,15 +12,15 @@ createWithTimestamps.t.sol
          │  └── it should revert
          └── when the start time is not zero
             ├── when the cliff time is zero
-            │  ├── when the start time is greater than the end time
+            │  ├── when the start time is not less than the end time
             │  │  └── it should revert
-            │  └── when the start time is not greater than the end time
+            │  └── when the start time is less than the end time
             │     └── it should create the stream
             └── when the cliff time is greater than zero
                ├── when the start time is not less than the cliff time
                │  └── it should revert
-               └── when the start time is not greater than the cliff time
-                  ├── when the cliff time is less than the end time
+               └── when the start time is less than the cliff time
+                  ├── when the cliff time is not less than the end time
                   │  └── it should revert
                   └── when the cliff time is less than the end time
                      ├── when the end time is not in the future