Flow
Sablier
FoundryDeFi
20,000 USDC
View results
Previous Next
Submission Details
Severity: medium
Invalid

Ongoing debt may return a wrong value

greed

Summary

When users withdraw accumulated debt, the stream's snapshotDebtScaled is updated differently on 2 different conditions :

https://github.com/Cyfrin/2024-10-sablier/blob/main/src/SablierFlow.sol#L823-L840

function _withdraw(
uint256 streamId,
address to,
uint128 amount
) {
// ---------- snip -----------
// If the amount is less than the snapshot debt, reduce it from the snapshot debt and leave the snapshot
// time unchanged.
if (amountScaled <= _streams[streamId].snapshotDebtScaled) {
@> _streams[streamId].snapshotDebtScaled -= amountScaled;
}
// Else reduce the amount from the ongoing debt by setting snapshot time to `block.timestamp` and set the
// snapshot debt to the remaining total debt.
else {
@> _streams[streamId].snapshotDebtScaled = totalDebtScaled - amountScaled;
​
// Effect: update the stream time.
_streams[streamId].snapshotTime = uint40(block.timestamp);
}

One of the condition will update the snapshotTime as well while the other condition won't.

Vulnerability Details

The issue relies in the fact that when snapshotTime is NOT updated in a particular scenario.

All the functions relying solely on the returned value of the internal _ongoingDebtScaledOf() function will return a wrong value.

The reason for that is _ongoingDebtScaledOf() performs its internal calculations based upon the elapsed time which is obtained through the subtraction block.timestamp - snapshotTime.

The below is a coded PoC that can be pasted in tests\integration\concrete\depletion-time-of\depletionTimeOf.t.sol that demonstrates the ongoingDebtScaled returning a wrong value :

function test_WithdrawToWrongTotalDebtOf() external givenNotNull givenNotPaused givenBalanceNotZero givenNoUncoveredDebt {
// 1s == 0.1 USDC
UD21x18 rps = UD21x18.wrap(0.1e18);
uint256 streamId = createDefaultStream(rps, usdc);
uint40 actualDepletionTime;
​
deal({ token: address(usdc), to: users.sender, give: UINT128_MAX });
usdc.approve(address(flow), UINT128_MAX);
​
flow.deposit(streamId, 1000e6, users.sender, users.recipient);
​
vm.warp(block.timestamp + 100);
// 1s == 1 USDC
rps = UD21x18.wrap(1e18);
flow.adjustRatePerSecond(streamId, rps);
uint256 expectedOnGoingDebtScaled = 100 ether;
​
vm.warp(block.timestamp + 100);
​
// on going debt scaled is 100e18
assertEq(flow.ongoingDebtScaledOf(streamId), expectedOnGoingDebtScaled);
​
// withdrawing part of the stream
flow.withdraw(streamId, users.recipient, 10e6);
​
// on going debt scaled remains unchanged
assertEq(flow.ongoingDebtScaledOf(streamId), expectedOnGoingDebtScaled);
}

Impact

The functions relying on _ongoingDebtScaledOf() will return a wrong value leading to wrong calculations

Tools Used

Manual review

Recommendations

Systematically update the snapshotTime when the snapshotDebtScaled is updated.

Updates

Lead Judging Commences

inallhonesty Lead Judge
8 months ago
inallhonesty Lead Judge 8 months ago
Submission Judgement Published
Invalidated
Reason: Too generic

Support

FAQs

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