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

Using delegatecall within view functions, to implement proxies #14577

Closed
hoytech opened this issue Sep 26, 2023 · 5 comments
Closed

Using delegatecall within view functions, to implement proxies #14577

hoytech opened this issue Sep 26, 2023 · 5 comments
Labels
closed due inactivity The issue/PR was automatically closed due to inactivity. feature stale The issue/PR was marked as stale because it has been open for too long.

Comments

@hoytech
Copy link

hoytech commented Sep 26, 2023

Abstract

With some types of proxy contracts, functions invoke delegatecall to execute the code stored in a separate implementation contract. However, these functions cannot be declared as views because delegatecall cannot be used within view functions. This means that the exported ABIs for these functions will be (ie) nonpayable, resulting in contracts invoking these functions with call instead of staticcall, and causing these functions to be incorrectly categorised by smart contract wallets, block explorers, etc.

Motivation

There are several work-arounds to this issue, most of which are discussed in these StackOverflow answers by @k06a : thread 1 and thread 2. Another hack involves post-processing the compiled ABI. Unfortunately, these work-arounds all involve a run-time cost, and/or don't actually mark the functions as views in the ABI output by the compiler itself.

The "best" work-around I have found is described in the first StackOverflow thread above. In this approach, the view function staticcalls to address(this), which is then handled by a special non-view function that delegatecalls the implementation. This results in the functions having view state mutability in the output ABI, at the expense of some extra run-time gas usage. (Note that msg.sender is lost, but this typically does not matter for view functions.)

This approach works because if a caller staticcalls a contract that invokes delegatecall, it will succeed as long as the called contract, and all contracts it invokes, do not actually attempt any state modifications. This demonstrates that in principle we could address this issue purely by making a change to the solidity compiler (and no EVM changes are needed).

Specification

I did a few small experiments to feel out the possibilities for how this could be addressed.

Option A

Patch / test

This trivial patch adds a delegatecallToView function to address. This works exactly the same as delegatecall except that it is considered to have view state mutability instead of nonpayable:

function myView() external view returns (uint) {
    (bool success, bytes memory ret) = module.delegatecallToView(msg.data);
    require(success);
    return abi.decode(ret, (uint));
}

This solves all the issues mentioned above, but has the down-side of preventing the compiler from statically verifying that no state modifications will be attempted when invoking view functions. In the broad sense, I don't see this as a problem because the actual decision about whether to interpret the function as a view is done by the caller by choosing to invoke it with staticcall or not.

That said, there may very well be scenarios where the static verification is important, and I would like to learn more about these if so. The only case I can really think of is if you are interacting with a contract that has been verified on etherscan and its ABI says view, you can be sure that state modifications will not happen even if you use call instead of staticcall for some reason.

Option B

Patch / test

This patch is slightly more complicated but still pretty minimal. It adds support for a special viewable modifier. This is not a state mutability specifier. All it does is override the function's stateMutability field to be view in the ABI output:

function myView() external viewable returns (uint) {
    (bool success, bytes memory ret) = module.delegatecall(msg.data);
    require(success);
    return abi.decode(ret, (uint));
}

The above function is compiled with default state mutability (ie nonpayable), which means that delegatecall is allowed. However, at ABI export-time its stateMutability is overridden to be view. The modifier is called viewable to indicate that these functions are intended to be invoked with staticcall, even though the compiler has not been able to statically verify that it won't attempt state modifications.

Typically you would use this function with no state mutability specifier and then perform a delegatecall within the function. If you do not do a delegatecall (or some state changing operation) then you will receive a warning that this function can be made a view instead.

One drawback of the patch in its current form is that when importing full contracts into a compilation unit, the external ABI is not used, so viewable functions will be invoked with call instead of staticcall. This can be worked around by instead using an interface where the functions are labeled as views.

This patch needs a bit more polish before I'd consider it ready. Testing, obviously, but also things like throwing errors when combining viewable with view/pure, specifying viewable multiple times, etc.

Option C

I did not try implementing this, but viewable could itself become a state modification specifier, somewhere "in-between" nonpayable and view.

This would probably solve the "compilation unit" issue mentioned in the previous section, but I think if we went this route then we may as well make function capabilities more general/granular, for example as described here. Personally I would selfishly prefer a focused solution for this specific issue.

Backwards Compatibility

Option A would change the static-verification guarantee implied by view, which may or may not cause problems (I'd like to learn more about this!).

Option B could break contracts that define their own modifiers named viewable.

I haven't thought enough about Option C to say one way or the other.

@k06a
Copy link

k06a commented Sep 27, 2023

We could have delegatestaticcall in Solidity, which will be compiled into the same delegatecall but would be allowed to use it inside of the view methods.

@Derixtar54

This comment was marked as spam.

@hoytech
Copy link
Author

hoytech commented Sep 27, 2023

We could have delegatestaticcall in Solidity, which will be compiled into the same delegatecall but would be allowed to use it inside of the view methods.

I think this is my "Option A". The patch for this seems quite trivial, as long as nobody identifies any additional issues with it. delegatestaticcall is a good name, although perhaps could confuse people into thinking its state mutability is actually run-time restricted like with staticcall.

Copy link

github-actions bot commented Jan 2, 2024

This issue has been marked as stale due to inactivity for the last 90 days.
It will be automatically closed in 7 days.

@github-actions github-actions bot added the stale The issue/PR was marked as stale because it has been open for too long. label Jan 2, 2024
Copy link

github-actions bot commented Jan 9, 2024

Hi everyone! This issue has been automatically closed due to inactivity.
If you think this issue is still relevant in the latest Solidity version and you have something to contribute, feel free to reopen.
However, unless the issue is a concrete proposal that can be implemented, we recommend starting a language discussion on the forum instead.

@github-actions github-actions bot added the closed due inactivity The issue/PR was automatically closed due to inactivity. label Jan 9, 2024
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Jan 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed due inactivity The issue/PR was automatically closed due to inactivity. feature stale The issue/PR was marked as stale because it has been open for too long.
Projects
None yet
Development

No branches or pull requests

3 participants