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

Withdrawal is not possible because of precision loss in `SablierFlow._withdraw()`

baz1ka

Relevant GitHub Links

https://github.com/sablier-labs/flow/blob/9213c4eb38419be233bd74cdca7b6afb24cd7bcd/src/SablierFlow.sol#L881

Summary

Because of incorrect assert statement in SablierFlow._withdraw() transaction would revert when recepient (or approved wallets) tries to withdraw accumulated funds.

Vulnerability Details

SablierFlow.withdraw() function allows recepient (or approved wallet) to withdraw provided amount of funds that were accumulated in stream and are available (i.e. sender have deposited enough funds). This function removes amount of funds from balance, and to make sure all calculations are done corret (so noone under no conditions can steal funds) function have additional check:

// Interaction: perform the ERC-20 transfer.
token.safeTransfer({ to: to, value: amount });
​
// Protocol Invariant: the difference in total debt should be equal to the difference in the stream balance.
@> assert(totalDebt - _totalDebtOf(streamId) == balance - _streams[streamId].balance);
​
// Log the withdrawal.
emit ISablierFlow.WithdrawFromFlowStream({
streamId: streamId,
to: to,

This expression (assert) checks that the amount that was withdrawn is the same as amount that was reduced from debt. But problem arises because this amount can actually be different and because of that assert fails and call reverts not allowing user to withdraw funds even though withdraw amount is correct (enough balance to withdraw and enough amount was accumulated).

Proof of Concept

Firstly lets create test and modify SablierFlow (add console logs) and then see why it fails.

  1. Add this test to /tests/integration/fuzz/withdraw.t.sol

function testFuzz_RevertWithdrawBecauseAssertFails(uint128 rps, uint128 depositAmount) external {
UD21x18 ratePerSecond = UD21x18.wrap(boundUint128(rps, 1, 10_000));
uint256 streamId = createDefaultStream(ratePerSecond, usdc);
​
depositAmount = boundUint128(depositAmount, 30, 1e18);
console2.log("depositAmount", depositAmount);
​
// Need that depletionTimeOf is not 0
vm.assume(depositAmount > ratePerSecond.intoUint128());
​
deposit(streamId, depositAmount);
uint256 depletionTimeOf = flow.depletionTimeOf(streamId);
​
vm.warp(depletionTimeOf);
// Try to withdraw all funds
resetPrank(users.recipient);
flow.withdraw(streamId, users.recipient, depositAmount);
}

  1. Modify SablierFlow

diff --git a/src/SablierFlow.sol b/src/SablierFlow.sol
index 55c4103..1107869 100644
--- a/src/SablierFlow.sol
+++ b/src/SablierFlow.sol
@@ -18,6 +18,8 @@ import { Errors } from "./libraries/Errors.sol";
import { Helpers } from "./libraries/Helpers.sol";
import { Broker, Flow } from "./types/DataTypes.sol";
+import { console2 } from "forge-std/src/console2.sol";
+
/// @title SablierFlow
/// @notice See the documentation in {ISablierFlow}.
contract SablierFlow is
@@ -829,6 +831,8 @@ contract SablierFlow is
// 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 {
+ console2.log("totalDebtScaled", totalDebtScaled);
+ console2.log("amountScaled", amountScaled);
_streams[streamId].snapshotDebtScaled = totalDebtScaled - amountScaled;
// Effect: update the stream time.
@@ -863,6 +867,10 @@ contract SablierFlow is
token.safeTransfer({ to: to, value: amount });
// Protocol Invariant: the difference in total debt should be equal to the difference in the stream balance.
+ console2.log("totalDebt", totalDebt);
+ console2.log("_totalDebtOf(streamId)", _totalDebtOf(streamId));
+ console2.log("balance", balance);
+ console2.log("_streams[streamId].balance", _streams[streamId].balance);
assert(totalDebt - _totalDebtOf(streamId) == balance - _streams[streamId].balance);
// Log the withdrawal.

If you run test above it should fail you should see next logs:

[FAIL: panic: assertion failed (0x01); counterexample: calldata=0x2aa84b4d0000000000000000000000000000000068421b37d71d946a91c483bcf261496c000000000000000000000000000000000000000000000000000000000934befd args=[138582955203477823121574976868663249260 [1.385e38], 154451709 [1.544e8]]]

Logs:
depositAmount 154451709
totalDebtScaled 154451710000000008580
amountScaled 154451709000000000000
totalDebt 154451710
_totalDebtOf(streamId) 154442835
balance 154451709
_streams[streamId].balance 0

We can see right away that left part of assert calculated correctly:

balance - _streams[streamId].balance => 154451709 - 0 = 154451709 which is the same as deposit amount and amount to withdraw (user input).

Now let's see left part result:

totalDebt - _totalDebtOf(streamId) = 154451710 - 154442835 = 8875 which is not 0.

Because 8875 != 0 assert fails and call reverts.

Unfortunately I couldn't manage to investigate error further. From what I understood it's because of precision loss and (I think) mostly because rps is small (1.544e8). I tried to avoid precision loss of Helpers.descaleAmount() for totalDebt:

uint256 newTotalDebtScaled = _ongoingDebtScaledOf(streamId) + _streams[streamId].snapshotDebtScaled;
assert(totalDebtScaled - newTotalDebtScaled == balance - _streams[streamId].balance);

but this didn't help. I consider that small rps, descaling and _streams[streamId].snapshotDebtScaled = totalDebtScaled - amountScaled; together make this error.

Impact

  • Break of protocol invariant

  • User can not withdraw funds

Updates

Lead Judging Commences

inallhonesty Lead Judge
8 months ago
inallhonesty Lead Judge 8 months ago
Submission Judgement Published
Invalidated
Reason: Known issue

Appeal created

baz1ka Submitter
8 months ago
inallhonesty Lead Judge
8 months ago
inallhonesty Lead Judge 8 months ago
Submission Judgement Published
Invalidated
Reason: Known issue

Support

FAQs

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