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

Address audit findings 3, 5, and 11 #1131

Merged
merged 6 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
2 changes: 1 addition & 1 deletion benchmark/LockupDynamic.Gas.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
});

Expand Down
2 changes: 1 addition & 1 deletion benchmark/LockupLinear.Gas.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
});

Expand Down
2 changes: 1 addition & 1 deletion benchmark/LockupTranched.Gas.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
});

Expand Down
2 changes: 1 addition & 1 deletion benchmark/results/SablierLockup_Dynamic.md
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
# Benchmarks for Lockup Dynamic Model
# Benchmarks for the Lockup Dynamic model

| Implementation | Gas Usage |
| ------------------------------------------------------------ | --------- |
Expand Down
2 changes: 1 addition & 1 deletion benchmark/results/SablierLockup_Linear.md
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
# Benchmarks for Lockup Linear Model
# Benchmarks for the Lockup Linear model

| Implementation | Gas Usage |
| ------------------------------------------------------------- | --------- |
Expand Down
2 changes: 1 addition & 1 deletion benchmark/results/SablierLockup_Tranched.md
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
# Benchmarks for Lockup Tranched Model
# Benchmarks for the Lockup Tranched model

| Implementation | Gas Usage |
| ------------------------------------------------------------ | --------- |
Expand Down
2 changes: 1 addition & 1 deletion src/LockupNFTDescriptor.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}

Expand Down
44 changes: 21 additions & 23 deletions src/SablierLockup.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}
Expand Down
3 changes: 3 additions & 0 deletions src/interfaces/ISablierLockup.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand Down
7 changes: 4 additions & 3 deletions src/interfaces/ISablierLockupBase.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand Down
71 changes: 60 additions & 11 deletions src/libraries/VestingMath.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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.
Expand Down Expand Up @@ -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:
Expand All @@ -109,18 +126,32 @@ library VestingMath {
/// - $sa$ is the streamable amount, i.e. deposited amount minus unlock amounts' sum.
PaulRBerg marked this conversation as resolved.
Show resolved Hide resolved
/// - $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) {
Expand Down Expand Up @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This if block is redundant. We already have the following logic in this function:

        if (tranches[0].timestamp > blockTimestamp) {
            return 0;
        }

Copy link
Member Author

@andreivladbrg andreivladbrg Jan 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That’s correct—in our case, this check can be removed. However, I left it intentionally because if someone calls this function with an incorrect set of tranches but the correct start time, it would return 0.

Our example for abort functionality

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"start time" does not exist in the context of tranched streams. So, one solution could be to only pass timestamps.end as the input parameter but then I suppose, for the API consistencies, you decided to pass timestamps struct, which is good choice.

How about that, instead of reverting on invalid timestamps.start value, we add it as an assumption i.e. this function does not check for timestamps.start value?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {
Expand Down
Loading