Return correctedTime in getters that rely on block.timestamp

[smol-ninja]

Recently, I've realized that there is no point in having block.timestamp anywhere in the code. The debt calculations rely more on the value of the corrected time (#220) than block.timestamp. Even when we pass block.timestamp, the theoretical amounts calculated may not match because they are rounded down using corrected time.

Thus, I propose to go with one of the following options:

1. remove block.timestamp from functions like coveredDebtOf, totalDebt, etc., and introduce a time input parameter.
2. Or return correctedTime in all these functions to inform users that the returned amounts are calculated based on correctedTime and not block.timestamp.

PS: in option (1), we would still have to return correctedTime so only option (2) makes sense.

cc @sablier-labs/solidity

[PaulRBerg]

I will review this and the related PR tomorrow if that's OK!

[smol-ninja]

Absolutely fine. I'd also recommend you to ask questions as they come to your mind. There are a few changes that would require explanations.

[andreivladbrg]

having block.timestamp anywhere in the code.

I am not sure I understand what are you referring to. How do you calculate the elapsedTime?

As mentioned on slack, we won't have anymore the time param (in any place) after we change the withdraw function to amount

[smol-ninja]

I am not talking about withdraw function. I suppose even with the amount parameter, the notion of correctedTime will not disappear. You will likely face the same precision problem as we have discussed. What I am saying is that _ongoinDebt returns correctedTime which is the time that will have no precision loss.

So when a user calls totalDebt and similar functions, we use block.timestamp internally to calculate the total debt but in real, that value is calculated at correctedTime. For example, in my previous example, at t = 170, total debt returns 1 to the user but in theory it should have returned 1.96. So we say, that hey, your total debt is 1 but its 1 at t = 90 (corrected time).

Did I make sense now?

[andreivladbrg]

nope, sorry but it didn't make sense, once again, how do you calculate the elapsedTime?

correctedTime is calculated based on a range [snapshotTime, what's here?]

[smol-ninja]

In the OP, I provided two options:

1. remove block.timestamp from functions like coveredDebtOf, totalDebt, etc., and introduce a time input parameter

In this case, the elapsedTime is calculated based on the time input parameter as = time - snapshotTime

2. return correctedTime in all these functions to inform users that the returned amounts are calculated based on correctedTime and not block.timestamp.

In this case, we calculate elapsedTime as block.timestamp - snapshotTime but also returns correctedTime to the user so he knows what time is block.timestamp corrected to.

Does that answer?

PS: in option (1), we would still have to return correctedTime so only option (2) makes sense.

[andreivladbrg]

In this case, we calculate elapsedTime as block.timestamp - snapshotTime but also returns correctedTime to the user so he knows what time is block.timestamp corrected to.

ok, but that's what i said re withdraw amount

flow/src/SablierFlow.sol

Lines 440 to 456 in 0bc7524

	/// @dev Calculates the ongoing debt accrued since last snapshot. Return 0 if the stream is paused.
	function _ongoingDebtOf(uint256 streamId) internal view returns (uint128) {
	uint40 snapshotTime = _streams[streamId].snapshotTime;
	uint40 blockTimestamp = uint40(block.timestamp);
	// If the stream is paused or the time is less than the `snapshotTime`, return zero.
	if (_streams[streamId].isPaused || blockTimestamp <= snapshotTime) {
	return 0;
	}
	uint128 elapsedTime;
	// Safe to use unchecked because subtraction cannot underflow.
	unchecked {
	// Calculate time elapsed since the last snapshot.
	elapsedTime = blockTimestamp - snapshotTime;
	}

we will no longer have to pass block.timestamp as there will no longer be uint40 time in _ongoingDebt coveredDebtOf totalDebt etc.

when i will open that PR, may be it will be easier to understand

[smol-ninja]

As long as we have snapshotTime, the issue exists even if we move away from time to amount. The issue exists because of the precision error due to division in solidity. So, the notion of correctedTime would still be required.

[PaulRBerg]

My Reinterpretation

I have reviewed your findings in #220 and the proposal herein now. Absolutely stellar work, @smol-ninja. This was a tough nut to crack.

To explain the problem in my own terms:

• The _ongoingDebtOf function is non-injective with respect to its input parameter time.
• Due to that, the calculations can lead to the stored balance lagging the real ERC-20 balance of the stream (it would lead to a delta).
• The deltas between the real ERC-20 balances and the stored balance would remain as surpluses in the contract, which could be claimed by the Sablier Admin (related: Implement ERC-20 token recovery mechanism #187). But this surplus belongs rightfully to users.
• While we cannot make the _ongoingDebtOf function injective (ERC-20 be damned), we can correct for the lack of injectivity by discretizing the snapshot times.
• Discretizing the snapshot times means rounding down to the nearest Unix timestamp that produces the smallest remainder greater than the minimum transferable value a.k.a. the correctedTime

Questions

1. The renormalizedOngoingDebt / ratePerSecond calculation will produce a remainder most of the time. Can't this also lead to rounding errors and an imbalance between the stored balance and the real ERC-20 balance (I think not — I am just thinking out loud).
2. When you suggested 'reducing the minimum RPS bound' to 0.0000001e18, did you @smol-ninja mean to do this in production or in the tests? (BTW, I think you meant to say to increase the minimum bound)

Alternative Solution

I think that the discrete snapshot time is a better idea, but here's an alternative solution.

• Add a remainder variable in the Stream struct
• At withdrawal time, take the modulus between normalizedOngoingDebt with respect to renormalizedOngoingDebt, and add it to this remainder variable
• Also at withdrawal time, check if the the remainder is greater than the $mtf$ (minimum transferable value) of the stream's token, and if it is, remove the $mtf$ from the remainder and add it to the withdraw amount

This solution might result in more calculations and an increased gas cost.

---

P.S. it looks like it was a good thing after all to use a token with 6 decimals as the default.

[smol-ninja]

To explain the problem in my own terms:

Fantastic summary 👏👏👏. I would update the natspec for _ongoingDebtOf using your summary points.

The renormalizedOngoingDebt / ratePerSecond calculation will produce a remainder most of the time. Can't this also lead to rounding errors and an imbalance between the stored balance and the real ERC-20 balance (I think not — I am just thinking out loud).

Good question. An amount less than renormalizedOngoingDebt and it would produce a different correctedTime. So I would say the difference would be less than 1 second and therefore the delta in balances would be less than rps.

When you suggested 'reducing the minimum RPS bound' to 0.0000001e18, did you mean to do this in production or in the tests? (BTW, I think you meant to say to increase the minimum bound)

Reduce the minimum bound in boundRatePerSecond function so that foundry can run the fuzz tests with very low rps in CI as well as during local testing.

/// @dev Bounds the rate per second between a realistic range.
function boundRatePerSecond(UD21x18 ratePerSecond) internal pure returns (UD21x18) {
    return ud21x18(boundUint128(ratePerSecond.unwrap(), 0.0000001e18, 10e18));
}

alternative solution

Thanks for sharing. It can work too. But as you mentioned, due to storage access, it will be more expensive. However, lets keep it in mind in case we find another bug in the current implementation or any concerns during audits.

it looks like it was a good thing after all to use a token with 6 decimals as the default

Absolutely 👑.

[PaulRBerg]

I would update the natspec for _ongoingDebtOf using your summary points.

Great!

I would say the difference would be less than 1 second and therefore the delta in balances would be less than rps.

Oh, yes. So it's negligible.

Reduce the minimum bound in boundRatePerSecond function

Got it, thanks for confirming.

lets keep it in mind in case we find another bug in the current implementation or any concerns during audits.

Good point.

[PaulRBerg]

Responding to @andreivladbrg here:

A potential solution might be to return ongoingAmount = 0 in case correctedTime = snapshotTime

That would work, but I have this gnawing sense that the implementation would become increasingly uglier, like a Rube Goldberg machine.

I am starting to like the remainder tracker solution more because it's cleaner, which also means (slightly) safer and easier to audit.

WDYT?

[smol-ninja]

I don't think that it would become uglier if we do something like this. The remainder tracker solution may have its own problems that we cannot anticipate without implementing it. With the correctedTime, we have spent good amount of time to fix all the issues and its working now or atleast we understand it thoroughly (I may be biased here because I spent the time on the implementation of correctedTime but just a thought before we refactor to the new solution).

[andreivladbrg]

I am starting to like the remainder tracker solution more because it's cleaner,

I still believe that it is not cleaner.

implementation would become increasingly uglier

it is just one more check at the end of the function:

flow/src/SablierFlow.sol

Lines 516 to 519 in ab49775

	// There is an edge case where the corrected time is equal to the snapshot time.
	if (correctedTime == snapshotTime) {
	ongoingDebt = 0;
	}

Let's make a comparison. Will add only the lines that are added in plus compared to main:

Current version and correctTime check

Solidity implementation

function _ongoingDebtOf(
    uint256 streamId,
    uint40 time
)
    internal
    view
    returns (uint128 ongoingDebt, uint40 correctedTime)
{
    // --snip--
    // Renormalize the ongoing debt for the calculation of corrected time.
    uint128 renormalizedOngoingDebt = (ongoingDebt * (10 ** factor)).toUint128();

    // Derive the corrected time from the renormalized ongoing debt.
    correctedTime = uint40(snapshotTime + renormalizedOngoingDebt / ratePerSecond);

    // There is an edge case where the corrected time is equal to the snapshot time.
    if (correctedTime == snapshotTime) {
        ongoingDebt = 0;
    }
}

function _withdrawAt(uint256 streamId, address to, uint40 time) internal returns (uint128 withdrawAmount) {
    // --snip--
    (uint128 ongoingDebt, uint40 correctedTime) = _ongoingDebtOf(streamId, time);
    _streams[streamId].snapshotTime = correctedTime;
    // --snip--
}

//  there are also `adjustRatePerSecond` and `pause` functions which have the same change as `withdrawAt`

Remainder solution

(I hope I understood it correctly)

Solidity implementation

/// DataTypes.sol
struct Stream {
    // --snip--
    uint128 remainderDebt;
}

/// SablierFlow.sol

// also `_create` must be updated

// same number of calculation in this function, only the check at the end missing (cheap operation)
function _ongoingDebtOf(
    uint256 streamId,
    uint40 time
)
    internal
    view
    returns (uint128 ongoingDebt, uint128 remainderDebt)
{
    // --snip--
    // Renormalize the ongoing debt for the calculation of corrected time.
    uint128 renormalizedOngoingDebt = (ongoingDebt * (10 ** factor)).toUint128();

    // Derive the corrected time from the renormalized ongoing debt.
    remainderDebt = normalizedOngoingDebt % renormalizedOngoingDebt;
}

// 1 more storage update, and two more calculations (newRemainderDebt and mvt)
function _withdrawAt(uint256 streamId, address to, uint40 time) internal returns (uint128 withdrawAmount) {
    // --snip--
    (uint128 ongoingDebt, uint128 remainderDebt) = _ongoingDebtOf(streamId, time);

    uint 128 newRemainderDebt = _streams[streamId].remainderDebt + remainderDebt;

    uint128 mvt = 10 ** _streams[streamId].tokenDecimals;

    if (newRemainderDebt >= mvt) {
        withdrawAmount += newRemainderDebt;
        _streams[streamId].remainderDebt = 0;
    } else {
    _streams[streamId].remainderDebt += remainderDebt;
   }
    // --snip--
}

// one more storage update both `remainderDebt` and `snapshotTime` updates 
function _adjustRatePerSecond(uint256 streamId, UD21x18 newRatePerSecond) internal {
    // --snip--
    (uint128 ongoingDebt, uint128 remainderDebt) = _ongoingDebtOf(streamId, time);
    _streams[streamId].remainderDebt += remainderDebt;
    _streams[streamId].snapshotTime = uint40(block.timestamp);
}

// same thing in pause function as in `_adjustRatePerSecond`

As you can see, the remainder solution is still more complex and not easier to audit. IMO as the number of storage variables increases, the audit becomes more challenging. Moreover, the remainder solution doesn’t just add an extra storage variable - it also introduces additional calculations.

Other aspect, is that the remainder solution would create a discrepancy between the amount parameter in this PR and the actual amount withdrawn, which adds "noise" for the recipient (e.g. requesting 10 tokens but receiving 10.000001 tokens). How would we explain that? With the protocol fee, it’s easy to justify

[PaulRBerg]

Excellent criticism from both, and thank you so much for the code comparison @andreivladbrg.

Let's go with your proposal and adjust correctedTime. I am convinced.

[smol-ninja]

Closing this discussion now.