Feature Request: Granular Control Modifiers in Context.sol Contract

[1anyway]

🧐 Motivation
In the process of developing smart contracts, I've identified a potential enhancement to the Context contract in OpenZeppelin. The goal is to provide more granular control over the execution context, ensuring that certain functions can only be called directly by end-users (Externally Owned Accounts or EOAs) and not by other smart contracts or through proxy contracts. This can be particularly important for functions that deal with sensitive operations or where the intent is to have direct interaction without any intermediaries.

📝 Details
I propose adding three new modifiers to the Context contract:

1. onlyEOAWithoutProxies: Ensures the call is made directly by an EOA and not through any proxies.
2. onlyEOA: Ensures the call is made directly by an EOA, preventing smart contracts from executing certain functionalities.
3. noProxy: Ensures the call is not made through a proxy.

modifier onlyEOAWithoutProxies(address thisAddr) {
        bool cond1 = msg.sender == tx.origin;
        bool cond2 = address(this) == thisAddr;
        require(cond1 && cond2, "Context: call must be direct and without proxies");
        _;
    }

    modifier onlyEOA() {
        require(msg.sender == tx.origin, "Context: caller must be EOA");
        _;
    }

    modifier noProxy(address thisAddr) {
        require(address(this) == thisAddr, "Context: call must not be through a proxy");
        _;
    }

These modifiers provide developers with more tools to control and validate the execution context of their functions, ensuring better security and functionality alignment.
You can find the complete implementation here: https://github.com/1anyway/ValidateContext/blob/main/contracts/Context.sol
I believe these enhancements could be a valuable addition to the OpenZeppelin library, providing developers with more flexibility and security options.

[Amxx]

Hello @1anyway, and thank you for openning this issue.

• About the noProxy, I'm not sure what thisAddr is supposed to represent. We already have something that may be similar in UUPSUpgradeable. The onlyProxy modifier ensure that some operations are only performed through delegation and not directly. This allows us to enforce that the upgradeToAndCall is never called on an implementation, and only on the actual proxies that point to the implementation.

  • In the case of upgradeable contracts, this ensure that the upgrade workflow cannot accidentally selfdestruct the implementation.
  • I'm currious about other usecases for this modifier. What do you ave in mind ?

• onlyEOA: This is something we have strongly opposed in the past, and that we will continue to oppose. We believe smart contracts should be able to perform calls just like EOA. Using such a modifier prevents contract composability, and prevent users using AA to interract with you. This hurt you and your potential users.

• onlyEOAWithoutProxies: this looks like a mix of the two. So answers for the previous points apply here.

[1anyway]

Hello OpenZeppelin team, thank you for the detailed feedback.
thisAddr represent the expected address of the current contract instance, this is used to ensure that the call is not being made through a proxy contract.
I'd like to emphasize that the intention behind introducing these modifiers is to provide options, not mandates. While I understand and respect the philosophy of contract composability and the importance of catering to a broad user base, there are specific use cases and projects that might require a different approach.

For instance, in our applications, we've chosen to impose restrictions against the use of smart contracts for certain functionalities. For security reasons, we also wish to impose restrictions on proxy calls. This is to ensure that our application operates under specific environments and conditions, thereby enhancing its security. This decision is based on our project's unique requirements and security considerations. I believe there might be other projects in the ecosystem that could benefit from having such options readily available in the OpenZeppelin library.

While the Context contract is a foundational piece, perhaps these modifiers could be introduced in an extended version, say ContextExtended or Context2, so that developers have the choice to opt-in based on their project's needs.

[Amxx]

So is thisAddr just an immutable reference to self ?

contract Test {
    address internal immutable thisAddr = address(this); 
    ...
}

[Amxx]

For instance, in our applications, we've chosen to impose restrictions against the use of smart contracts for certain functionalities. For security reasons, we also wish to impose restrictions on proxy calls. This is to ensure that our application operates under specific environments and conditions, thereby enhancing its security. This decision is based on our project's unique requirements and security considerations. I believe there might be other projects in the ecosystem that could benefit from having such options readily available in the OpenZeppelin library.

You have your reasons to prevent smart wallets (AA) to interract with you, and you are free to make that decision.
At our level, we believe that making this decision is wrong, and we don't want to validate or encourage it. That is why we are not providing this.

It has been requested mutliple times, and our answer has always been the same. We think doing so is bad for the ecosystem. By providing this tool we would be "validating" it, and people may see that as OZ recommanding using it. Not providing it is an opinionated decision. We don't take many strong position on how code should be written, but this is one.

[1anyway]

So is thisAddr just an immutable reference to self ?

contract Test {
    address internal immutable thisAddr = address(this); 
    ...
}

yes, you are right

[1anyway]

For instance, in our applications, we've chosen to impose restrictions against the use of smart contracts for certain functionalities. For security reasons, we also wish to impose restrictions on proxy calls. This is to ensure that our application operates under specific environments and conditions, thereby enhancing its security. This decision is based on our project's unique requirements and security considerations. I believe there might be other projects in the ecosystem that could benefit from having such options readily available in the OpenZeppelin library.

You have your reasons to prevent smart wallets (AA) to interract with you, and you are free to make that decision. At our level, we believe that making this decision is wrong, and we don't want to validate or encourage it. That is why we are not providing this.

It has been requested mutliple times, and our answer has always been the same. We think doing so is bad for the ecosystem. By providing this tool we would be "validating" it, and people may see that as OZ recommanding using it. Not providing it is an opinionated decision. We don't take many strong position on how code should be written, but this is one.

I'd like to clarify that my primary concern is not about smart wallets, which I understand are a valuable part of the Ethereum ecosystem. Instead, my focus is on the broader set of smart contracts, especially those that could be used for malicious purposes, such as bots or contracts designed to exploit vulnerabilities.

The ability to restrict certain types of contracts from interacting with our own can be a valuable security measure. By providing tools or modifiers that allow developers to easily implement such restrictions, we can enhance the security of the ecosystem. This isn't about limiting composability or innovation but about giving developers more tools to defend against potential threats.
If the reluctance to provide such tools is based on business reasons, then that's another topic.

[Amxx]

So is thisAddr just an immutable reference to self ?

contract Test {
    address internal immutable thisAddr = address(this); 
    ...
}

yes, you are right

I believe onlyDelegate or notDelegate modifiers, similar to the one we have in UUPSUpgradeable could be a nice addition to the library. I'm not 100% sure it should go in Context, but that is definitely something we will explore for 5.1 !

[1anyway]

So is thisAddr just an immutable reference to self ?

contract Test {
    address internal immutable thisAddr = address(this); 
    ...
}

yes, you are right

I believe onlyDelegate or notDelegate modifiers, similar to the one we have in UUPSUpgradeable could be a nice addition to the library. I'm not 100% sure it should go in Context, but that is definitely something we will explore for 5.1 !

Thank you for considering the suggestion. I've gone ahead and implemented a complete version of the feature. You can check it out here githublink

[1anyway]

I wanted to point out that Uniswap has also utilized the noDelegateCall in their implementation. This serves as a strong use case for its relevance and effectiveness in the ecosystem. Here's the GitHub link to their implementation for your reference.