fix: precision issues

[smol-ninja]

Please see natspec comments for the explanation.

Tasks:

• Adjust amount in withdraw
• Check that withdrawAmount is not less than rps
• Update concrete tests for withdrawAmount

[andreivladbrg]

Thanks for the PR.

I want to mention that this solution seems to work for _withdraw (I couldn’t find anything that breaks it), but pause and adjustRatePerSecond still suffer from the delay issue.

You can run this:

Delay test

Ran on this commit eb096bc

function testDelayUsdc_OngoingDebt() public {
    uint128 rps = 0.000000011574e18; // 0.001e6 USDC per day, less than smallest value of USDC 0.00000001e6
    uint128 depositAmount = 0.001e6;

    uint128 factor = uint128(10 ** (18 - 6));

    uint40 constantInterval = uint40(factor / rps); // 10^12 / (1.1574 * 10^10)
    assertEq(constantInterval, 86, "constant interval");

    uint256 streamId = flow.createAndDeposit(users.sender, users.recipient, ud21x18(rps), usdc, true, depositAmount);

    uint40 initialSnapshotTime = MAY_1_2024;
    assertEq(flow.getSnapshotTime(streamId), initialSnapshotTime, "snapshot time");

    // rps * 1 days = 0.000999e6 due to how the rational numbers work in math :))
    // so we need to warp one more second in the future to get the deposit amount
    vm.warp(initialSnapshotTime + 1 days + 1 seconds);

    assertEq(flow.ongoingDebtOf(streamId), depositAmount, "ongoing debt vs deposit amount");

    // now, since everything has work as expected, let's go back in time to pause and then immediately restart

    // the first discrete release is at constantInterval + 1 second
    // after that, it is periodic to constantInterval

    // warp to a timestamp that ongoing debt is greater than zero
    vm.warp(initialSnapshotTime + constantInterval + 1);
    assertEq(flow.ongoingDebtOf(streamId), 1, "ongoing debt vs first discrete release");

    // to test the constant interval is correct
    uint40 delay = constantInterval - 1;
    vm.warp(initialSnapshotTime + (constantInterval + 1) + delay);
    assertEq(flow.ongoingDebtOf(streamId), 1, "ongoing debt vs delay"); // same as before

    // we will have delay of (constantInterval - 1)
    flow.pause(streamId);
    flow.restart(streamId, ud21x18(rps));

    // assert that the snapshot debt has been updated with the ongoing debt
    assertEq(flow.getSnapshotDebt(streamId), 1, "snapshot debt");

    // now, let's go again at the time we've tested ongoingDebt == depositAmount
    vm.warp(initialSnapshotTime + 1 days + 1 seconds);

    // theoretically, it needs to be depositAmount - snapshotDebt, but it is not
    // as we have discrete intervals, the full initial deposited amount gets released now after the delay

    assertFalse(
        flow.ongoingDebtOf(streamId) == depositAmount - flow.getSnapshotDebt(streamId),
        "ongoing debt vs deposit amount - snapshot debt first warp"
    );

    vm.warp(initialSnapshotTime + 1 days + 1 seconds + delay + 1 seconds);
    assertEq(
        flow.ongoingDebtOf(streamId),
        depositAmount - flow.getSnapshotDebt(streamId),
        "ongoing debt vs deposit amount - snapshot debt second warp"
    );
}

Therefore, my alternative is to reintroduce correctedTime, but calculate it only if certain criteria are met:

1. factor = 10 ^ (18 - tokenDecimals)
2. rps < factor
3. rps > factor && rps % factor != 0
4. re-calculate ongoingDebt from correctedTime

Not yet very sure about 3. , but re. 4:

newElapsedTime = correctedTime - snapshotTime;
uint128 recalculatedOngoingDebt = (newElapsedTime * ratePerSecond) / factor;

[smol-ninja]

Thanks for the feedback @andreivladbrg. The following code in withdraw is analogous to correctedTime.

_streams[streamId].snapshotTime += uint40(
    ((amount - _streams[streamId].snapshotDebt) * (10 ** (18 - _streams[streamId].tokenDecimals)))
        / _streams[streamId].ratePerSecond.unwrap()
);

Are you talking about using storing correctedTime in pause and adjustPerSecond instead of block.timestamp? If yes, I have tried it already. It has some issues as described below:

1. In case of adjustPerSecond, when you store corrected time and then change the rps, since the snapshot time would be the corrected time, between corrected time and block.timestamp, it would use the new rps to calculated streamed amount. This can result into over-streaming to the recipient.
2. In case of pause, the objective with not using corrected time is to allow user to withdraw the entire amount.

Thus, I intentionally did not use corrected time in both of them.

[andreivladbrg]

The following code in withdraw is analogous to correctedTime

Yes

Are you talking about using storing correctedTime in pause and adjustPerSecond instead of block.timestamp? If yes, I have tried it already. It has some issues as described below:

In ongoingDebt I mean.

• In case of adjustPerSecond, when you store corrected time and then change the rps, since the snapshot time would be the corrected time, between corrected time and block.timestamp, it would use the new rps to calculated streamed amount. This can result into over-streaming to the recipient.
• In case of pause, the objective with not using corrected time is to allow user to withdraw the entire amount

Hmm, I don't understand these arguments, as correctedTime is bounded between [t0,t1] where each value would cause ongoingDebt in to a constant value. And we need to find t0.

Having said that, how would this lead to over-streaming? Even more, what if the block.timestamp when adjustRatePerSecond is called is actually the correctedTime?

[andreivladbrg]

One other alternative would be:

uint40 constantInterval = uint40((1e18 * factor) / rps);
// so ongoing debt would be how many intervals can fit in elapsed time
uint128 ongoingDebt = (1e18 * elapsedTime) / constantInterval;

what it does, it finds the "period" of time when the ongoingDebt remains constant, and then calculates the ongoing debt, by finding how many intervals can fit within elapsed time

but this version, since it has one more division involved might be more problematic

[smol-ninja]

Hmm, I don't understand these arguments, as correctedTime is bounded between [t0,t1] where each value would cause ongoingDebt in to a constant value. And we need to find t0.

In adjustRatePerSecond, this is what can happen:

• Update snapshot time to t0
• Set new value for ratePerSecond.
• Between t0 and the tx time, which rate per second should be used? We cannot use old rps because its no longer accessible. And if we use new RPS, it leads to overstreaming by (new rps - old rps) * (tx time - t0).

Its a small number we can adjust for it, but then we might have to modify some of the failing invariants because of this.

[andreivladbrg]

Damn, yeah got it now

[PaulRBerg]

Feedback below. I think I found two important issues:

• Invariant new total debt == ongoing debt does not seem to hold at the end of the execution in _withdraw
• Return amount is incorrect

P.S. sorry for renaming previous to initial and normalized to scaled in this PR.

[andreivladbrg]

since the idea behind the adjusment of withdrawAmount is to find the largest multiple of rps that is lower than or equal to the parameter itself, it would be a good idea to add an assert statement:

if (prevAmount != withdrawAmount) {
    assert(10 ** (18 - _streams[streamId].tokenDecimals) * (prevAmount - withdrawAmount) < ratePerSecond.unwrap());
}

[andreivladbrg]

i find the current version of withdraw verbose, it has too many comments which might make the reader "distracted" IMO - thus i would reduce the comments

also, we could consider dividing it in multiple functions (regardless of the gas implications)

wydt? @sablier-labs/solidity

[smol-ninja]

we could consider dividing it in multiple functions (regardless of the gas implications)

Since these logics are only used in withdraw function, how would splitting it into multiple function improve readability?

it has too many comments which might make the reader "distracted" IMO - thus i would reduce the comments

On the contrary, the comments helps readers while reading the code instead of distracting them. Without the comments, the readers might not even understand why does it have some lines.

[andreivladbrg]

Since these logics are only used in withdraw function, how would splitting it into multiple function improve readability?

One idea could be:

Toggle to see code

/// FlowBase
function _updateProtocolState(
    uint128 totalAmount,
    IERC20 token
)
    internal
    returns (uint128 netAmount, uint128 feeAmount)
{
    // Load the variables in memory.
    UD60x18 protocolFee = protocolFee[token];

    if (protocolFee > ZERO) {
        // Calculate the fee amount based on the fee percentage.
        feeAmount = ud(totalAmount).mul(protocolFee).intoUint128();

        // Calculate the net amount after subtracting the fee from the total amount.
        netAmount = totalAmount - feeAmount;

        // Safe to use unchecked because addition cannot overflow.
        unchecked {
            // Effect: update the protocol revenue.
            protocolRevenue[token] += feeAmount;
        }
    } else {
        netAmount = totalAmount;
    }

    unchecked {
        // Effect: update the aggregate amount.
        aggregateBalance[token] -= netAmount;
    }
}

/// Flow
function _withdraw(uint256 streamId, address to, uint128 withdrawAmount) internal {
    // --snip--
    (withdrawAmount, feeAmount) = _updateProtocolState(withdrawAmount, token);
}

i think solutions are

---

the comments helps readers while reading the code instead of distracting them

i consider self-explanatory code + succinct comments to be the best way to write code. anything more detailed can be addressed in the docs IMO

[PaulRBerg]

As discussed on Slack, we will keep the verbose comments in the withdraw function because the logic is sophisticated and non-intuitive and requires a proper explanation.

In addition:

• Let's round down the user-provided amount in the refund function
• Let's create a subroutine for rounding down since this is shared logic between withdraw and refund

[smol-ninja]

The test failed because I moved withdrawnAmount == 0 check post if..else which we need to update in the invariant handler.

[PaulRBerg]

which we need to update in the invariant handler.

Also in BTT trees right?

[smol-ninja]

I am not sure if there is a need to round down the refund amount. Even if refund amount modifies the balance such that withdrawable is not a multiple of rps (when balance is withdrawable), the else logic of _withdraw is independent of balance value. Ergo, functions like deposit, refund etc does not affect it. cc @andreivladbrg.

[PaulRBerg]

Yeah, I think you're right @smol-ninja. If the stream is paused or voided, the withdrawable amount that is not a multiple of rps would be withdrawable in full without being rounded down.

[smol-ninja]

@andreivladbrg can we please merge this PR now since we have all agreed on the solution? And #255 also helps in verifying that the approach used in this PR is better.

[smol-ninja]

Closing this PR. Thanks everyone the long productive discussion.

I have created a new branch: https://github.com/sablier-labs/flow/tree/feat/support-extremely-low-rps for future reference.