fix: address audit findings 4, 8, 10 (#1132)

* refactor: add startTime as input argument in Helpers functions

* refactor: drop unchecked from cliff and end calculations

* fix: remove MetadataUpdate event from renounce functions
docs: add MetadataUpdate event in create functions natspecs

* docs: polish comments

Co-authored-by: Paul Razvan Berg <prberg@proton.me>

---------

Co-authored-by: Paul Razvan Berg <prberg@proton.me>

.gitignore

@@ -1,14 +1,18 @@
 # directories
 artifacts
+artifacts-zk
 broadcast
 cache
+cache_hardhat-zk
 coverage
 deployments
+deployments-zk
 docs
 node_modules
 out
 out-optimized
 out-svg
+typechain-types
 
 # files
 *.env

src/SablierLockup.sol

@@ -141,12 +141,15 @@ contract SablierLockup is ISablierLockup, SablierLockupBase {
         noDelegateCall
         returns (uint256 streamId)
     {
+        // Use the block timestamp as the start time.
+        uint40 startTime = uint40(block.timestamp);
+
         // Generate the canonical segments.
-        LockupDynamic.Segment[] memory segments = Helpers.calculateSegmentTimestamps(segmentsWithDuration);
+        LockupDynamic.Segment[] memory segments = Helpers.calculateSegmentTimestamps(segmentsWithDuration, startTime);
 
         // Declare the timestamps for the stream.
         Lockup.Timestamps memory timestamps =
-            Lockup.Timestamps({ start: uint40(block.timestamp), end: segments[segments.length - 1].timestamp });
+            Lockup.Timestamps({ start: startTime, end: segments[segments.length - 1].timestamp });
 
         // Checks, Effects and Interactions: create the stream.
         streamId = _createLD(
@@ -182,15 +185,11 @@ contract SablierLockup is ISablierLockup, SablierLockupBase {
 
         uint40 cliffTime;
 
-        // Calculate the cliff time and the end time. It is safe to use unchecked arithmetic because {_createLL} will
-        // nonetheless check that the end time is greater than the cliff time, and also that the cliff time, if set,
-        // is greater than the start time.
-        unchecked {
-            if (durations.cliff > 0) {
-                cliffTime = timestamps.start + durations.cliff;
-            }
-            timestamps.end = timestamps.start + durations.total;
+        // Calculate the cliff time and the end time.
+        if (durations.cliff > 0) {
+            cliffTime = timestamps.start + durations.cliff;
         }
+        timestamps.end = timestamps.start + durations.total;
 
         // Checks, Effects and Interactions: create the stream.
         streamId = _createLL(
@@ -221,12 +220,15 @@ contract SablierLockup is ISablierLockup, SablierLockupBase {
         noDelegateCall
         returns (uint256 streamId)
     {
+        // Use the block timestamp as the start time.
+        uint40 startTime = uint40(block.timestamp);
+
         // Generate the canonical tranches.
-        LockupTranched.Tranche[] memory tranches = Helpers.calculateTrancheTimestamps(tranchesWithDuration);
+        LockupTranched.Tranche[] memory tranches = Helpers.calculateTrancheTimestamps(tranchesWithDuration, startTime);
 
         // Declare the timestamps for the stream.
         Lockup.Timestamps memory timestamps =
-            Lockup.Timestamps({ start: uint40(block.timestamp), end: tranches[tranches.length - 1].timestamp });
+            Lockup.Timestamps({ start: startTime, end: tranches[tranches.length - 1].timestamp });
 
         // Checks, Effects and Interactions: create the stream.
         streamId = _createLT(

src/abstracts/SablierLockupBase.sol

@@ -357,9 +357,6 @@ abstract contract SablierLockupBase is
 
         // Log the renouncement.
         emit ISablierLockupBase.RenounceLockupStream(streamId);
-
-        // Emit an ERC-4906 event to trigger an update of the NFT metadata.
-        emit MetadataUpdate({ _tokenId: streamId });
     }
 
     /// @inheritdoc ISablierLockupBase

src/interfaces/ISablierLockup.sol

@@ -82,7 +82,7 @@ interface ISablierLockup is ISablierLockupBase {
     /// `block.timestamp` and all specified time durations. The segment timestamps are derived from these
     /// durations. The stream is funded by `msg.sender` and is wrapped in an ERC-721 NFT.
     ///
-    /// @dev Emits a {Transfer} and {CreateLockupDynamicStream} event.
+    /// @dev Emits a {Transfer}, {CreateLockupDynamicStream} and {MetadataUpdate} event.
     ///
     /// Requirements:
     /// - All requirements in {createWithTimestampsLD} must be met for the calculated parameters.
@@ -103,7 +103,7 @@ interface ISablierLockup is ISablierLockupBase {
     /// the sum of `block.timestamp` and `durations.total`. The stream is funded by `msg.sender` and is wrapped in an
     /// ERC-721 NFT.
     ///
-    /// @dev Emits a {Transfer} and {CreateLockupLinearStream} event.
+    /// @dev Emits a {Transfer}, {CreateLockupLinearStream} and {MetadataUpdate} event.
     ///
     /// Requirements:
     /// - All requirements in {createWithTimestampsLL} must be met for the calculated parameters.
@@ -126,7 +126,7 @@ interface ISablierLockup is ISablierLockupBase {
     /// `block.timestamp` and all specified time durations. The tranche timestamps are derived from these
     /// durations. The stream is funded by `msg.sender` and is wrapped in an ERC-721 NFT.
     ///
-    /// @dev Emits a {Transfer} and {CreateLockupTrancheStream} event.
+    /// @dev Emits a {Transfer}, {CreateLockupTrancheStream} and {MetadataUpdate} event.
     ///
     /// Requirements:
     /// - All requirements in {createWithTimestampsLT} must be met for the calculated parameters.
@@ -146,7 +146,7 @@ interface ISablierLockup is ISablierLockupBase {
     /// @notice Creates a stream with the provided segment timestamps, implying the end time from the last timestamp.
     /// The stream is funded by `msg.sender` and is wrapped in an ERC-721 NFT.
     ///
-    /// @dev Emits a {Transfer} and {CreateLockupDynamicStream} event.
+    /// @dev Emits a {Transfer}, {CreateLockupDynamicStream} and {MetadataUpdate} event.
     ///
     /// Notes:
     /// - As long as the segment timestamps are arranged in ascending order, it is not an error for some
@@ -180,7 +180,7 @@ interface ISablierLockup is ISablierLockupBase {
     /// @notice Creates a stream with the provided start time and end time. The stream is funded by `msg.sender` and is
     /// wrapped in an ERC-721 NFT.
     ///
-    /// @dev Emits a {Transfer} and {CreateLockupLinearStream} event.
+    /// @dev Emits a {Transfer}, {CreateLockupLinearStream} and {MetadataUpdate} event.
     ///
     /// Notes:
     /// - A cliff time of zero means there is no cliff.
@@ -218,7 +218,7 @@ interface ISablierLockup is ISablierLockupBase {
     /// @notice Creates a stream with the provided tranche timestamps, implying the end time from the last timestamp.
     /// The stream is funded by `msg.sender` and is wrapped in an ERC-721 NFT.
     ///
-    /// @dev Emits a {Transfer} and {CreateLockupTrancheStream} event.
+    /// @dev Emits a {Transfer}, {CreateLockupTrancheStream} and {MetadataUpdate} event.
     ///
     /// Notes:
     /// - As long as the tranche timestamps are arranged in ascending order, it is not an error for some

src/interfaces/ISablierLockupBase.sol

@@ -228,7 +228,7 @@ interface ISablierLockupBase is
 
     /// @notice Burns the NFT associated with the stream.
     ///
-    /// @dev Emits a {Transfer} event.
+    /// @dev Emits a {Transfer} and {MetadataUpdate} event.
     ///
     /// Requirements:
     /// - Must not be delegate called.
@@ -241,7 +241,7 @@ interface ISablierLockupBase is
 
     /// @notice Cancels the stream and refunds any remaining tokens to the sender.
     ///
-    /// @dev Emits a {Transfer}, {CancelLockupStream}, and {MetadataUpdate} event.
+    /// @dev Emits a {Transfer}, {CancelLockupStream} and {MetadataUpdate} event.
     ///
     /// Notes:
     /// - If there any tokens left for the recipient to withdraw, the stream is marked as canceled. Otherwise, the
@@ -258,7 +258,7 @@ interface ISablierLockupBase is
 
     /// @notice Cancels multiple streams and refunds any remaining tokens to the sender.
     ///
-    /// @dev Emits multiple {Transfer}, {CancelLockupStream}, and {MetadataUpdate} events.
+    /// @dev Emits multiple {Transfer}, {CancelLockupStream} and {MetadataUpdate} events.
     ///
     /// Notes:
     /// - Refer to the notes in {cancel}.
@@ -279,7 +279,7 @@ interface ISablierLockupBase is
 
     /// @notice Removes the right of the stream's sender to cancel the stream.
     ///
-    /// @dev Emits a {RenounceLockupStream} and {MetadataUpdate} event.
+    /// @dev Emits a {RenounceLockupStream} event.
     ///
     /// Notes:
     /// - This is an irreversible operation.
@@ -295,7 +295,7 @@ interface ISablierLockupBase is
 
     /// @notice Renounces multiple streams.
     ///
-    /// @dev Emits multiple {RenounceLockupStream} and {MetadataUpdate} events.
+    /// @dev Emits multiple {RenounceLockupStream} events.
     ///
     /// Notes:
     /// - Refer to the notes in {renounce}.
@@ -321,7 +321,7 @@ interface ISablierLockupBase is
 
     /// @notice Withdraws the provided amount of tokens from the stream to the `to` address.
     ///
-    /// @dev Emits a {Transfer}, {WithdrawFromLockupStream}, and {MetadataUpdate} event.
+    /// @dev Emits a {Transfer}, {WithdrawFromLockupStream} and {MetadataUpdate} event.
     ///
     /// Notes:
     /// - If `msg.sender` is not the recipient and the address is on the allowlist, this function will invoke a hook on
@@ -341,7 +341,7 @@ interface ISablierLockupBase is
 
     /// @notice Withdraws the maximum withdrawable amount from the stream to the provided address `to`.
     ///
-    /// @dev Emits a {Transfer}, {WithdrawFromLockupStream}, and {MetadataUpdate} event.
+    /// @dev Emits a {Transfer}, {WithdrawFromLockupStream} and {MetadataUpdate} event.
     ///
     /// Notes:
     /// - Refer to the notes in {withdraw}.
@@ -357,7 +357,7 @@ interface ISablierLockupBase is
     /// @notice Withdraws the maximum withdrawable amount from the stream to the current recipient, and transfers the
     /// NFT to `newRecipient`.
     ///
-    /// @dev Emits a {WithdrawFromLockupStream} and a {Transfer} event.
+    /// @dev Emits a {WithdrawFromLockupStream}, {Transfer} and {MetadataUpdate} event.
     ///
     /// Notes:
     /// - If the withdrawable amount is zero, the withdrawal is skipped.
@@ -381,7 +381,7 @@ interface ISablierLockupBase is
 
     /// @notice Withdraws tokens from streams to the recipient of each stream.
     ///
-    /// @dev Emits multiple {Transfer}, {WithdrawFromLockupStream}, and {MetadataUpdate} events. For each stream that
+    /// @dev Emits multiple {Transfer}, {WithdrawFromLockupStream} and {MetadataUpdate} events. For each stream that
     /// reverted the withdrawal, it emits an {InvalidWithdrawalInWithdrawMultiple} event.
     ///
     /// Notes:

src/libraries/Helpers.sol

@@ -13,17 +13,17 @@ library Helpers {
     //////////////////////////////////////////////////////////////////////////*/
 
     /// @dev Calculate the timestamps and return the segments.
-    function calculateSegmentTimestamps(LockupDynamic.SegmentWithDuration[] memory segmentsWithDuration)
+    function calculateSegmentTimestamps(
+        LockupDynamic.SegmentWithDuration[] memory segmentsWithDuration,
+        uint40 startTime
+    )
         public
-        view
+        pure
         returns (LockupDynamic.Segment[] memory segmentsWithTimestamps)
     {
         uint256 segmentCount = segmentsWithDuration.length;
         segmentsWithTimestamps = new LockupDynamic.Segment[](segmentCount);
 
-        // Make the block timestamp the stream's start time.
-        uint40 startTime = uint40(block.timestamp);
-
         // It is safe to use unchecked arithmetic because {SablierLockup._createLD} will nonetheless
         // check the correctness of the calculated segment timestamps.
         unchecked {
@@ -46,17 +46,17 @@ library Helpers {
     }
 
     /// @dev Calculate the timestamps and return the tranches.
-    function calculateTrancheTimestamps(LockupTranched.TrancheWithDuration[] memory tranchesWithDuration)
+    function calculateTrancheTimestamps(
+        LockupTranched.TrancheWithDuration[] memory tranchesWithDuration,
+        uint40 startTime
+    )
         public
-        view
+        pure
         returns (LockupTranched.Tranche[] memory tranchesWithTimestamps)
     {
         uint256 trancheCount = tranchesWithDuration.length;
         tranchesWithTimestamps = new LockupTranched.Tranche[](trancheCount);
 
-        // Make the block timestamp the stream's start time.
-        uint40 startTime = uint40(block.timestamp);
-
         // It is safe to use unchecked arithmetic because {SablierLockup-_createLT} will nonetheless check the
         // correctness of the calculated tranche timestamps.
         unchecked {

tests/integration/concrete/lockup-base/renounce-multiple/renounceMultiple.t.sol

@@ -1,7 +1,6 @@
 // SPDX-License-Identifier: UNLICENSED
 pragma solidity >=0.8.22 <0.9.0;
 
-import { IERC4906 } from "@openzeppelin/contracts/interfaces/IERC4906.sol";
 import { Solarray } from "solarray/src/Solarray.sol";
 
 import { ISablierLockupBase } from "src/interfaces/ISablierLockupBase.sol";
@@ -90,15 +89,11 @@ contract RenounceMultiple_Integration_Concrete_Test is Integration_Test {
         givenNoColdStreams
         whenCallerAuthorizedForAllStreams
     {
-        // It should emit {MetadataUpdate} and {RenounceLockupStream} events for both the streams.
+        // It should emit {RenounceLockupStream} events for both streams.
         vm.expectEmit({ emitter: address(lockup) });
         emit ISablierLockupBase.RenounceLockupStream(streamIds[0]);
         vm.expectEmit({ emitter: address(lockup) });
-        emit IERC4906.MetadataUpdate({ _tokenId: streamIds[0] });
-        vm.expectEmit({ emitter: address(lockup) });
         emit ISablierLockupBase.RenounceLockupStream(streamIds[1]);
-        vm.expectEmit({ emitter: address(lockup) });
-        emit IERC4906.MetadataUpdate({ _tokenId: streamIds[1] });
 
         // Renounce the streams.
         lockup.renounceMultiple(streamIds);

tests/integration/concrete/lockup-base/renounce-multiple/renounceMultiple.tree

@@ -17,5 +17,5 @@ RenounceMultiple_Integration_Concrete_Test
                ├── given at least one non cancelable stream
                │  └── it should revert
                └── given all streams cancelable
-                  ├── it should emit {MetadataUpdate} and {RenounceLockupStream} events
+                  ├── it should emit {RenounceLockupStream} events
                   └── it should make streams non cancelable

tests/integration/concrete/lockup-base/renounce/renounce.t.sol

@@ -1,8 +1,6 @@
 // SPDX-License-Identifier: UNLICENSED
 pragma solidity >=0.8.22 <0.9.0;
 
-import { IERC4906 } from "@openzeppelin/contracts/interfaces/IERC4906.sol";
-
 import { ISablierLockupBase } from "src/interfaces/ISablierLockupBase.sol";
 import { Errors } from "src/libraries/Errors.sol";
 
@@ -66,11 +64,9 @@ abstract contract Renounce_Integration_Concrete_Test is Integration_Test {
         givenWarmStreamRenounce
         whenCallerSender
     {
-        // It should emit {MetadataUpdate} and {RenounceLockupStream} events.
+        // It should emit {RenounceLockupStream} event.
         vm.expectEmit({ emitter: address(lockup) });
         emit ISablierLockupBase.RenounceLockupStream(streamId);
-        vm.expectEmit({ emitter: address(lockup) });
-        emit IERC4906.MetadataUpdate({ _tokenId: streamId });
 
         // Renounce the stream.
         lockup.renounce(streamId);

tests/integration/concrete/lockup-base/renounce/renounce.tree

@@ -19,5 +19,5 @@ Renounce_Integration_Concrete_Test
             ├── given non cancelable stream
             │  └── it should revert
             └── given cancelable stream
-               ├── it should emit {MetadataUpdate} and {RenounceLockupStream} events
+               ├── it should emit {RenounceLockupStream} event
                └── it should make stream non cancelable

tests/integration/concrete/lockup-linear/create-with-durations-ll/createWithDurationsLL.t.sol

@@ -4,7 +4,6 @@ pragma solidity >=0.8.22 <0.9.0;
 import { IERC4906 } from "@openzeppelin/contracts/interfaces/IERC4906.sol";
 
 import { ISablierLockup } from "src/interfaces/ISablierLockup.sol";
-import { Errors } from "src/libraries/Errors.sol";
 import { Lockup, LockupLinear } from "src/types/DataTypes.sol";
 
 import { Lockup_Linear_Integration_Concrete_Test } from "../LockupLinear.t.sol";
@@ -19,47 +18,11 @@ contract CreateWithDurationsLL_Integration_Concrete_Test is Lockup_Linear_Integr
         });
     }
 
-    function test_RevertWhen_CliffTimeCalculationOverflows() external whenNoDelegateCall whenCliffDurationNotZero {
-        uint40 startTime = getBlockTimestamp();
-        _defaultParams.durations.cliff = MAX_UINT40 - startTime + 2 seconds;
-
-        // Calculate the end time. Needs to be "unchecked" to avoid an overflow.
-        uint40 cliffTime;
-        unchecked {
-            cliffTime = startTime + _defaultParams.durations.cliff;
-        }
-
-        // It should revert.
-        vm.expectRevert(
-            abi.encodeWithSelector(Errors.SablierHelpers_StartTimeNotLessThanCliffTime.selector, startTime, cliffTime)
-        );
-        createDefaultStreamWithDurations();
-    }
-
-    function test_WhenCliffTimeCalculationNotOverflow() external whenNoDelegateCall whenCliffDurationNotZero {
+    function test_WhenCliffDurationNotZero() external whenNoDelegateCall {
         _test_CreateWithDurations(_defaultParams.durations);
     }
 
-    function test_RevertWhen_EndTimeCalculationOverflows() external whenNoDelegateCall whenCliffDurationZero {
-        uint40 startTime = getBlockTimestamp();
-        _defaultParams.durations = LockupLinear.Durations({ cliff: 0, total: MAX_UINT40 - startTime + 1 seconds });
-        _defaultParams.unlockAmounts.cliff = 0;
-
-        // Calculate the end time. Needs to be "unchecked" to allow an overflow.
-        uint40 endTime;
-        unchecked {
-            endTime = startTime + _defaultParams.durations.total;
-        }
-
-        // It should revert.
-        vm.expectRevert(
-            abi.encodeWithSelector(Errors.SablierHelpers_StartTimeNotLessThanEndTime.selector, startTime, endTime)
-        );
-
-        createDefaultStreamWithDurations();
-    }
-
-    function test_WhenEndTimeCalculationNotOverflow() external whenNoDelegateCall whenCliffDurationZero {
+    function test_WhenCliffDurationZero() external whenNoDelegateCall {
         _defaultParams.durations.cliff = 0;
         _test_CreateWithDurations(_defaultParams.durations);
     }