OpenZeppelin / openzeppelin-contracts-upgradeable

Upgradeable variant of OpenZeppelin Contracts, meant for use in upgradeable contracts.
https://openzeppelin.com/contracts
MIT License
1k stars 436 forks source link

`MulticallUpgradeable` conflicts with `UUPSUpgradeable` #162

Closed julianmrodri closed 1 year ago

julianmrodri commented 2 years ago

There is an issue when combining both MulticallUpgradeable and UUPSUpgradeable in the same contract. This causes a collision with _functionDelegateCall and none of these implement virtual in order to fix the issue.

This forum thread is about the same topic: https://forum.openzeppelin.com/t/right-way-to-extend-both-multicallupgradeable-and-uupsupgradeable/14840

TypeError: Derived contract must override function "_functionDelegateCall". 
Two or more base classes define function with same name and parameter types.

 function _functionDelegateCall(address target, bytes memory data) private returns (bytes memory) {
  ^ (Relevant source part starts here and spans across multiple lines).

We found a workaround which is to use the MultiCall (from the traditional package) and then avoid the delegateCall checks when deploying the implementation. We believe its safe because Multicall only delegates to address(this). The problem is we fill this should be doable using the MulticallUpgradeable for consistency and clarity.

Is it possible to review this and see if we can get it fixed so we can use the MulticallUpgradeable contract? Thanks!

Amxx commented 2 years ago

Hello @julianmrodri

This has been known for a while, and I think we documented it somewhere. IMO this issue is mostly down to solidity language design, has discussed in this issue.

We don't have a fix right now, but thank for reminding us we have to fix this.

frangio commented 2 years ago

Note that this is both a Solidity problem and an OpenZeppelin Upgrades problem. :slightly_smiling_face: Because the plugin identifies library errors with low granularity, if any function in the Address library is used it throws an error from Address.functionDelegateCall, even if that one in particular is not used. See https://github.com/OpenZeppelin/openzeppelin-upgrades/issues/240. This is why we have to introduce the private functions that clash.

julianmrodri commented 2 years ago

Thanks @Amxx and @frangio ... we are in the meantime going to use the traditional Multicall together with UUPSUpgradeable and allowing the unsafe deletegatecall check, because In this case all delegate calls are to address(this) so we dont think it imposes a risk to our implementation/logic contract.

frangio commented 2 years ago

Indeed, that should be fine!

frangio commented 1 year ago

Fixed since OpenZeppelin Contracts v4.9.0 thanks to https://github.com/OpenZeppelin/openzeppelin-upgrades/pull/702.