sablier-labs / v2-core

⏳ Core smart contracts of the Sablier V2 token distribution protocol
https://sablier.com
Other
307 stars 43 forks source link

Use `try...catch` in `cancelMultiple` and `withdrawMultiple` to handle invalid stream IDs #917

Open smol-ninja opened 4 months ago

smol-ninja commented 4 months ago

As discussed here, batch functions such as cancelMultiple and withdrawMultiple should be allowed to continue execution if one of the stream IDs revert.

A sample implementation would look like the following:

event InvalidStreamIDInBatch(uint256 id, string memory reason);

function cancelMultiple(uint256[] calldata streamIds) external override {
    for (uint256 i = 0; i < streamIds.length; ++i) {
        try cancel(streamIds[i]) {
        } catch Error(string memory reason){
            emit InvalidStreamIDInBatch(streamIds[i], reason);
        }
    }
}
PaulRBerg commented 4 months ago

LGTM except for the error name.

I would create two errors, one for withdrawals, and another for cancellations.

I'd also say something like InvalidWithdrawalInBatch

PaulRBerg commented 4 months ago

@smol-ninja could you please create a milestone for the next Lockup release, and include this issue in that milestone?

And let's name it according to the package tethering approach.

smol-ninja commented 4 months ago

Will do.

smol-ninja commented 4 months ago

Did you mean board or milestone @PaulRBerg?

We used board to track v2.2, which I have created one here: https://github.com/orgs/sablier-labs/projects/19/views/2. I am calling it Lockup 1.2.1 but if it ends up being a big release (depending on the issues and features we add to it), we can rename it to Lockup 1.3.0.

Note the description: Tracking bugs, new ideas and feature requests for Sablier Lockup 1.2.1 which will succeed 1.2.0 (also known as V2.2)

PaulRBerg commented 4 months ago

Sorry, I meant a board.

The name sounds good.

PaulRBerg commented 3 months ago

This feature would mitigate L-07. WithdrawMultiple can be DOS'ed by a random user from the CodeHawk report, wouldn't it?

smol-ninja commented 3 months ago

Yes.