sablier-labs / flow

🍃 Smart contracts of the Sablier Flow protocol.
Other
8 stars 0 forks source link

feat: recover surplus amount of tokens #234

Closed smol-ninja closed 3 weeks ago

smol-ninja commented 1 month ago

Changelog

andreivladbrg commented 1 month ago

The PR is still marked as draft, but I want to mention that we need to update the state in the refund function as well

smol-ninja commented 1 month ago

@andreivladbrg feel free to give it a review. This is the final PR to be merged before we can hand over the sloc to the auditors.

smol-ninja commented 1 month ago

is there a reason why you did not add new invariant tests?

I can add but the invariant that makes sense is aggregateBalance == token.balanceOf(flow). Handler can't make deposits to the protocol so it should just be equal to token.balanceOf(flow) all the time.

However, I plan to introduce a new invariant that compares stream balance, revenue, aggregateBalance and token balance which goes like this: all stream balance sum + revenue = aggregate balance = token balance.

But to do that, I'd like to track revenue separately through flowStore. I know you have said that flowStore tracks flow contract states but that does not seem true to me anymore. We track depositedAmounts refundedAmounts and withdrawnAmounts, none of them are contract states so why should we not track revenue as well.

If you agree, I will bring this invariant. if you don't, I would like to know why.

PS: let's also introduce FlowAdminHandler to implement collectProtocolRevenue and recover.

smol-ninja commented 1 month ago

I've added an invariant: token.balanceOf() = flow.aggregateBalance(token). Combining invariant (2) and (3) yield to stream balance sum + revenue = aggregate balance = token balance.

Re

FlowAdminHandler to implement collectProtocolRevenue and recover

I will start a discussion for inputs on this idea.

smol-ninja commented 3 weeks ago

@andreivladbrg I have updated the PR. LMK if it looks good now.

smol-ninja commented 3 weeks ago

Lets also reduce the CI runs now as we have CI deep for higher runs.