Address audit findings 3, 5, and 11 (#1131)

* refactor: add blockTimestamp param in VestingMath functions refactor: remove unchecked in calculateStreamedPercentage docs: add assumptions in VestingMath function docs: improve explanatory comments * docs: fix typos * docs: polish natspec * refactor: move logic in VestingMath chore: remove MAX_COUNT assumption * docs: polish assumptions * refactor: use unchecked and casting in calculateStreamedPercentage docs: last polishes --------- Co-authored-by: Paul Razvan Berg <[email protected]>
sablier-labs · Jan 7, 2025 · 974f93a · 974f93a
1 parent b70c831
commit 974f93a
Show file tree

Hide file tree

Showing 11 changed files with 95 additions and 44 deletions.
diff --git a/benchmark/LockupDynamic.Gas.t.sol b/benchmark/LockupDynamic.Gas.t.sol
@@ -38,7 +38,7 @@ contract Lockup_Dynamic_Gas_Test is Benchmark_Test {
         vm.writeFile({
             path: benchmarkResultsFile,
             data: string.concat(
-                "# Benchmarks for Lockup Dynamic Model\n\n", "| Implementation | Gas Usage |\n", "| --- | --- |\n"
+                "# Benchmarks for the Lockup Dynamic model\n\n", "| Implementation | Gas Usage |\n", "| --- | --- |\n"
             )
         });
 

diff --git a/benchmark/LockupLinear.Gas.t.sol b/benchmark/LockupLinear.Gas.t.sol
@@ -22,7 +22,7 @@ contract Lockup_Linear_Gas_Test is Benchmark_Test {
         vm.writeFile({
             path: benchmarkResultsFile,
             data: string.concat(
-                "# Benchmarks for Lockup Linear Model\n\n", "| Implementation | Gas Usage |\n", "| --- | --- |\n"
+                "# Benchmarks for the Lockup Linear model\n\n", "| Implementation | Gas Usage |\n", "| --- | --- |\n"
             )
         });
 

diff --git a/benchmark/LockupTranched.Gas.t.sol b/benchmark/LockupTranched.Gas.t.sol
@@ -29,7 +29,7 @@ contract Lockup_Tranched_Gas_Test is Benchmark_Test {
         vm.writeFile({
             path: benchmarkResultsFile,
             data: string.concat(
-                "# Benchmarks for Lockup Tranched Model\n\n", "| Implementation | Gas Usage |\n", "| --- | --- |\n"
+                "# Benchmarks for the Lockup Tranched model\n\n", "| Implementation | Gas Usage |\n", "| --- | --- |\n"
             )
         });
 

diff --git a/benchmark/results/SablierLockup_Dynamic.md b/benchmark/results/SablierLockup_Dynamic.md
@@ -1,4 +1,4 @@
-# Benchmarks for Lockup Dynamic Model
+# Benchmarks for the Lockup Dynamic model
 
 | Implementation                                               | Gas Usage |
 | ------------------------------------------------------------ | --------- |

diff --git a/benchmark/results/SablierLockup_Linear.md b/benchmark/results/SablierLockup_Linear.md
@@ -1,4 +1,4 @@
-# Benchmarks for Lockup Linear Model
+# Benchmarks for the Lockup Linear model
 
 | Implementation                                                | Gas Usage |
 | ------------------------------------------------------------- | --------- |

diff --git a/benchmark/results/SablierLockup_Tranched.md b/benchmark/results/SablierLockup_Tranched.md
@@ -1,4 +1,4 @@
-# Benchmarks for Lockup Tranched Model
+# Benchmarks for the Lockup Tranched model
 
 | Implementation                                               | Gas Usage |
 | ------------------------------------------------------------ | --------- |

diff --git a/src/LockupNFTDescriptor.sol b/src/LockupNFTDescriptor.sol
@@ -200,7 +200,7 @@ contract LockupNFTDescriptor is ILockupNFTDescriptor {
     {
         // This cannot overflow because both inputs are uint128s, and zero deposit amounts are not allowed in Sablier.
         unchecked {
-            return streamedAmount * 10_000 / depositedAmount;
+            return uint256(streamedAmount) * 10_000 / depositedAmount;
         }
     }
 

diff --git a/src/SablierLockup.sol b/src/SablierLockup.sol
@@ -184,7 +184,7 @@ contract SablierLockup is ISablierLockup, SablierLockupBase {
 
         // 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 or equal to the start time.
+        // is greater than the start time.
         unchecked {
             if (durations.cliff > 0) {
                 cliffTime = timestamps.start + durations.cliff;
@@ -297,45 +297,43 @@ contract SablierLockup is ISablierLockup, SablierLockupBase {
 
     /// @inheritdoc SablierLockupBase
     function _calculateStreamedAmount(uint256 streamId) internal view override returns (uint128) {
-        Lockup.Timestamps memory timestamps =
-            Lockup.Timestamps({ start: _streams[streamId].startTime, end: _streams[streamId].endTime });
-
-        // If the start time is in the future, return zero.
+        // Load in memory the parameters used in {VestingMath}.
         uint40 blockTimestamp = uint40(block.timestamp);
-        if (timestamps.start >= blockTimestamp) {
-            return 0;
-        }
-
-        // If the end time is not in the future, return the deposited amount.
         uint128 depositedAmount = _streams[streamId].amounts.deposited;
-        if (timestamps.end <= blockTimestamp) {
-            return depositedAmount;
-        }
-
-        uint128 streamedAmount;
         Lockup.Model lockupModel = _streams[streamId].lockupModel;
+        uint128 streamedAmount;
+        Lockup.Timestamps memory timestamps =
+            Lockup.Timestamps({ start: _streams[streamId].startTime, end: _streams[streamId].endTime });
 
-        // Calculate streamed amount for Lockup Dynamic model.
+        // Calculate the streamed amount for the Lockup Dynamic model.
         if (lockupModel == Lockup.Model.LOCKUP_DYNAMIC) {
             streamedAmount = VestingMath.calculateLockupDynamicStreamedAmount({
+                depositedAmount: depositedAmount,
                 segments: _segments[streamId],
-                startTime: timestamps.start,
+                blockTimestamp: blockTimestamp,
+                timestamps: timestamps,
                 withdrawnAmount: _streams[streamId].amounts.withdrawn
             });
         }
-        // Calculate streamed amount for Lockup Linear model.
+        // Calculate the streamed amount for the Lockup Linear model.
         else if (lockupModel == Lockup.Model.LOCKUP_LINEAR) {
             streamedAmount = VestingMath.calculateLockupLinearStreamedAmount({
                 depositedAmount: depositedAmount,
+                blockTimestamp: blockTimestamp,
                 timestamps: timestamps,
                 cliffTime: _cliffs[streamId],
                 unlockAmounts: _unlockAmounts[streamId],
                 withdrawnAmount: _streams[streamId].amounts.withdrawn
             });
         }
-        // Calculate streamed amount for Lockup Tranched model.
+        // Calculate the streamed amount for the Lockup Tranched model.
         else if (lockupModel == Lockup.Model.LOCKUP_TRANCHED) {
-            streamedAmount = VestingMath.calculateLockupTranchedStreamedAmount({ tranches: _tranches[streamId] });
+            streamedAmount = VestingMath.calculateLockupTranchedStreamedAmount({
+                depositedAmount: depositedAmount,
+                blockTimestamp: blockTimestamp,
+                timestamps: timestamps,
+                tranches: _tranches[streamId]
+            });
         }
 
         return streamedAmount;
@@ -471,16 +469,16 @@ contract SablierLockup is ISablierLockup, SablierLockupBase {
         // Load the stream ID in a variable.
         streamId = nextStreamId;
 
-        // Effect: set the start unlock amount if its non-zero.
+        // Effect: set the start unlock amount if it is non-zero.
         if (unlockAmounts.start > 0) {
             _unlockAmounts[streamId].start = unlockAmounts.start;
         }
 
-        // Effect: update cliff time if its non-zero.
+        // Effect: update cliff time if it is non-zero.
         if (cliffTime > 0) {
             _cliffs[streamId] = cliffTime;
 
-            // Effect: set the cliff unlock amount if its non-zero.
+            // Effect: set the cliff unlock amount if it is non-zero.
             if (unlockAmounts.cliff > 0) {
                 _unlockAmounts[streamId].cliff = unlockAmounts.cliff;
             }

diff --git a/src/interfaces/ISablierLockup.sol b/src/interfaces/ISablierLockup.sol
@@ -164,6 +164,7 @@ interface ISablierLockup is ISablierLockupBase {
     /// - `params.recipient` must not be the zero address.
     /// - `params.sender` must not be the zero address.
     /// - `msg.sender` must have allowed this contract to spend at least `params.totalAmount` tokens.
+    /// - `params.shape.length` must not be greater than 32 characters.
     ///
     /// @param params Struct encapsulating the function parameters, which are documented in {DataTypes}.
     /// @param segments Segments used to compose the dynamic distribution function.
@@ -198,6 +199,7 @@ interface ISablierLockup is ISablierLockupBase {
     /// deposit amount.
     /// - If `params.timestamps.cliff` not set, the `params.unlockAmounts.cliff` must be zero.
     /// - `msg.sender` must have allowed this contract to spend at least `params.totalAmount` tokens.
+    /// - `params.shape.length` must not be greater than 32 characters.
     ///
     /// @param params Struct encapsulating the function parameters, which are documented in {DataTypes}.
     /// @param cliffTime The Unix timestamp for the cliff period's end. A value of zero means there is no cliff.
@@ -234,6 +236,7 @@ interface ISablierLockup is ISablierLockupBase {
     /// - `params.recipient` must not be the zero address.
     /// - `params.sender` must not be the zero address.
     /// - `msg.sender` must have allowed this contract to spend at least `params.totalAmount` tokens.
+    /// - `params.shape.length` must not be greater than 32 characters.
     ///
     /// @param params Struct encapsulating the function parameters, which are documented in {DataTypes}.
     /// @param tranches Tranches used to compose the tranched distribution function.

diff --git a/src/interfaces/ISablierLockupBase.sol b/src/interfaces/ISablierLockupBase.sol
@@ -246,7 +246,7 @@ interface ISablierLockupBase is
     /// Notes:
     /// - If there any tokens left for the recipient to withdraw, the stream is marked as canceled. Otherwise, the
     /// stream is marked as depleted.
-    /// - This function attempts to invoke a hook on the recipient, if the resolved address is a contract.
+    /// - If the address is on the allowlist, this function will invoke a hook on the recipient.
     ///
     /// Requirements:
     /// - Must not be delegate called.
@@ -274,7 +274,7 @@ interface ISablierLockupBase is
     /// @dev Emits a {CollectFees} event.
     ///
     /// Notes:
-    /// - If the admin is a contract, it must be able to receive ETH.
+    /// - If the admin is a contract, it must be able to receive native token payments, e.g., ETH for Ethereum Mainnet.
     function collectFees() external;
 
     /// @notice Removes the right of the stream's sender to cancel the stream.
@@ -324,7 +324,8 @@ interface ISablierLockupBase is
     /// @dev Emits a {Transfer}, {WithdrawFromLockupStream}, and {MetadataUpdate} event.
     ///
     /// Notes:
-    /// - This function attempts to call a hook on the recipient of the stream, unless `msg.sender` is the recipient.
+    /// - If `msg.sender` is not the recipient and the address is on the allowlist, this function will invoke a hook on
+    /// the recipient.
     ///
     /// Requirements:
     /// - Must not be delegate called.

diff --git a/src/libraries/VestingMath.sol b/src/libraries/VestingMath.sol
@@ -33,18 +33,33 @@ library VestingMath {
     /// 2. The stream's start time must be in the past so that the calculations below do not overflow.
     /// 3. The stream's end time must be in the future so that the loop below does not panic with an "index out of
     /// bounds" error.
+    ///
+    /// Assumptions:
+    /// 1. The sum of all segment amounts does not overflow uint128.
+    /// 2. The segment timestamps are ordered chronologically.
+    /// 3. There are no duplicate segment timestamps.
     function calculateLockupDynamicStreamedAmount(
+        uint128 depositedAmount,
         LockupDynamic.Segment[] memory segments,
-        uint40 startTime,
+        uint40 blockTimestamp,
+        Lockup.Timestamps memory timestamps,
         uint128 withdrawnAmount
     )
         public
-        view
+        pure
         returns (uint128)
     {
-        unchecked {
-            uint40 blockTimestamp = uint40(block.timestamp);
+        // If the start time is in the future, return zero.
+        if (timestamps.start > blockTimestamp) {
+            return 0;
+        }
 
+        // If the end time is not in the future, return the deposited amount.
+        if (timestamps.end <= blockTimestamp) {
+            return depositedAmount;
+        }
+
+        unchecked {
             // Sum the amounts in all segments that precede the block timestamp.
             uint128 previousSegmentAmounts;
             uint40 currentSegmentTimestamp = segments[0].timestamp;
@@ -64,7 +79,7 @@ library VestingMath {
             if (index == 0) {
                 // 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 = startTime;
+                previousTimestamp = timestamps.start;
             } 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.
@@ -100,7 +115,9 @@ library VestingMath {
     /// @dev Lockup linear model uses the following distribution function:
     ///
     /// $$
-    /// f(x) = x * sa + s + c
+    ///        ( x * sa + s, block timestamp < cliff time
+    /// f(x) = (
+    ///        ( x * sa + s + c, block timestamp => cliff time
     /// $$
     ///
     /// Where:
@@ -109,18 +126,32 @@ library VestingMath {
     /// - $sa$ is the streamable amount, i.e. deposited amount minus unlock amounts' sum.
     /// - $s$ is the start unlock amount.
     /// - $c$ is the cliff unlock amount.
+    ///
+    /// Assumptions:
+    /// 1. The sum of the unlock amounts (start and cliff) does not overflow uint128.
+    /// 2. The start time is before the end time, and the block timestamp is between the start and end times.
+    /// 3. If the cliff time is not zero, it is after the start time and before the end time.
     function calculateLockupLinearStreamedAmount(
         uint128 depositedAmount,
+        uint40 blockTimestamp,
         Lockup.Timestamps memory timestamps,
         uint40 cliffTime,
         LockupLinear.UnlockAmounts memory unlockAmounts,
         uint128 withdrawnAmount
     )
         public
-        view
+        pure
         returns (uint128)
     {
-        uint256 blockTimestamp = block.timestamp;
+        // If the start time is in the future, return zero.
+        if (timestamps.start > blockTimestamp) {
+            return 0;
+        }
+
+        // If the end time is not in the future, return the deposited amount.
+        if (timestamps.end <= blockTimestamp) {
+            return depositedAmount;
+        }
 
         // If the cliff time is in the future, return the start unlock amount.
         if (cliffTime > blockTimestamp) {
@@ -177,12 +208,30 @@ library VestingMath {
     /// Where:
     ///
     /// - $\Sigma(eta)$ is the sum of all vested tranches' amounts.
-    function calculateLockupTranchedStreamedAmount(LockupTranched.Tranche[] memory tranches)
+    ///
+    /// Assumptions:
+    /// 1. The sum of all tranche amounts does not overflow uint128.
+    /// 2. The tranche timestamps are ordered chronologically.
+    /// 3. There are no duplicate tranche timestamps.
+    function calculateLockupTranchedStreamedAmount(
+        uint128 depositedAmount,
+        uint40 blockTimestamp,
+        Lockup.Timestamps memory timestamps,
+        LockupTranched.Tranche[] memory tranches
+    )
         public
-        view
+        pure
         returns (uint128)
     {
-        uint256 blockTimestamp = block.timestamp;
+        // If the start time is in the future, return zero.
+        if (timestamps.start > blockTimestamp) {
+            return 0;
+        }
+
+        // If the end time is not in the future, return the deposited amount.
+        if (timestamps.end <= blockTimestamp) {
+            return depositedAmount;
+        }
 
         // If the first tranche's timestamp is in the future, return zero.
         if (tranches[0].timestamp > blockTimestamp) {