Rename "onLockupStreamWithdrawn()" hooks to avoid duplicate calls

[smol-ninja]

A note to the reader: The following doesn't affect the deployed version of Sablier contracts.

Context

The recent PR: Make withdraw function callable by any account enables anyone to call withdraw() on any stream. Each withdrawal attempts to call onLockupStreamWithdrawn() on both sender and recipient if they are contracts. As discussed privately on Slack, it raises a concern when the stream has the same sender and recipient. ¹

Problem

We use the same function name onLockupStreamWithdrawn() in both ISablierV2Recipient and ISablierV2Sender. The user is expected to implement onLockupStreamWithdrawn() with no information regarding which interface he wants to implement it for. This leads to duplicate calls on this function when withdraw() is triggered by a third party and the sender and recipient are the same contract addresses.

SablierV2Lockup.sol#L278C1-L297C10

if (msg.sender != recipient && recipient.code.length > 0) {
    try ISablierV2Recipient(recipient).onLockupStreamWithdrawn({.. /* snip */ ..}) { } catch { }
}

if (msg.sender != sender && sender.code.length > 0) {
    try ISablierV2Sender(sender).onLockupStreamWithdrawn({.. /* snip */ ..}) { } catch { }
}

Solutions

Option 1

Rename onLockupStreamWithdrawn()

• to callSenderOnLockupStreamWithdrawn() in ISablierV2Sender
• to callRecipientOnLockupStreamWithdrawn() in ISablierV2Recipient

Because we have other functions OnLockupStreamCanceled() and onLockupStreamRenounced() in the ISablierV2Recipient, we should also consider renaming them to follow the same naming convention.

Option 2

When the sender is the same as the recipient, withdraw() calls on onLockupStreamWithdrawn() only once.

if (msg.sender != recipient && recipient.code.length > 0) {
    try ISablierV2Recipient(recipient).onLockupStreamWithdrawn({.. /* snip */ ..}) { } catch { }
}

if (sender != recipient && msg.sender != sender && sender.code.length > 0) {
    try ISablierV2Sender(sender).onLockupStreamWithdrawn({.. /* snip */ ..}) { } catch { }
}

Footnotes

1. A user provides liquidity on AMM through a multisig and then locks the liquidity by setting up a stream of LP tokens to the same multisig. The multisig implements onLockupStreamWithdrawn() to withdraw liquidity from the AMM every time a withdraw() is triggered. ↩

[PaulRBerg]

Good catch, @smol-ninja, and thanks for the detailed report. I think this problem was introduced in #781 (when the withdrawal hook was added for the sender).

I'm leaning towards option 2 on the basis that it doesn't seem worth it to me to optimize the overall design of the contracts based on an edge case (when sender = recipient). The hook names suggested in option 1 are excessively verbose (and I can't think of any good shorter names).

The implementation might be simpler than in your code snippet - we can just use if/else:

if (msg.sender != recipient && recipient.code.length > 0) {
    try ISablierV2Recipient(recipient).onLockupStreamWithdrawn({.. /* snip */ ..}) { } catch { }
} else if (msg.sender != sender && sender.code.length > 0) {
    try ISablierV2Sender(sender).onLockupStreamWithdrawn({.. /* snip */ ..}) { } catch { }
}

[smol-ninja]

Thanks. I am also inclined towards option 2.

However, in your implementation, did you miss the last else? Because, when msg.sender is neither a recipient nor a sender, it would ONLY trigger the first if i.e. ISablierV2Recipient(recipient).onLockupStreamWithdrawn(). Though the expected result would be to trigger it on both.

[PaulRBerg]

I am also inclined towards option 2.

Glad to hear.

However, in your implementation, did you miss the last else?

Yeah. Anyway - this is fairly complicated and might require careful thinking.

[PaulRBerg]

@smol-ninja could you create an issue out of this discussion, and then close it?

[smol-ninja]

Tracking it with #822.