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
10 months ago
inallhonesty Lead Judge 10 months ago
Submission Judgement Published
Invalidated
Reason: Too generic

Support

FAQs

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