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.

Sign up for GitHub

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

Jump to bottom

fix(receive): don't prevent receiving raw ETH #285

Merged
merged 4 commits into from
Nov 5, 2023
Merged

fix(receive): don't prevent receiving raw ETH #285

merged 4 commits into from
Nov 5, 2023

Conversation

Rubilmax
Copy link
Collaborator

@Rubilmax Rubilmax commented Oct 5, 2023

@Rubilmax Rubilmax marked this pull request as ready for review October 5, 2023 09:22
MerlinEgalite
MerlinEgalite requested changes Oct 5, 2023
View reviewed changes
Copy link
Contributor

@MerlinEgalite MerlinEgalite left a comment

Choose a reason for hiding this comment

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

Let's wait cantina's audit and draw our conclusion at this moment?

QGarchery
QGarchery reviewed Oct 5, 2023
View reviewed changes
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.

Actually this PR prevents receiving ETH via raw transfers. Adding a simple receive function would be nice, although maybe not necessary since there is no shortage of payable functions in this contract :D

@Jean-Grimal
Copy link
Contributor

Actually this PR prevents receiving ETH via raw transfers. Adding a simple receive function would be nice, although maybe not necessary since there is no shortage of payable functions in this contract :D

Yes I don't understand the upsides of exposing a receive function

@Rubilmax Rubilmax changed the title fix(receive): don't prevent receiving ETH fix(receive): prevent receiving raw ETH Oct 24, 2023
@Rubilmax Rubilmax changed the base branch from main to review-cantina October 24, 2023 12:55
@Rubilmax Rubilmax force-pushed the fix/remove-receive-require branch from 99e5360 to 699a632 Compare October 24, 2023 12:58
@Rubilmax Rubilmax changed the title fix(receive): prevent receiving raw ETH fix(receive): don't prevent receiving raw ETH Oct 24, 2023
MerlinEgalite
MerlinEgalite requested changes Oct 24, 2023
View reviewed changes
src/WNativeBundler.sol Outdated Show resolved Hide resolved
Jean-Grimal
Jean-Grimal approved these changes Oct 24, 2023
View reviewed changes
Copy link
Contributor

@Jean-Grimal Jean-Grimal left a comment

Choose a reason for hiding this comment

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

I still think that this change should not be applied

@Rubilmax @MerlinEgalite
docs(wnative): update section name
8a3fe2a 
Co-authored-by: Merlin Egalite <44097430+MerlinEgalite@users.noreply.github.com>
Signed-off-by: Romain Milon <rmilon@gmail.com>
@Rubilmax
Copy link
Collaborator Author

Rubilmax commented Oct 24, 2023 •
edited
Loading

I still think that this change should not be applied

I'm ambivalent and I understand why we'd question this change so it's up to discussion

@Rubilmax Rubilmax self-assigned this Oct 26, 2023
@MerlinEgalite MerlinEgalite requested a review from StErMi October 30, 2023 13:38
@StErMi
Copy link

StErMi commented Nov 3, 2023 •
edited
Loading

This change forces any bundler that needs to receive ETH to inherit from the WNativeBundler otherwise it will revert. See for example the compound one (just as an example).

If you are going down this route, both behavior should be documented:

  • If any of the bundler needs to receive ETH it should inherit the WNativeBundler
  • EOA/contracts can now transfer ETH to the bundler directly and should be aware that they have to "spend" such amount (with bundler actions) or transfer it at the end.

What if I want to receive ETH but I don't want to inherit the WNativeBundler extra functions?

@Rubilmax
Copy link
Collaborator Author

Rubilmax commented Nov 3, 2023 •
edited
Loading

What if I want to receive ETH but I don't want to inherit the WNativeBundler extra functions?

The bundler can simply receive ETH via the plenty of payable functions available right?
Otherwise the bundler is not expected to receive ETH from protocols except WETH & CompoundV2 and anyway I believe the feature "receive native ETH" does not make sense without being able to "wrap & unwrap it to WETH"

@openzeppelin-code OpenZeppelin Code
Copy link

fix(receive): don't prevent receiving raw ETH

Generated at commit: 820180b56fb7e87e66e64dadd3e52d7c0c4cdc39

🚨 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

@MerlinEgalite
Copy link
Contributor

@StErMi we added a comment

@MerlinEgalite MerlinEgalite merged commit 9560f33 into review-cantina Nov 5, 2023
@MerlinEgalite MerlinEgalite deleted the fix/remove-receive-require branch November 5, 2023 10:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@QGarchery QGarchery QGarchery approved these changes

@MerlinEgalite MerlinEgalite MerlinEgalite approved these changes

@Jean-Grimal Jean-Grimal Jean-Grimal approved these changes

@StErMi StErMi Awaiting requested review from StErMi

Assignees

@Rubilmax Rubilmax

Labels
☑️ lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Restricted transfer of funds to the bundler
5 participants
@Rubilmax @Jean-Grimal @StErMi @MerlinEgalite @QGarchery