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

[smol-ninja]

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]

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]

@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]

Will do.

[smol-ninja]

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 2.0.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]

Sorry, I meant a board.

The name sounds good.

[PaulRBerg]

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

[smol-ninja]

Yes.

[smol-ninja]

Unfortunately, there is a problem with this approach. Calls made through try statement are external calls to itself, which means it changes the value of msg.sender and sets it to the Lockup contract itself.

function withdrawMultiple(....) external {
    for (uint256 i = 0; i < streamIdsCount; ++i) {
        try this.withdraw(....) { }
        catch (bytes memory errorData) {
            emit InvalidStreamIdInWithdrawMultiple(streamIds[i], errorData);
        }
    }
}

is not same as

function withdrawMultiple(....) external {
    for (uint256 i = 0; i < streamIdsCount; ++i) {
        withdraw(....) { }
    }
}

The latter does not change the value of msg.sender.

1. Therefore, this approach does not work for cancelMultiple and renounceMultiple because msg.sender will never be equal to sender. So all calls will fail.

2. For withdrawMultiple, this will work but the following statement can no longer be true: "This function attempts to call a hook on the recipient of each stream, unless msg.sender is the recipient."

The hook call is skipped when msg.sender is the recipient. However, in this case, if the recipient makes a call to withdrawMultiple, the msg.sender would change which means it will make successful calls to onSablierLockupWithdraw hook. What is the reason that we do not allow hook calls when msg.sender is the recipient?

Having said that, I have tested try..catch statement inside withdrawMultiple and it works as expected. It also solves the DDOS'ing problem with withdrawMultiple as reported by Codehawk.

Thoughts? Should I do it? Its a very small change.

[andreivladbrg]

Unfortunately, there is a problem with this approach. Calls made through try statement are external calls to itself, which means it changes the value of msg.sender and sets it to the Lockup contract itself.

ohh, correct, it seems like a big problem to me

The hook call is skipped when msg.sender is the recipient. However, in this case, if the recipient makes a call to withdrawMultiple, the msg.sender would change which means it will make successful calls to onSablierLockupWithdraw hook. What is the reason that we do not allow hook calls when msg.sender is the recipient?

can't this approach (run hook on recipient when the caller is the recipient) cause reentrancy issues?🤔

Having said that, I have tested try..catch statement inside withdrawMultiple and it works as expected. It also solves the DDOS'ing problem with withdrawMultiple as reported by Codehawk.
Thoughts? Should I do it? Its a very small change

given the time constraints and the edges that this PR can have, i am of the opinion to not include it in 2.0.0

[PaulRBerg]

Thanks for looking into this, Shub!

We don't allow hook calls when the recipient makes the withdrawal because they are redundant. The recipient knows that they made the withdrawal, so there's no point in notifying them about that.

Performing the hook calls in withdrawMultiple (but for the 1st one) would not be technically incorrect, but it might lead to vulnerabilities/ issues in hooks where the implementor assumes that only non-recipient-triggered withdrawals call hooks.

Also, what Andrei said — withdrawing to a third-party address would no longer be possible.

So I don't think we should include this in V2.0.0

[PaulRBerg]

reentrancy issues?🤔

Reentrancy is an issue only if CEI is not respected.

[smol-ninja]

Thank you to both of you for your comments.

withdrawing to a third-party address would no longer be possible

withdrawMultiple does not allow withdrawing to third parties so its not a problem.

Nevertheless, I spent some time to work on this: #1101. Feel free to discard the PR if both of you wants to exclude it from the next release. I would still suggest you to have a look at the PR before making the decision. I don't think there will be any problem with msg.sender context getting changed when making calls through try catch in withdrawMultiple. Withdraws are public functions anyways so anybody can call them.

The use of try..catch and not getting reverts in case of an invalid stream ID seems to be a good feature for users.

[PaulRBerg]

Thanks Shub, good work so far. I pushed a commit to improve the writing in #1101.

However, I'm against the PR as it currently stands because it introduces a new, important assumption — that the Sablier Lockup protocol itself can act as the msg.sender for withdrawals. This can be problematic for hook implementations that perform withdrawals because they may assume that they are not called back. And AFAIK, the EarnM integration does exactly that.

How about delegate-calling and checking the boolean result instead of using try/catch?

[andreivladbrg]

important assumption — that the Sablier Lockup protocol itself can act as the msg.sender for withdrawals. This can be problematic for hook implementations

agree with this

How about delegate-calling and checking the boolean result instead of using try/catch?

would this be different from the batch function? or is it simply to have a function called withdrawMultiple?

[smol-ninja]

important assumption — that the Sablier Lockup protocol itself can act as the msg.sender for withdrawals. This can be problematic for hook implementations. This can be problematic for hook implementations that perform withdrawals because they may assume that they are not called back.

Its correct that they will be called back in this case. If thats an important assumption, I agree with discarding the PR as well.

On delegate call, that can work. @andreivladbrg it would be different because in batch, the entire tx is reverted if one of the txns revert. Here, we do not want to revert.

[smol-ninja]

I have pushed a new commit using delegatecall. LMK what you think about it. #1101