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

[ETHEREUM-CONTRACTS] Relax app registration requirement for callAppAction feature #1038

Closed
hellwolf opened this issue Aug 18, 2022 · 4 comments
Assignees
Labels
Project: PROTOCOL-EVMv1 Superfluid protocol EVM v1 implementation in Solidity Resolution: Duplicate This issue or pull request already exists Resolution: Wontfix This will not be worked on Tag: Idea Raw idea, questions, thoughts and brainstorming notes

Comments

@hellwolf
Copy link
Contributor

To be clarified.

Notes

  • There is no strong security reason why only super app can use callAppAction. So long as a contract trust superfluid host contract, it could also use the feature.
  • Alternative is to push people to use eip-2771 more and let batch call to support eip-2771.
@hellwolf hellwolf added the Tag: Idea Raw idea, questions, thoughts and brainstorming notes label Aug 18, 2022
@hellwolf hellwolf added this to the [email protected] - Giga milestone Aug 30, 2022
@hellwolf hellwolf changed the title [ETHEREUM-CONTRACTS] Relax app registration require for callAppAction feature [ETHEREUM-CONTRACTS] Relax app registration requirement for callAppAction feature Aug 31, 2022
@hellwolf hellwolf added the Team: Protocol Protocol Core, Sentinel, Peripherals, Protocol Infrastructure Tools & DevOps label Aug 23, 2023
@0xdavinchee
Copy link
Contributor

0xdavinchee commented Sep 5, 2023

the reason we didn't do this was likely because of the possibility of calling SuperToken.operationXYZ functions via host.batchCall arbitrarily

@d10r
Copy link
Collaborator

d10r commented Sep 5, 2023

There's methods giving privileged access to the host contract in various places. Some of them (host itself and agreements) are filtered out in the proposed PR. But there's also SuperToken contracts, SuperApps etc. where the host contract has privileged access, and where such a change may make it possible to maliciously impersonate the host.
Enumerating this cases and blocking them may be possible, but this wouldn't be a robust solution.

A cautious solution would be to create a dedicated invoker contract which does the actual call to arbitrary contracts.
That way we don't need to curate a target exclusion list in order to avoid batch operations doing illegitimate calls to methods.

Alongside an operation type which provides a ctx (such that the consuming method can extract the abstracted msgSender), we can then also add an operation type omitting ctx. This would allow calls to arbitrary contract methods with arbitrary argument list.

In order to keep backwards compatibility, we can't switch the existing APP_ACTION operation to an invoker, because that would break existing SuperApps requiring msg.sender to be the host.
So a solution could look like this:

  • leave OPERATION_TYPE_SUPERFLUID_CALL_APP_ACTION as is
  • add OPERATION_TYPE_ANY_CALL: invoked by a helper contract (address provided by host.getAnyCallInvoker()
  • add OPERATION_TYPE_ANY_CALL_WITH_CTX: adds a ctx argument, allowing the consumer to use the relayed msgSender and/or to do agreement calls

@d10r
Copy link
Collaborator

d10r commented Sep 16, 2023

related: #1500

@hellwolf hellwolf added the Project: PROTOCOL-EVMv1 Superfluid protocol EVM v1 implementation in Solidity label Jan 18, 2024
@hellwolf hellwolf removed the Team: Protocol Protocol Core, Sentinel, Peripherals, Protocol Infrastructure Tools & DevOps label Mar 11, 2024
@hellwolf hellwolf added Resolution: Wontfix This will not be worked on Resolution: Duplicate This issue or pull request already exists labels Mar 15, 2024
@hellwolf
Copy link
Contributor Author

Superceded by #1895.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Project: PROTOCOL-EVMv1 Superfluid protocol EVM v1 implementation in Solidity Resolution: Duplicate This issue or pull request already exists Resolution: Wontfix This will not be worked on Tag: Idea Raw idea, questions, thoughts and brainstorming notes
Projects
None yet
3 participants