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

feat(protect): add onlyInitiated modifier #313

Merged
merged 10 commits into from
Nov 8, 2023

Conversation

@Rubilmax Rubilmax marked this pull request as ready for review October 23, 2023 12:33
@Rubilmax Rubilmax linked an issue Oct 23, 2023 that may be closed by this pull request
@Rubilmax Rubilmax force-pushed the feat/only-initiated branch 2 times, most recently from 38aafd8 to 2f9f492 Compare October 24, 2023 08:00
@Rubilmax Rubilmax force-pushed the feat/only-initiated branch from 2f9f492 to c38104f Compare October 24, 2023 13:03
@MerlinEgalite
Copy link
Contributor

@QGarchery @MathisGD can one of you review this PR?

@Rubilmax Rubilmax self-assigned this Oct 26, 2023
@Rubilmax Rubilmax linked an issue Oct 26, 2023 that may be closed by this pull request
@MerlinEgalite
Copy link
Contributor

@Rubilmax can you update this branche please?

Copy link
Collaborator Author

@Rubilmax Rubilmax left a comment

Choose a reason for hiding this comment

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

Ideally this one is merged among the last ones, but I reviewed still

Copy link
Contributor

@QGarchery QGarchery left a comment

Choose a reason for hiding this comment

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

Many conflicts, sorry :/

Copy link

openzeppelin-code bot commented Nov 1, 2023

feat(protect): add onlyInitiated modifier

Generated at commit: 0a052d8e36b46d8abb6ccaec7b660a2bcba1af13

🚨 Vulnerabilities Summary

Process Issues Results
Contract Inspector note
low
critical
Total
20
10
1
31
Dependency Checker Total 0

For more details view the full report in OpenZeppelin Code

Copy link
Collaborator Author

@Rubilmax Rubilmax left a comment

Choose a reason for hiding this comment

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

LGTM

@MerlinEgalite
Copy link
Contributor

Conflicts to fix cc @Rubilmax

Copy link
Contributor

@QGarchery QGarchery left a comment

Choose a reason for hiding this comment

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

LGTM except one point on native transfers that I think we should discuss.
The goal of the onlyInitiated modifier is to:

  1. explicit the error in case of an accidental usage of the bundler. In most cases it is not a safety measure, because the UNSET_INITIATOR address would be used instead, which would lead to a revert anyway.

  2. The onlyInitiated modifier is also used in the WNativeBundler. Here I assume that the reasoning is that native tokens could be used without having been sent by a previous transfer in the same multicall. Indeed, without the modifier, one could make a direct call to wrapNative with a positive msg.value and it would result to lost funds most likely (wrapped tokens could be skimmed by anyone). In this case it is a safety measure against accidentally sending native tokens.

I think that the goal 2 is not consistent in the current state, because of the following functions:

  • wrapNative is gated by onlyInitiated, which is useful against accidental native transfers
  • unwrapNative is unnecessarily gated, because it already assumes that funds are on the bundler. It is similar to the wrap/unwrap stETH functions (which are not gated)
  • stakeEth is not gated, but it would be useful against accidental native transfer, similarly to wrapNative

I think we can either:
a. remove goal 2 entirely, by removing the onlyInitiated modifier to the functions mentioned above
b. implement goal 2 everywhere. Notice that we could also gate the receive function if we go that route

@Rubilmax
Copy link
Collaborator Author

Rubilmax commented Nov 6, 2023

Thanks @QGarchery for pointing out that the change wasn't consistent. I removed onlyInitiated from wrapNative & unwrapNative. We'll move to (b) in #354 , in addition to protect from reentrancies

@Rubilmax Rubilmax requested a review from QGarchery November 6, 2023 12:46
@@ -52,7 +51,6 @@ abstract contract WNativeBundler is BaseBundler {
}

/// @notice Unwraps the given `amount` of wNative to the native token.
/// @notice Warning: should only be called via the bundler's `multicall` function.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I still remove this comment because in this case we're not using initiator(). The goal of this comment wasn't to avoid losing funds, otherwise this comment should be duplicated on all functions. In any case, #354 will protect the user from everything

@Rubilmax Rubilmax force-pushed the feat/only-initiated branch from 51a250b to 0a052d8 Compare November 6, 2023 13:27
@MerlinEgalite MerlinEgalite merged commit c57705a into review-cantina Nov 8, 2023
8 checks passed
@MerlinEgalite MerlinEgalite deleted the feat/only-initiated branch November 8, 2023 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Potentially misleading warning comments Quantify _checkInitiated in actions that require initialization
5 participants