Implementation() conflict with UpgradeableBeacon and TransparentUpgradeableProxy

Hello! Ivan M (Kiwi) from NFTX here.

We are currently working on NFTX V2 and have decided to implement beacon proxies into our system with a our vault factory serving as a beacon for the proxies.
However, we have encountered problems with etherscan detecting the implementation, because of a function conflict.

UpgradeableBeacon.sol and TransparentUpgradeableProxy.sol both use the function implementation(), however, this functions naming on UpgradeableBeacon.sol may be misleading.
For example, on TransparentUpgradeableProxy.sol and most other proxies, the implementation() function returns the implementation address that the proxy uses. But, in UpgradeableBeacon.sol, this function returns the implementation of the BeaconProxys that follow the Beacon.
This seems to me as an unintended difference between the two, and I propose renaming the implementation() function in UpgradeableBeacon.sol to something that fits its usage better. Such as beaconProxyImplementation() or childImplementation(). This way it no longer conflicts with other proxies, and the usage is quite clear.

:1234: Steps to reproduce

  1. Use a contract with UpgradeableBeacon.
  2. Deploy an UpgradeableBeacon using a TransparentUpgradeableProxy.
  3. Call the implementation() function.
  4. Notice the returned result is the implementation for the BeaconProxy, not the TransparentUpgradeableProxy.

I hope this post details the problem clearly and that a solution is considered :slightly_smiling_face:
Thank you!

1 Like

Thanks for reporting this @IvanTheGreatDev!

So the problem here is that Etherscan wrongly thinks this is a transparent proxy because they see an implementation() function in the ABI?

Can you give more detail about the practical consequences of this? Do you have an example Etherscan link we could look at?

Hey Francisco, I’m working along side Kiwi and might be able to shed more light on the issue.

I noticed the other day that etherscan and tenderly were reporting different implementation addresses for one of the proxy contracts we have deployed, so I redeployed to kovan and the same thing happened.

EDIT: the tenderly implementation address is the correct implementation address

Etherscan: https://kovan.etherscan.io/address/0xcB62303a5Ecc5f9C5cF7B5AA967a25d9Bb2B4b08#readProxyContract

Tenderly: https://dashboard.tenderly.co/contract/kovan/0xcb62303a5ecc5f9c5cf7b5aa967a25d9bb2b4b08

Image of deploy script:

Yes essentially, and while this could be an error on Etherscans part, I think the root of the problem is that the function implementation() on UpgradeableBeacon.sol is misleading since it isn’t really the implementation of that contract, but the implementation of the child proxies.

I’m guessing Tenderly checks the storage slot directly for the implementation, while Etherscan tries the function?

1 Like

Ok so I did not understand at the beginning that we were talking about an UpgradeableBeacon behind a transparent proxy. This is a setup that I had never considered!

Can you expand on that a little? Why are you making the beacon itself upgradeable? The beacon is meant to be a simple contract so I don’t see a big reason to make it upgradeable.

As for the implementation() conflict, this is a little unfortunate but we can’t do anything about it as we now need to keep backwards compatibility. :confused: Tenderly is doing the right thing by reading from the storage variable. I think Etherscan is also aware of the storage variable but they must be trying the implementation() function first and using its return value. Because of the transparency property that is going right through to the beacon.

1 Like

We have a factory contract for NFTX vaults and we have the factory hold the beacon proxy implementations. (inherits UpgradeableBeacon).

Ok! Understandable about staying backwards compatible. We will rename the function locally and will make sure to email Etherscan!
I do think the function name is currently misleading/inaccurate though. Since it isn’t the contracts implementation itself, but the child proxies. Would it be possible for this change to be made on the next breaking update?

Thank you!

2 Likes

I agree that the function name can be confusing if interpreted in the same way as a more traditional proxy.

The backwards compatibility is a big constraint though, I’m not sure it would be a good idea for us to make this change now, unfortunately.