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

Annotate MulticallUpgradeable to allow delegatecall for Upgrades Plugins #3843

Closed
ericglau opened this issue Dec 1, 2022 · 0 comments · Fixed by #3961
Closed

Annotate MulticallUpgradeable to allow delegatecall for Upgrades Plugins #3843

ericglau opened this issue Dec 1, 2022 · 0 comments · Fixed by #3961
Milestone

Comments

@ericglau
Copy link
Member

ericglau commented Dec 1, 2022

🧐 Motivation
When MulticallUpgradeable is inherited in an upgradeable contract, using the contract with Upgrades Plugins would cause a validation error with

@openzeppelin/contracts-upgradeable/utils/MulticallUpgradeable.sol:41: Use of delegatecall is not allowed
    https://zpl.in/upgrades/error-002

However, the delegatecall in this context is not necessarily unsafe.

To get around this, users would currently need to provide an option to allow delegatecalls for the entire validation run, which could hide issues elsewhere.

📝 Details
MulticallUpgradeable contains a delegatecall, but the target of that delegatecall is always address(this). This delegatecall can be considered safe as long as the contract including its dependencies does not have any exposed functions with selfdestruct.

Consider annotating MulticallUpgradeable with @custom:oz-upgrades-unsafe-allow delegatecall so that Upgrades Plugins will not raise a validation error for the delegatecall. Note that the Upgrades Plugins will still perform validations to ensure that the contract does not have selfdestructs.

@frangio frangio added this to the 4.9 milestone Jan 5, 2023
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.

2 participants