fix: bug in snapshot time in the presence of low rps

[smol-ninja]

During withdraw, the normalization process can introduce a small discrepancy in the original time value due to loss of significant digits during the process. So we reverse the normalization calculation and reconstruct the time value to be used in _updateSnapshotTime.

Failed invariant test: https://github.com/sablier-labs/flow/actions/runs/10618225623/job/29432980263?pr=219

@andreivladbrg note the failing fuzz test (WithdrawAt_Integration_Fuzz_Test) in this PR. Ideally, the fuzz test should not fail. The failing fuzz test proves the discrepancy between the the original and the recovered value.

Once we agree on this, I will modify the test to account for the recovered time value.

[andreivladbrg]

Thanks for creating this PR, it indeed seems to be a problem - yet not sure if in src or in tests.

Regarding your proposed solution, it doesn't seem to be a correct implementation as, when you normalize the amount returned by _ongoingDebtOf you add zeros at the end, and not the actual correct normalized amount calculated inside. (i.e. the decimals after the factor: in case of USDC, the last 12 decimals).

For a better understand please see this code example:

    function testUsdc() public pure returns (uint256, uint256, uint256, uint256) {
        // rps ideally for 10 USDC per day
        uint256 rps = 0.00011574074074074e18;
        uint256 elapsedTime = 1 days;
        uint8 decimals = 6;

        // 9.999999999999936000
        uint256 ongoingDebtNorm = elapsedTime * rps;
        // 9.999999
        uint256 ongoingDebtDen = denormalizeAmount(ongoingDebtNorm, decimals);
        // 9.999999000000000000
        uint256 ongoingDebtNormCalculatedInWithdraw = normalizeAmount(ongoingDebtDen, decimals);

        // 86399 - it is normal to be less, since we divide: ongoingDebtNormCalculatedInWithdraw / rps
        uint256 timeReCalculated = ongoingDebtNormCalculatedInWithdraw / rps;

        return (ongoingDebtNorm, ongoingDebtDen, ongoingDebtNormCalculatedInWithdraw, timeReCalculated);
    }

The tests failling is normal.

If you change the withdraw function as the fuzz tests won't fail anymore:

        uint40 reconstructedTime = time;
        uint40 snapshotTime = _streams[streamId].snapshotTime;
        
        if (!_streams[streamId].isPaused && time > snapshotTime) {
            // The normalization process can introduce a small discrepancy in the original `time` value due to loss of
            // significant digits during the process. So we reverse the normalization calculation and reconstruct the
            // `time` value to be used in `_updateSnapshotTime`.

            uint128 normalizedAmount = (time - snapshotTime) * _streams[streamId].ratePerSecond.unwrap();

            reconstructedTime =
                uint40(_streams[streamId].snapshotTime + normalizedAmount / _streams[streamId].ratePerSecond.unwrap());
        }

[smol-ninja]

when you normalize the amount returned by _ongoingDebtOf you add zeros at the end, and not the actual correct normalized amount calculated inside

Precisely why the bug exists. Just to give you an idea, lets assume the following sequence:

1. We provide time as t and calculate ongoing debt as 9.999999999999936000 lets say.
2. Then we denormalize the debt amount in _ongoingDebtOf to 9.999999.
3. Now, given the return value of _ongoingDebtOf to be 9.999999, the exact time corresponds to this new denormalized value would differ by some δ and will not be the one that we provided originally i.e. t.
4. So, in theory, we should store $(t - δ)$ in as snapshot time and not t.

Here is a question for you, given a stream, should function _ongoingDebtOf be mapped one-to-one for all values of time? IMO yes, since its a straightforward calculation. However, normalization transforms _ongoingDebtOf into a many-to-one function and thus the bug.

You can find a range $[t, t+δ]$ in which, you can expect the "same" _ongoingDebtOf for any value of t.

[smol-ninja]

Consider a wrapped bitcoin with 6 decimals. A sender decides to stream 0.001e6 sBTC (~ $60) a day, which makes rps = 0.000000011574e18.

When the elapsed time is between 90 and 170 seconds, the withdrawal amount would always be 1 token. So, an attacker waits until the maximum of this range, i.e., 170 seconds, and triggers a withdrawal with time = snapshotTime + 170. This would withdraw 1 token. Theoretically, at 170 seconds, the amount streamed is 1.96. So, the recipient loses 0.96 tokens (48% of streamed). This can be repeated any number of times and create a non-trivial loss for the recipient.

The solution is to reverse calculate the time and update that as the snapshot time. In this case, since the net withdrawal would be 1 token, we should update the snapshot time as 90 when the time provided is in the range of 90-170.

This is a realistic example.

[andreivladbrg]

I am sorry, but I don't think your example is correct.

the amount streamed is 1.96.

as you said: time = snapshotTime + 170 which means that elapsedTime is 170, right?

thus, ongoingDebt = denormalized(rps * elapsedTime = 0.000000011574e18 * 170) which is actually 1

am i missing smth?

[andreivladbrg]

as discussed on Slack, the problem occurs when elapsedTime is non-zero, but the denormalized ongoing debt is 0.

also, the problem arises when the rps is lower than the minimum value transferable, in case of USDC (6 decimals = min =0.000001).

    uint rps = 0.000000115740740740e18;
    uint elapsedTime = 2 seconds; 
    uint8 decimals = 6;

    // this will return 0, but the snapshotTime will be update with +2
    function testWithdraw() public view returns (uint) {
        uint ongoingDebtNorm = elapsedTime * rps;
        return denormalizeAmount(ongoingDebtNorm, decimals);
    }

---

my proposed solution is to add these to withdraw function:

// Update the snapshot time only if the ongoing debt is greater than 0.
if (ongoingDebt > 0) {
    _updateSnapshotTime(streamId, time);
}

[smol-ninja]

thus, ongoingDebt = denormalized(rps * elapsedTime = 0.000000011574e18 * 170) which is actually 1

At 170s, ongoing debt returns 1 but at 180s it would return 2. So theoretical value of ongoing debt is 1.96 (rps * elapsedTime = 0.000000011574e18 * 170) but due to denormalization, it returns 1. You can supply any value of time in the range [90, 170] and it would always return 1. The loss is zero, if the withdraw is called with t = 90 and positive for all 170 > t > 90.

my proposed solution is to add these to withdraw function

The example I provided has a non zero ongoing debt. So the issue can exist with any value and not just zero.

[andreivladbrg]

oh sheesh, got it now, thanks for re-exlaining

[smol-ninja]

I found an easier way to replicate this issue locally. Bound boundRatePerSecond to boundUint128(ratePerSecond, 0.0000001e18, 10e18); in Utils, i.e. minimum 0.008 tokens a day ($480 in BTC).

I suggest lets reduce the minimum rps bound to 0.0000001e18. Then there is no need to add a new integration test to focus on low RPS with a 6-decimal token as it is now being covered by the fuzz test itself.

Added in my latest commit. Now few more tests fail and the goal is to resolve them all.

[andreivladbrg]

found an easier way to replicate this issue locally. Bound boundRatePerSecond to boundUint128(ratePerSecond, 0.0000001e18, 10e18);

yeah, you are right, it is easier this way

[smol-ninja]

@andreivladbrg feel free to give it a review tomorrow and ask as many questions as come to your mind.

[andreivladbrg]

Is it safe to say, that the problem appears only for tokenDecimals < 18?

If yes, should we optimize the _ongoingDebt so that corrected time is calculated only in this case?

---

Have not reviewed fork tests yet.

Leaving this comments for now.

[andreivladbrg]

hmm, why does adding 1 solves this?

i think what would solve this is, adding the MVT (min value transferable) - in case of USDC is 0.000001e6

also, we should add 1 on if the token decimals is not 18

[andreivladbrg]

i think this is why the CI failed: https://github.com/sablier-labs/flow/actions/runs/10705430477/job/29680865435

[smol-ninja]

why does adding 1 solves this?

I think its a bad solution to add 1 here. I am still trying to figure it out. Will keep you updated.

adding the MVT (min value transferable) - in case of USDC is 0.000001e6

It should be max value of time. Depletion time is beyond which, uncovered debt will be > 0. And its going to be a range [t1, t1 + δ], and during this range, the uncovered debt will still be zero. So I was trying to return t1 + δ so that if we warp to t1 + δ + 1, the uncovered debt will become > 0.

Otherwise if we return t1, at t1 + 1, the uncovered debt may still be zero if its less than t1 + δ.

[smol-ninja]

Here is an explanation for + 1

[smol-ninja]

I fixed it and added it back. have a look know and lmk what you think.

[andreivladbrg]

hmm, why do we need this since the idea behind corrected time is that it should have the same ongoing debt den?

[smol-ninja]

The new total debt = previous total debt at the corrected time (and not at block.timestamp) - withdrawnAmount.

We had uint128 expectedTotalDebt = totalDebt - withdrawAmount before but since the actual values can only be calculated using the correctedTime, the residual debt accounts for that. The point is there is no notion of debt at block.timestamp anymore. Any debt calculated at current time is basically debt at the corrected time which would be slightly less than the block.timestamp.

So either we calculate residualDebt like this or we calculate the previous total debt as sum of snapshot debt and ongoing debt at corrected time.

[smol-ninja]

We can refactor the whole code to allow time to be between snapshot time and corrected time. Right now, the check is between snapshot time and current time which no longer applies. What do you think about it?

If you agree, I can create a separate PR to show it and if you also like it we can consider merging it.

[smol-ninja]

This makes Flow streams works in discrete similar to tranches. The debt remains constant from the last corrected time until the next corrected time regardless of block.timestamp.

[smol-ninja]

I have increased number of runs in ci as I think its more appropriate to have higher number of runs in Flow.

[smol-ninja]

yes, should we optimize the _ongoingDebt so that corrected time is calculated only in this case?

Yes good point. In that case, should we move logic from Helpers contract to _ongoingDebtOf since its the only place where we are calling normalizeAmount and denormalizeAmount? It would be much more efficient to have everything inside _ongoingDebt. Wdyt?

[andreivladbrg]

Helpers contract to _ongoingDebtOf

interesting idea, we still use normalizeAmount in depletionTimeOf

much more efficient

i wouldn't say "much", it is small difference. the problem i see with it, is that the SablierFlow is already having a lot of logic (> 800 lines of code), i would keep Helpers regardless of the small gas differences

[andreivladbrg]

After seeing the code, I can agree with moving the Helpers function directly in the calculation functions

Left some comments:

[andreivladbrg]

what if we add a _snapshotDebtAndTime(streamId) function that is going to be used in pause and adjustRPS:

function _snapshotDebtAndTime(uint256 streamId) internal {
    //  Calculate the ongoing debt and corrected time.
    (uint128 ongoingDebt, uint40 correctedTime) = _ongoingDebtOf(streamId, uint40(block.timestamp));

    // Effect: update the snapshot debt.
    _streams[streamId].snapshotDebt += ongoingDebt;

    // Effect: update the snapshot time.
    _streams[streamId].snapshotTime = correctedTime;
}

[smol-ninja]

Well, the pause does not update the snapshot time and only the debt. So we cannot do this.

[andreivladbrg]

should we add a mirror function for _ongoingDebt in Utils to return both the amount and the corrected time?

[smol-ninja]

Yeah we could.

[smol-ninja]

It requires access to getSnapshotTime, ongoingDebt, getTokenDecimals and oldRatePerSecond which Utils does not have. So how do you propose to mirror it? One way is to pass all these as variables. Is that what you have in your mind?

[PaulRBerg]

Good finding. WDYT about renaming correctedTime to discreteSnapshotTime or smth that includes the 'discrete' terminology?

[smol-ninja]

To @PaulRBerg

WDYT about renaming correctedTime to discreteSnapshotTime or smth that includes the 'discrete' terminology?

I am not sure what would be the appropriate name. Naming variables is hard. The way I see it is that it represents the earliest timestamp when the current amount values (calculated using block.timestamp) would also hold true. So at current timestamp, if we calculate all the amounts, the amounts will also result the same value all the way into the past until we hit correctedTime, regardless of whether we take the snapshot or not. How about minTimeCheckpoint or something like that?

PS: snapshotTime is always discrete technically.

[PaulRBerg]

Good points about the name of the variable .. in this case, let's stick with snapshotTime to not break the symmetry with snapshotDebt. If we find a better name before or during the audit, we will change it then.

[andreivladbrg]

I've just found a big problem with the introduction of correctedTime. This will prevent the PR from being merged, since it could trigger a cascading effect that drains all the stream balance.

The issue arises when the correctedTime calculated inside _ongoingDebtOf equals the snapshotTime and the ongoing debt is greater than zero, which would basically allow anyone to call withdraw until there is not balance left on that stream. This can happen when only a small amount of time has passed, causing the renormalized ongoing amount to result in a value smaller than the rate per second.

To better understand, please see the code

Proof of concept

function test_DrainBalance() public {
    uint8 decimals = 3;
    IERC20 token = createToken(decimals);
    uint128 factor = uint128(10 ** (18 - decimals));

    deal({ token: address(token), to: users.sender, give: 1_000_000e3 });
    resetPrank(users.sender);
    token.approve(address(flow), type(uint256).max);

    vm.warp(MAY_1_2024);

    uint128 rps = 3205.285530987472545e15;
    uint128 depAmount = (rps * 1 days) / factor; // deposit for 1 day
    uint256 streamId = flow.createAndDeposit(users.sender, users.recipient, ud21x18(rps), token, true, depAmount);

    uint40 previousActualSnapshotTime = flow.getSnapshotTime(streamId);
    assertEq(previousActualSnapshotTime, MAY_1_2024);

    uint128 previousActualBalance = flow.getBalance(streamId);
    assertEq(previousActualBalance, depAmount);

    // warp just one second
    uint40 newTimestamp = MAY_1_2024 + 1 seconds;
    vm.warp(newTimestamp);

    uint128 previousActualOngoingDebt = flow.ongoingDebtOf(streamId);
    uint128 previousExpectedOngoingDebt = (rps * 1 seconds) / factor;
    assertEq(previousActualOngoingDebt, previousExpectedOngoingDebt);

    // after 1 second the ongoing debt should be basically rps normalized to token decimals
    assertEq(previousActualOngoingDebt, 3205); // https://www.wolframalpha.com/input?i=3205285530987472545%2F%2810%5E15%29

    // 3205.000000000000000e15 / 3205.285530987472545e15 = 0
    // correctedTime = MAY_1_2024 --> snapshotTime remains the same
    uint40 expectedCorrectedTime = uint40(previousActualOngoingDebt * factor / rps + MAY_1_2024);

    flow.withdrawAt(streamId, users.recipient, newTimestamp);

    uint40 actualSnapshotTime = flow.getSnapshotTime(streamId);
    assertEq(actualSnapshotTime, expectedCorrectedTime);
    assertEq(actualSnapshotTime, previousActualSnapshotTime);
    assertEq(actualSnapshotTime, MAY_1_2024);

    uint128 actualBalance = flow.getBalance(streamId);
    uint128 expectedBalance = previousActualBalance - previousActualOngoingDebt;
    assertEq(actualBalance, expectedBalance);

    // theorectically, the ongoing debt should be 0, it remains the same
    uint128 actualOngoingDebt = flow.ongoingDebtOf(streamId);
    assertEq(previousActualOngoingDebt, actualOngoingDebt);

    // since we have deposited for 1 day, it means we can withdraw for seconds 1 in one day - 1 second (prev
    // withdraw)
    for (uint256 i = 0; i < 1 days - 1 seconds; ++i) {
        uint128 amountWithdrawn = flow.withdrawAt(streamId, users.recipient, newTimestamp);
        assertEq(amountWithdrawn, 3205); // each time 3205
    }

    // the snapshot time remains the same

    actualSnapshotTime = flow.getSnapshotTime(streamId);
    assertEq(actualSnapshotTime, MAY_1_2024);
}

The reason this was never caught by the invariants is that we warp by at least 2 minutes:

flow/test/invariant/handlers/BaseHandler.sol

Line 49 in 7f6ae6c

uint256 timeJump = _bound(timeJumpSeed, 2 minutes, 40 days);

However, I am surprised that it was not caught by fuzz tests, as we do:

flow/test/integration/fuzz/withdrawAt.t.sol

Lines 222 to 230 in 7f6ae6c

	timeJump = boundUint40(timeJump, 1 seconds, 100 weeks);
	uint40 warpTimestamp = getBlockTimestamp() + boundUint40(timeJump, 1 seconds, 100 weeks);
	// Simulate the passage of time.
	vm.warp({ newTimestamp: warpTimestamp });
	// Bound the withdraw time between the allowed range.
	withdrawTime = boundUint40(withdrawTime, MAY_1_2024, warpTimestamp);

A potential solution might be to return ongoingAmount = 0 in case correctedTime = snapshotTime
@sablier-labs/solidity wdyt?

[andreivladbrg]

I think we should also add an invariant: if the functions (adjustRatePerSecond, pause, withdrawAt ) that updates snapshotTime are called, the time cannot remain constant

[PaulRBerg]

the time cannot remain constant

What time? correctedTime?

And why cannot it remain constant? If multiple withdraw txs are included in the same block, the correctedTime should be the same. Right?

[andreivladbrg]

Please review my finding above. correctedTime cannot be the same with snapshotTime, and if ongoingDebt > 0.

If the ongoing amount is zero, it's fine to update snapshotTime with the same value

[PaulRBerg]

Good catch, @andreivladbrg!

I have followed up here.

[smol-ninja]

Good finding @andreivladbrg. Glad you found it before we could merge the PR.

I am thinking, as a solution, if we compare the value of correctedTime with snapshotTime, it may not be very obvious why we return 0. So how about, if we put a comparison between renormalizedOngoingDebt and ratePerSecond and return 0 if renormalizedOngoingDebt < ratePerSecond? This seems to be easier to understand i.e. to return 0 if the renormalized value is less than ratePerSecond.

[smol-ninja]

I think we should also add an invariant: if the functions (adjustRatePerSecond, pause, withdrawAt ) that updates snapshotTime are called, the time cannot remain constant

Agree, and also the following: allow 1 second to pass through passTime and in fuzz too. Wdyt?

[andreivladbrg]

if we put a comparison between renormalizedOngoingDebt and ratePerSecond and return 0 if renormalizedOngoingDebt < ratePerSecond? This seems to be easier to understand i.e. to return 0 if the renormalized value is less than ratePerSecond.

agree with you, it is easier to understand if we compare the rps with the renormalized amount

allow 1 second to pass through passTime and in fuzz too. Wdyt?

i would even bound the time between [0 seconds, x number of days]

[smol-ninja]

I have only included the check in the SablierFlow.sol file. And will review / edit tests in #232 as it requires more changes in tests than expected.

@andreivladbrg lmk if its good to merge then. And lets continue this in your PR.

[andreivladbrg]

I have only included the check in the SablierFlow.sol file. And will review / edit tests in #232 as it requires more changes in tests than expected.

agree, let's merge it, the PR is already spammed, we will address issues in the withdraw one 👍

[PaulRBerg]

team work