Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from
Jan 7, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .gitignore
andreivladbrg marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -1,14 +1,18 @@
# directories
artifacts
artifacts-zk
broadcast
cache
cache_hardhat-zk
PaulRBerg marked this conversation as resolved.
Show resolved Hide resolved
coverage
deployments
deployments-zk
docs
node_modules
out
out-optimized
out-svg
typechain-types

# files
*.env
Expand Down
26 changes: 14 additions & 12 deletions src/SablierLockup.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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);
smol-ninja marked this conversation as resolved.
Show resolved Hide resolved

// 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(
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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);
smol-ninja marked this conversation as resolved.
Show resolved Hide resolved

// 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(
Expand Down
3 changes: 0 additions & 3 deletions src/abstracts/SablierLockupBase.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
12 changes: 6 additions & 6 deletions src/interfaces/ISablierLockup.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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.
Expand All @@ -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.
Expand All @@ -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
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand Down
18 changes: 9 additions & 9 deletions src/interfaces/ISablierLockupBase.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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
Expand All @@ -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}.
Expand All @@ -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.
Expand All @@ -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}.
Expand All @@ -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
Expand All @@ -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}.
Expand All @@ -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.
Expand All @@ -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:
Expand Down
20 changes: 10 additions & 10 deletions src/libraries/Helpers.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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 {
Expand Down
Original file line number Diff line number Diff line change
@@ -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";
Expand Down Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Original file line number Diff line number Diff line change
@@ -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";

Expand Down Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand All @@ -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);
}
Expand Down
Loading
Loading