Avoid duplicate calls to onLockupStreamWithdrawn() hook when sender == recipient

[smol-ninja]

As discussed in #804

^{Originally posted by smol-ninja January 26, 2024}
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.

Problem

We use the same function name onLockupStreamWithdrawn() in both ISablierV2Recipient and ISablierV2Sender. 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 { }
}

Solution

Add a a check whether sender and recipient are same.

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 { }
}

[smol-ninja]

I am facing with a few problems while writing test cases for this.

1. when sender == recipient, how to write a test to not expect a call? A test which should detect the above issue.

2. New concrete tests are needed for hook calls. Incorporating them into withdraw would be difficult because the basis of the withdraw tree structure is based on whether the withdrawal address is stream recipient or not. In the new test, we can have a structure like this:

   1. When the caller is unknown
      • if sender != recipient
        • both hook calls should be expected
      • if sender == recipient
        • only one hook call should be expected
   2. When the caller is the sender
      • if sender != recipient
        • recipient hook calls should be expected
      • if sender == recipient
        • no hook call should be expected
   3. When the caller is the recipient
      • if sender != recipient
        • sender hook calls should be expected
      • if sender == recipient
        • no hook call should be expected

Any thoughts @andreivladbrg?

[PaulRBerg]

@smol-ninja all this is not a matter of internal knowledge at Sablier. You should ask your questions in the Foundry community forums (GitHub discussions, issues, and their Telegram chat groups).

how to write a test to not expect a call?

don't think this is possible but again, let's ask the Foundry team

only one hook call should be expected

By passing the count parameter to vm.expectCall, see:

• https://book.getfoundry.sh/cheatcodes/expect-call?highlight=expectCall#expectcall
• https://github.com/sablier-labs/v2-periphery/blob/3051337fcf2e252481b057afd175828485d39c86/test/Base.t.sol#L168-L179

[smol-ninja]

Thank you for your reply @PaulRBerg.

You should ask your questions in the Foundry community forums

Totally, I agree. I will ask there.

By passing the count parameter to vm.expectCall

Oh yes!

[andreivladbrg]

Any thoughts @andreivladbrg?

for 1. PRB already answered, i.e. count can be used

regarding 2. i think it would be better to have these branches instead, as they are more concise, clearer and shorter:

1. given sender == recipient
   • when caller unknown
     • only one hook call should be expected
   • when the caller is the sender and the recipient
     • no hook call should be expected
2. given sender != recipient
   • when caller unknown
     • both hook calls should be expected
   • when the caller is the sender
     • recipient hook calls should be expected
   • when the caller is the recipient
     • sender hook calls should be expected

all this is not a matter of internal knowledge at Sablier. You should ask your questions in the Foundry community forums

@PaulRBerg are you sure about this statement? this might only be ok for the question "how to write a test to not expect a call?" but, the rest, what does Foundry have to do with how we write our tree structure for the withdraw function?

[PaulRBerg]

@PaulRBerg are you sure about this statement?

Yes. It's always a good idea to talk to the community and correct errors in our understanding. That's the default approach we should take.

https://twitter.com/PaulRBerg/status/1335282362968633344

what does Foundry have to do with how we write our tree structure for the withdraw function?

I wasn't referring to that - I was referring to his questions about how to expect calls

[PaulRBerg]

My wording was bad, though - I shouldn't have said "all this". Sorry for that.

[smol-ninja]

regarding 2. i think it would be better to have these branches instead

I like this more.

My wording was bad, though - I shouldn't have said "all this". Sorry for that

No problem. I understood your point.