Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Try catch authorization (alternative 1) #49

Closed
wants to merge 4 commits into from

Conversation

MerlinEgalite
Copy link
Contributor

Fixes #46

@MerlinEgalite MerlinEgalite marked this pull request as ready for review August 15, 2023 13:08
This was referenced Aug 15, 2023
@@ -63,7 +64,13 @@ abstract contract MorphoBulker is BaseBulker, IMorphoBulker {
function morphoSetAuthorizationWithSig(Authorization calldata authorization, Signature calldata signature)
external
{
MORPHO.setAuthorizationWithSig(authorization, signature);
try MORPHO.setAuthorizationWithSig(authorization, signature) {
Copy link
Contributor

@pakim249CAL pakim249CAL Aug 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not convinced that skipping the revert is a good idea in the bulker. For example, if there were two bulk transactions like this in the mem pool:

1.1. Set authorization to true
1.2. actions
1.3. Set authorization to false

2.1. Set authorization to true
2.2. actions
2.3. Set authorization to false

Then if 1.1 if frontrun, the second set of tx should probably revert. But it would not with this implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understood the pb. Why would I submit 2 bundles at the same time? Missclicked?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is no perceived problem with this by the team, then I'm okay letting it go. But I think it's important to point out that this kind of behavior could happen, and the code would not operate as intended under circumstances in which multiple sets of transactions are in the mem pool.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we don't do that then you could prevent an entire bundle from happening by frontrunning the tx. I'd say this problem seems more likely to occur than the one you mentioned but I might be wrong tbh

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I agree that it is more likely that the tx gets frontrun. However, I think that signatures being frontrun and having the tx revert is a much lower severity problem than unintended interactions NOT reverting. I'm not entirely sure what attack vectors exist from the problem I point out, but the problem of a frontrun signature is much easier to define. Just something to consider.

@MerlinEgalite MerlinEgalite deleted the refactor/handle-auth branch November 9, 2023 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

try/catch on set authorization
3 participants