Submission Details
Severity:
Safety check for `SablierV2LockupDynamic` and `SablierV2LockupLinear` are implemented incorrectly.
Summary
In case of bug where streamed amount is more than deposited amount
stream is effectively frozen
but in fact, it's not. Sender can use this opportunity to refund more funds than they should've.
Vulnerability Details
In case of some bug contracts SablierV2LockupDynamic and SablierV2LockupLinear checks whether streamedAmount exceeds deposited amount:
SD59x18 multiplier = elapsedTimePercentage.pow(exponent);
SD59x18 streamedAmount = multiplier.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());
If it happens that the depositedAmount is exceeded withdrawn by recipient value would be returned. In that case, as written the stream should be frozen.
However this check doesn't block SablierV2Lockup.cancel() function. If bad sender sees that in his created stream such bug occurred and user has not withdrawn funds for a long time sender can cancel stream and withdraw more funds than sender should've withdrawn.
SablierV2Lockup.cancel() doesn't know whether bug occurred or not, it just subtracts streamedAmount from user withdrawn amount. If withdrawn amount is 0, sender can withdraw full amount of deposited funds:
// Calculate the streamed amount.
@> uint128 streamedAmount = _calculateStreamedAmount(streamId);
// Retrieve the amounts from storage.
Lockup.Amounts memory amounts = _streams[streamId].amounts;
// Check: the stream is not settled.
if (streamedAmount >= amounts.deposited) {
revert Errors.SablierV2Lockup_StreamSettled(streamId);
}
// Check: the stream is cancelable.
if (!_streams[streamId].isCancelable) {
revert Errors.SablierV2Lockup_StreamNotCancelable(streamId);
}
// Calculate the sender's amount.
uint128 senderAmount;
unchecked {
@> senderAmount = amounts.deposited - streamedAmount;
}
Impact
If bug occurs in _calculateStreamedAmount() sender can withdraw more funds than they can under normal conditions.
Proof of Concept
Because I couldn't find any possible params to make so that streamedAmount > depositedAmount I created dummy SablierV2LockupDynamic contract in which this condition returns true.
Proof Of Code
Paste next changes into v2-core/test/integration/concrete/lockup-dynamic/streamed-amount-of/streamedAmountOf.t.sol
// SPDX-License-Identifier: UNLICENSED
pragma solidity >=0.8.22 <0.9.0;
-import { LockupDynamic } from "src/types/DataTypes.sol";
+import { Lockup, LockupDynamic, Broker } from "src/types/DataTypes.sol";
+import { console2 } from "forge-std/src/console2.sol";
+
+import {SablierV2Lockup} from "src/abstracts/SablierV2Lockup.sol";
+import { ISablierV2NFTDescriptor } from "src/interfaces/ISablierV2NFTDescriptor.sol";
+import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
+import { SafeERC20 } from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
+import { ERC721 } from "@openzeppelin/contracts/token/ERC721/ERC721.sol";
+import { PRBMathCastingUint128 as CastingUint128 } from "@prb/math/src/casting/Uint128.sol";
+import { PRBMathCastingUint40 as CastingUint40 } from "@prb/math/src/casting/Uint40.sol";
+import { SD59x18 } from "@prb/math/src/SD59x18.sol";
+import { Helpers } from "src/libraries/Helpers.sol";
import { LockupDynamic_Integration_Concrete_Test } from "../LockupDynamic.t.sol";
import { StreamedAmountOf_Integration_Concrete_Test } from "../../lockup/streamed-amount-of/streamedAmountOf.t.sol";
+// SablierV2LockupDynamic contract with removed comments, external view functions and bug in _calculateStreamedAmountForOneSegmentWithBug()
+contract LockupDynamicWithBug is
+ SablierV2Lockup
+{
+ using CastingUint128 for uint128;
+ using CastingUint40 for uint40;
+ using SafeERC20 for IERC20;
+
+
+ mapping(uint256 id => LockupDynamic.Segment[] segments) internal _segments;
+
+ constructor(
+ address initialAdmin,
+ ISablierV2NFTDescriptor initialNFTDescriptor
+ )
+ ERC721("Sablier V2 Lockup Dynamic NFT", "SAB-V2-LOCKUP-DYN")
+ SablierV2Lockup(initialAdmin, initialNFTDescriptor)
+ {
+ nextStreamId = 1;
+ }
+
+ function createWithTimestamps(LockupDynamic.CreateWithTimestamps calldata params)
+ external
+ noDelegateCall
+ returns (uint256 streamId)
+ {
+ streamId = _create(params);
+ }
+
+ function _calculateStreamedAmount(uint256 streamId) internal view override returns (uint128) {
+ uint40 blockTimestamp = uint40(block.timestamp);
+ if (_streams[streamId].startTime >= blockTimestamp) {
+ return 0;
+ }
+ uint40 endTime = _streams[streamId].endTime;
+ if (endTime <= blockTimestamp) {
+ return _streams[streamId].amounts.deposited;
+ }
+
+ if (_segments[streamId].length > 1) {
+ return type(uint128).max;
+ } else {
+ return _calculateStreamedAmountForOneSegmentWithBug(streamId);
+ }
+ }
+
+
+ function _calculateStreamedAmountForOneSegmentWithBug(uint256 streamId) internal view returns (uint128) {
+ unchecked {
+ SD59x18 elapsedTime = (uint40(block.timestamp) - _streams[streamId].startTime).intoSD59x18();
+ SD59x18 totalDuration = (_streams[streamId].endTime - _streams[streamId].startTime).intoSD59x18();
+
+ SD59x18 elapsedTimePercentage = elapsedTime.div(totalDuration);
+ SD59x18 exponent = _segments[streamId][0].exponent.intoSD59x18();
+ SD59x18 depositedAmount = _streams[streamId].amounts.deposited.intoSD59x18();
+
+ SD59x18 multiplier = elapsedTimePercentage.pow(exponent);
+
+ // Here is bug - multiply streamed amount by 2 so by 50% elapsedTime streamedAmount exceeds depositedAmount
+ SD59x18 streamedAmount = multiplier.mul(depositedAmount).mul(uint128(2e18).intoSD59x18());
+
+ if (streamedAmount.gt(depositedAmount)) {
+ return _streams[streamId].amounts.withdrawn;
+ }
+
+ return uint128(streamedAmount.intoUint256());
+ }
+ }
+
+ function _create(LockupDynamic.CreateWithTimestamps memory params) internal returns (uint256 streamId) {
+ Lockup.CreateAmounts memory createAmounts =
+ Helpers.checkAndCalculateBrokerFee(params.totalAmount, params.broker.fee, MAX_BROKER_FEE);
+
+ Helpers.checkCreateLockupDynamic(createAmounts.deposit, params.segments, 1, params.startTime);
+
+ streamId = nextStreamId;
+
+ Lockup.Stream storage stream = _streams[streamId];
+ stream.amounts.deposited = createAmounts.deposit;
+ stream.asset = params.asset;
+ stream.isCancelable = params.cancelable;
+ stream.isStream = true;
+ stream.isTransferable = params.transferable;
+ stream.sender = params.sender;
+ stream.startTime = params.startTime;
+
+ unchecked {
+ uint256 segmentCount = params.segments.length;
+ stream.endTime = params.segments[segmentCount - 1].timestamp;
+
+ for (uint256 i = 0; i < segmentCount; ++i) {
+ _segments[streamId].push(params.segments[i]);
+ }
+
+ nextStreamId = streamId + 1;
+ }
+
+ _mint({ to: params.recipient, tokenId: streamId });
+
+ params.asset.safeTransferFrom({ from: msg.sender, to: address(this), value: createAmounts.deposit });
+ }
+}
+
+
contract StreamedAmountOf_LockupDynamic_Integration_Concrete_Test is
LockupDynamic_Integration_Concrete_Test,
StreamedAmountOf_Integration_Concrete_Test
contract StreamedAmountOf_LockupDynamic_Integration_Concrete_Test is
uint128 expectedStreamedAmount = defaults.segments()[0].amount + 2371.708245126284505e18; // ~7,500*0.1^{0.5}
assertEq(actualStreamedAmount, expectedStreamedAmount, "streamedAmount");
}
+
+ // forge test --match-test test_CancelWhenBugOccursInStreamedAmountOf
+ function test_CancelWhenBugOccursInStreamedAmountOf()
+ external
+ givenNotNull
+ givenStreamHasNotBeenCanceled
+ givenStatusStreaming
+ whenStartTimeInThePast
+ {
+ // Create lockup dynamic with 1 segment
+ LockupDynamic.CreateWithTimestamps memory params;
+ Broker memory broker; // Broker data is empty
+
+ params.sender = users.sender;
+ params.recipient = users.recipient;
+ params.asset = dai;
+ params.cancelable = true;
+ params.transferable = true;
+ params.startTime = defaults.START_TIME();pLocky
+ params.broker = broker;
+
+ LockupDynamic.Segment[] memory defaultSegments = defaults.segments();
+ LockupDynamic.Segment[] memory segments = new LockupDynamic.Segment[](1);
+
+ segments[0] = defaultSegments[1];
+ params.totalAmount = segments[0].amount; // set totalAmount to amout of onle segment which is 7500e18
+ params.segments = segments;
+
+ // Dummy SablierV2NFTDescriptor just to deploy contract
+ ISablierV2NFTDescriptor emptySablierV2NFTDescriptor = ISablierV2NFTDescriptor(makeAddr('emptySablierV2NFTDescriptor'));
+
+ // Deploy LockupDynamic which has calculation bug in streamedAmountOf
+ LockupDynamicWithBug lockupDynamicWithBug = new LockupDynamicWithBug(users.admin, emptySablierV2NFTDescriptor);
+
+ // Approve DAI from sender to LockupDynamic and create stream
+ vm.startPrank(users.sender);
+ dai.approve({ spender: address(lockupDynamicWithBug), value: MAX_UINT256 });
+ uint256 streamId = lockupDynamicWithBug.createWithTimestamps(params);
+ vm.stopPrank();
+
+ // Skip time to see it works as usual LockupDynamic
+ vm.warp(defaults.START_TIME() + 10 minutes);
+
+ uint128 streamedAmountAfter10m = lockupDynamicWithBug.streamedAmountOf(streamId);
+ assertEq(streamedAmountAfter10m, 3674234614174767150000);
+
+ // Skip time to timestamp where bug occurs | streamedAmount.gt(depositedAmount)
+ // In our case it's 50% of stream duration
+ vm.warp(defaults.START_TIME() + defaults.TOTAL_DURATION() / 2);
+
+ // Now user have streamedAmount 0 because of incorrect "safety-check" calculation
+ // 0 because user hadn't withdrawn single token
+ uint128 streamedAmountWithBug = lockupDynamicWithBug.streamedAmountOf(streamId);
+ assertEq(streamedAmountWithBug, 0);
+
+ uint128 withdrawableAmount = lockupDynamicWithBug.withdrawableAmountOf(streamId);
+ assertEq(withdrawableAmount, 0);
+
+ uint256 senderBalanceBeforeCancel = dai.balanceOf(users.sender);
+
+ // Sender see that bug and cancel stream
+ vm.prank(users.sender);
+ lockupDynamicWithBug.cancel(streamId);
+
+ uint256 senderBalanceAfterCancel = dai.balanceOf(users.sender);
+
+ // Sender got 100% of deposited amount, although sender should have received far fewer tokens
+ // And recipient have 0 tokens, even though 50% of stream time has passed
+ assertEq(senderBalanceAfterCancel - senderBalanceBeforeCancel, params.totalAmount);
+ }
}
Run this test with:
forge test --match-test test_CancelWhenBugOccursInStreamedAmountOf
Recommended Mitigation Steps
I am considering implementing one of 2 solutions:
Instead of returning withdrawn amount, return deposited amount
Example with SablierV2LockupDynamic._calculateStreamedAmountForOneSegment():
contract SablierV2LockupDynamic is
// 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;
+ return _streams[streamId].amounts.deposited;
}
Return tuple, in which second element would return boolean value which represents whether this condition had occurred or not and if there is error revert
cancel()function.
Updates
Lead Judging Commences
inallhonesty
about 1 year ago
inallhonesty about 1 year ago
Submission Judgement Published 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
In case of a frozen stream a cancelling Sender will withdraw more than they should
Prize pool breakdown
Total prize
53,440 USDC
nSLOC
2,65520 USDC / LOC
45,000 USDC
3,100 USDC
5,340 USDC
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.