Sablier
Sablier
DeFiFoundry
53,440 USDC
View results
Previous Next
Submission Details
Severity: low
Invalid

`SablierV2LockupLinear._calculateStreamedAmount` function executes division before multiplication

rbserver

Summary

Because the SablierV2LockupLinear._calculateStreamedAmount function executes division before multiplication, when a stream is cancelled, the recipient of the stream can receive less than the recipient is entitled to, and the sender of the stream can receive more than the sender is entitled to.

Vulnerability Details

When block.timestamp is after _cliffs[streamId] and before _streams[streamId].endTime, calling the following SablierV2LockupLinear._calculateStreamedAmount function would executes UD60x18 elapsedTimePercentage = elapsedTime.div(totalDuration) before UD60x18 streamedAmount = elapsedTimePercentage.mul(depositedAmount). Because division is executed before multiplication, streamedAmount can be smaller than what it should be.

https://github.com/Cyfrin/2024-05-Sablier/blob/1fe69e059dcbd71728bdfaa58dad85ff199ee6dc/v2-core/src/SablierV2LockupLinear.sol#L189-L230

function _calculateStreamedAmount(uint256 streamId) internal view override returns (uint128) {
// If the cliff time is in the future, return zero.
uint256 cliffTime = uint256(_cliffs[streamId]);
uint256 blockTimestamp = block.timestamp;
if (cliffTime > blockTimestamp) {
return 0;
}
​
// If the end time is not in the future, return the deposited amount.
uint256 endTime = uint256(_streams[streamId].endTime);
if (blockTimestamp >= endTime) {
return _streams[streamId].amounts.deposited;
}
​
// In all other cases, calculate the amount streamed so far. Normalization to 18 decimals is not needed
// because there is no mix of amounts with different decimals.
unchecked {
// Calculate how much time has passed since the stream started, and the stream's total duration.
uint256 startTime = uint256(_streams[streamId].startTime);
UD60x18 elapsedTime = ud(blockTimestamp - startTime);
UD60x18 totalDuration = ud(endTime - startTime);
​
// Divide the elapsed time by the stream's total duration.
UD60x18 elapsedTimePercentage = elapsedTime.div(totalDuration);
​
// Cast the deposited amount to UD60x18.
UD60x18 depositedAmount = ud(_streams[streamId].amounts.deposited);
​
// Calculate the streamed amount by multiplying the elapsed time percentage by the deposited amount.
UD60x18 streamedAmount = elapsedTimePercentage.mul(depositedAmount);
​
// Although the streamed amount should never exceed the deposited amount, this condition is checked
// without asserting to avoid locking funds in case of a bug. If this situation occurs, the withdrawn
// amount is considered to be the streamed amount, and the stream is effectively frozen.
if (streamedAmount.gt(depositedAmount)) {
return _streams[streamId].amounts.withdrawn;
}
​
// Cast the streamed amount to uint128. This is safe due to the check above.
return uint128(streamedAmount.intoUint256());
}
}

When the following SablierV2Lockup.cancel function is called, the SablierV2Lockup._cancel function below is called, which executes uint128 streamedAmount = _calculateStreamedAmount(streamId). Since streamedAmount can be smaller than what it should be, _streams[streamId].amounts.refunded, which equals amounts.deposited - streamedAmount, can be bigger than what it should be.

https://github.com/Cyfrin/2024-05-Sablier/blob/1fe69e059dcbd71728bdfaa58dad85ff199ee6dc/v2-core/src/abstracts/SablierV2Lockup.sol#L256-L271

function cancel(uint256 streamId) public override noDelegateCall notNull(streamId) {
...
​
// Checks, Effects and Interactions: cancel the stream.
_cancel(streamId);
}

https://github.com/Cyfrin/2024-05-Sablier/blob/1fe69e059dcbd71728bdfaa58dad85ff199ee6dc/v2-core/src/abstracts/SablierV2Lockup.sol#L551-L617

function _cancel(uint256 streamId) internal {
// Calculate the streamed amount.
uint128 streamedAmount = _calculateStreamedAmount(streamId);
​
// Retrieve the amounts from storage.
Lockup.Amounts memory amounts = _streams[streamId].amounts;
​
...
​
// Calculate the sender's amount.
uint128 senderAmount;
unchecked {
senderAmount = amounts.deposited - streamedAmount;
}
​
// Calculate the recipient's amount.
uint128 recipientAmount = streamedAmount - amounts.withdrawn;
​
...
​
// Effect: set the refunded amount.
_streams[streamId].amounts.refunded = senderAmount;
​
// Retrieve the sender and the recipient from storage.
address sender = _streams[streamId].sender;
address recipient = _ownerOf(streamId);
​
// Retrieve the ERC-20 asset from storage.
IERC20 asset = _streams[streamId].asset;
​
// Interaction: refund the sender.
asset.safeTransfer({ to: sender, value: senderAmount });
​
...
​
// Interaction: if the recipient is a contract, try to invoke the cancel hook on the recipient without
// reverting if the hook is not implemented, and without bubbling up any potential revert.
if (recipient.code.length > 0) {
try ISablierV2Recipient(recipient).onLockupStreamCanceled({
streamId: streamId,
sender: sender,
senderAmount: senderAmount,
recipientAmount: recipientAmount
}) { } catch { }
}
}

Impact

When a stream is cancelled, the recipient of the stream can receive an amount that is lower than what the recipient is entitled to, and the sender of the stream can receive an amount that is higher than what the sender is entitled to.

Proof of Concept

Please add the following test file in v2-core\test\unit\fuzz. This testFuzz_SablierV2LockupLinear_calculateStreamedAmountFunctionExecutesDivisionBeforeMultiplication test will pass to demonstrate the described scenario.

// SPDX-License-Identifier: UNLICENSED
pragma solidity >=0.8.22 <0.9.0;
​
import "forge-std/src/Test.sol";
import { UD60x18, ud } from "@prb/math/src/UD60x18.sol";
​
contract chTest is Test {
// run fuzzer 50000 times
/// forge-config: default.fuzz.runs = 50000
function testFuzz_SablierV2LockupLinear_calculateStreamedAmountFunctionExecutesDivisionBeforeMultiplication(uint128 deposited, uint40 startTime, uint40 blockTimestamp, uint40 endTime) external {
// blockTimestamp simulates block.timestamp
// assume block.timestamp is after stream's startTime and before stream's endTime
vm.assume(blockTimestamp > startTime);
vm.assume(endTime > blockTimestamp);
​
// following code simulate SablierV2LockupLinear._calculateStreamedAmount function's code that executes division before multiplication in its unchecked block
​
// Calculate how much time has passed since the stream started, and the stream's total duration.
uint256 startTime = uint256(startTime);
UD60x18 elapsedTime = ud(blockTimestamp - startTime);
UD60x18 totalDuration = ud(endTime - startTime);
​
// Divide the elapsed time by the stream's total duration.
UD60x18 elapsedTimePercentage = elapsedTime.div(totalDuration);
​
// Cast the deposited amount to UD60x18.
UD60x18 depositedAmount = ud(deposited);
​
// Calculate the streamed amount by multiplying the elapsed time percentage by the deposited amount.
UD60x18 streamedAmount = elapsedTimePercentage.mul(depositedAmount);
​
// Cast the streamed amount to uint128.
// streamedAmountReturned is returned by SablierV2LockupLinear._calculateStreamedAmount function that executes division before multiplication
uint128 streamedAmountReturned = uint128(streamedAmount.intoUint256());
​
/////////
​
// unlike streamedAmountReturned, streamedAmountExpected is calculated by executing multiplication before division
uint128 streamedAmountExpected = uint128(elapsedTime.intoUint256() * depositedAmount.intoUint256() / totalDuration.intoUint256());
​
// streamedAmountReturned never exceeds streamedAmountExpected
assertLe(streamedAmountReturned, streamedAmountExpected);
​
// it is likely that streamedAmountReturned is less than streamedAmountExpected
vm.assume(streamedAmountReturned < streamedAmountExpected);
assertLt(streamedAmountReturned, streamedAmountExpected);
}
}

Tools Used

Manual Review

Recommended Mitigation

Instead of executing UD60x18 elapsedTimePercentage = elapsedTime.div(totalDuration) before UD60x18 streamedAmount = elapsedTimePercentage.mul(depositedAmount) when block.timestamp is after _cliffs[streamId] and before _streams[streamId].endTime, the SablierV2LockupLinear._calculateStreamedAmount function can be updated to return the following value.

uint128(elapsedTime.intoUint256() * depositedAmount.intoUint256() / totalDuration.intoUint256())
Updates

Lead Judging Commences

inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

Info/Gas/Invalid as per Docs

https://docs.codehawks.com/hawks-auditors/how-to-determine-a-finding-validity

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.