feat(multicall): prevent double initiation

[Rubilmax]

• Fixes Potentially dangerous initiator value #282
• Fixes https://github.com/cantinasec/review-morpho-blue-1/issues/64

[QGarchery]

I don't find any use case to having embedded multicalls, so I'm in favor of this

[MerlinEgalite]

So after a callback you can't anything than just one action?

[Jean-Grimal]

So after a callback you can't anything than just one action?

Yes you are right I didn't think about it. Morpho callbacks don't work anymore (as they call multicall), that's a problem

[Rubilmax]

So after a callback you can't anything than just one action?

Why do you say so?

As you can see, the test suite passes: yes callbacks do execute the multicall, but does not execute the external multicall function, only the internal one

I have highlighted this in the issue

[MerlinEgalite]

Approving but putting this PR on hold

[MerlinEgalite]

I changed the base FYI

[MerlinEgalite]

I think we're missing those 2 things to fix the issue:

1. Unless there is a very specific use case, Morpho should not allow the initiator to re-enter a recursive multicall transaction (that could end up anyway to revert as soon as the second one exits the multicall and the following actions are executed with initiator = address(0)).

2. Morpho should anyway document all these possible worst-case scenarios when the original caller executes multicall operations that interact with arbitrary contracts that could re-enter in a malicious way the Bundler flow.

taken from https://github.com/cantinasec/review-morpho-blue-1/issues/64

For 1. I'm not sure how to prevent in the general while still allowing to use callbacks on Blue.

[Rubilmax]

I think we're missing those 2 things to fix the issue:

1. Unless there is a very specific use case, Morpho should not allow the initiator to re-enter a recursive multicall transaction (that could end up anyway to revert as soon as the second one exits the multicall and the following actions are executed with initiator = address(0)).
2. Morpho should anyway document all these possible worst-case scenarios when the original caller executes multicall operations that interact with arbitrary contracts that could re-enter in a malicious way the Bundler flow.

taken from cantinasec/review-morpho-blue-1#64

For 1. I'm not sure how to prevent in the general while still allowing to use callbacks on Blue.

1 is actually specifically addressed in this PR

Callbacks don't re-enter multicall but use the internal _multicall function

I need to add comments to specify that indeed, some addresses can re-enter the bundler flow in a malicious way

[MerlinEgalite]

Callbacks don't re-enter multicall but use the internal _multicall function

So 1 & 2 are actually addressed in this PR

But the example of the nativeTransfer is not covered no?

[MerlinEgalite]

But the example of the nativeTransfer is not covered no?

Not sure you read it @Rubilmax

[Rubilmax]

But the example of the nativeTransfer is not covered no?

Not sure you read it @Rubilmax

Indeed, I missed it. What do you want me to do about the possible re-entrancy in nativeTransfer? I think it is the purpose of #315

The goal of this PR is to prevent entering multicall specifically ; and it's been what was disclosed in the aforementioned issue
If it is about documenting possible reentrancies, then there is another dedicated issue for it

[MerlinEgalite]

Agree, approving!

[QGarchery]

LGTM, but it does not close https://github.com/cantinasec/review-morpho-blue-1/issues/64 as the description suggests