Should we allow only sender and recipient to call withdraw?

[andreivladbrg]

In the past two weeks, there has been a lot of back-and-forth between @smol-ninja and me regarding how to minimize the precision issue in withdraw, which ultimately only creates a delay problem. It’s becoming overwhelming—every time we think we’ve figured it out, something new comes up.

The major concern here is that an external attacker could exploit this by running a loop to withdraw tokens on behalf of the recipient, causing constant delay. For tokens with a lower value compared to USD (unlike BTC), this wouldn’t be practical from a gas cost perspective, as it would be too expensive.

The quickest solution to the attacker scenario from above, without adding extra complexity to withdraw, is to restrict the function so that only the sender or recipient can call it.

Cons:

• Inconsistency with the lockup.

Pros:

• Less complexity in the withdraw function. (less prone to issues we didn't find yet)
• Cleaner code.
• Consistency in how the time variable is updated in the contract (always using block.timestamp).

Since we need to make a quick decision with the audit approaching, I would appreciate fast feedback from @sablier-labs/engineers.

---

Also, @maxdesalle, do we have a dune query that searches for the percentage of withdraw calls in lockup where msg.sender != recipient , msg.sender != sender or msg.sender != approved operator?

[PaulRBerg]

I'm against this. It's not just a matter of inconsistency with the Lockup; all the cool benefits of public withdraw are lost.

Are you saying that the precision issue cannot be resolved, in principle? That it's inevitable? It's unclear from your statement how bad the situation is.

do we have a dune query that searches for the percentage of withdraw calls in lockup where msg.sender != recipient , msg.sender != sender or msg.sender != approved operator?

• We've just added this functionality
• It's meant to enable use cases not implemented yet (such as automated withdrawals)

[andreivladbrg]

all the cool benefits of public withdraw are lost.

i understand, this is why i asked about the dune query, they may be "cool" but if in practice it is not actually used at all, what to do with them?

it is not about "what is wrong" with the current solution, it is about "what could be wrong and we didn't find" and then --> fewer lines and fewer math operations in withdraw == less prone to issues , we are in a time constraint re audit, so a decision should be taken.

Are you saying that the precision issue cannot be resolved, in principle?

fundamentally it can't be fixed 100%, it is about how rational numbers work, if you want to stream 10 token per day, you would need to have:
rps = 0.0001157407407407407407407407407407407407407407407407 .... and many more decimals

so, with a 18 decimals format, you would have to wait 1 day + 1 second to get that 10 token "streamed" --> 1 second delay of the desired amount over 1 day

it's unclear from your statement how bad the situation is.

IMO, this whole situation isn't "bad" , as it doesn’t lead to a loss of funds, only to a delay of 1 unit of the token (in the case of USDC, $0.000001) over a certain period of time, depending on what the rate per second is

---

It's meant to enable use cases not implemented yet (such as automated withdrawals

we can release the current version without allowing public withdrawals, and after we have made enough tests and we are sure we can make the change

[PaulRBerg]

they may be "cool" but if in practice it is not actually used at all, what to do with them?

Might you have missed the the last two bullet points in my post?

4% of expected streamed amount in this particular case 0.000079e6 USDC

To me, that seems completely acceptable.

this whole situation isn't "bad"

OK then why bother with limiting such a useful functionality (public withdrawals) if the situation isn't bad?

[andreivladbrg]

Might you have missed the the last two bullet points in my post?

might you have missed my last point?

we can release the current version without allowing public withdrawals, and after we have made enough tests and we are sure we can make the change

OK then why bother with limiting such a useful functionality (public withdrawals) if the situation isn't bad?

because of the scenario detailed in the OP, which might annoy the recipient (this still wouldn’t be as bad - the delay - as if we had kept the rps in the token’s decimals), it is an expensive attack to do, tho

external attacker could exploit this by running a loop to withdraw tokens on behalf of the recipient, causing constant delay

[PaulRBerg]

might you have missed my last point?

I have, indeed, so sorry. But your first point was written as if there was no last point. Consolidating them would have been helpful.

we can release the current version without allowing public withdrawals, and after we have made enough tests and we are sure we can make the change

That will complicate things because we will only be able to do it for certain Flow releases.

it is an expensive attack to do, tho

Yeah and the 'attacker' has nothing to gain. The 'attacker' can only grief the recipient.

[andreivladbrg]

Yeah and the 'attacker' has nothing to gain. The 'attacker' can only grief the recipient

Yeah, this is Shub’s argument to this, @smol-ninja:

Llamapay can coordinate this (lossing money in favor of creating a worse experience for competitors)

[PaulRBerg]

It's a valid point.

I think to adjudicate this, we need more quantitative data. What's the worst that can happen?

[smol-ninja]

I am happy to vote for it if everyone agrees with the proposal. However, I have a few points to make:

1. Even if we restrict withdrawal, precision problem continue to exist. Compared to main branch (max deviation: 49%, avg deviation: 0.6%), we have minimized it in fix-precision PR (max deviation: 0.27%, avg deviation: 0.00019%) and data suggests the same. The only caveat is that the implementation in fix-precision PR is not as intuitive as main but can be explained using additional documents that we have already written.

2. We are trusting that sender / recipient won't make frequent withdrawals.

3. However complex a solution is, the inaccuracy / accurancy of the implementation should be justified using tests. Initially, the lack of insufficient tests led us to not been able to understand the precision situation, now with #255 and #258, we have good enough tests to verify for precision loss as well.

4. I'd recommend to test and audit the solution proposed in precision-PR, and if we find newer problems then we solve for them as well. Problems exist but all problems are soluble.

[andreivladbrg]

One mention, the data is made only for withdraw, for adjustRatePerSecond and pause, it would remain the same as in main

[PaulRBerg]

To @smol-ninja:

max deviation: 0.27%, avg deviation: 0.00019%

This doesn't seem that bad to me.

What's the worst that can happen if we continue to allow public withdrawals?

---

@andreivladbrg I didn't understand what you meant in your comment here. Can you explain, please?

[andreivladbrg]

I didn't understand what you meant in your comment here. Can you explain, please?

all functions that have a snapshotTime update suffer of delay problem. what were doing is to address it only in withdraw

[PaulRBerg]

OK, and? Is the point that if someone calls adjustRatePerSecond and pause multiple times, the delay can get very big?

[maxdesalle]

Here you go @andreivladbrg: https://dune.com/queries/4093543

There have essentially been 27 withdrawal transactions where the caller was not the recipient, nor the sender, in Sablier v2.2 on Ethereum. This represents about 2.68% of withdrawals in v2.2 on Ethereum. Only made this analysis for Ethereum as it would be time-consuming to do it for all chains we support, but please definitely let me know if you need data for those other networks as well.

[PaulRBerg]

Thank you for sharing this Max, but as I've pointed out above, this isn't a relevant data point because we haven't started using/ promoting this functionality yet.

[smol-ninja]

Update

We've decided to go with previous implementation (main branch). Given a very realistic scenario i.e. with USDC token, stream rate > $100 / month, 30 withdrawals a month, single withdrawal in range [1 hr, 1 days], it leads to no delay in either of the implementation. The max deviation between the actual withdrawn and expected withdrawn is 0.000029e6 USDC which is also too small.

Link to data

@andreivladbrg should we close this discussion now?

[andreivladbrg]

yes, we can 👍🏻