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

External functions from bundlers can be called by anyone #117

Closed
Rubilmax opened this issue Aug 29, 2023 · 8 comments · Fixed by #169
Closed

External functions from bundlers can be called by anyone #117

Rubilmax opened this issue Aug 29, 2023 · 8 comments · Fixed by #169
Assignees

Comments

@Rubilmax
Copy link
Collaborator

In order to protect users from unexpected behaviors of the bundlers, we may consider protecting external functions with onlySelf:

modifier onlySelf() {
  require(msg.sender == address(this));

  _;
}

However, this would not work with callbacks, because they are expected to be called by specific addresses. In all cases, we know they expected address to call callbacks (Morpho, Balancer, etc), so we can craft a similar, dedicated solution for each callback type

In any case, it doesn't threaten security because the bundler is expected to be stateless between txs

@MerlinEgalite
Copy link
Contributor

MerlinEgalite commented Aug 29, 2023

Adding those modifiers means that we're not sure about the security of bundlers in some way, no?

@Rubilmax
Copy link
Collaborator Author

These modifiers would only have the goal of protecting the user, as I stated in the description of this issue ; I don't see why adding them means we're not sure about the security of bundlers?

@MerlinEgalite
Copy link
Contributor

I'm not sure what you mean by "unexpected behaviors of the bundlers" then. Can you elaborate on that please?

@Rubilmax
Copy link
Collaborator Author

When calling the bundler without going through multicall:

  • _initiator is uninitialized whereas it's used inside functions
  • you can't provide ETH to the call (functions are not payable)

@MerlinEgalite
Copy link
Contributor

MerlinEgalite commented Aug 29, 2023

So there's 2 paths:

  1. Either we write a comment tellings users to pass though the multicall only
  2. Add this modifier to protect users

2 is not necessary but does not add much gas cost either. 1 could be sufficient and does not add more complexity.

@morpho-labs/onchain thoughts?

@pakim249CAL
Copy link
Contributor

I agree with just putting a comment. Users should generally not be interacting with the contracts directly anyway.

@makcandrov
Copy link
Contributor

I agree with Patrick. These contracts are not meant to be used by users who know how to use/write smart contracts anyway

@Rubilmax
Copy link
Collaborator Author

Rubilmax commented Aug 29, 2023

In addition to this, I closed #97 because having dedicated interfaces for each abstract bundler would be error-prone: bundler functions should not be called directly

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 a pull request may close this issue.

4 participants