search

mudgen / diamond-1-hardhat

EIP-2535 Diamond reference implementation using Hardhat and Solidity 0.8.*
MIT License
135 stars 84 forks source link

DiamondLoupeFacet required, yet not added in constructor? #3

Closed webthethird closed 2 years ago

webthethird commented 2 years ago

Hi @mudgen, this is mostly just a question for you, and may not be a big issue.

I noticed the following language in this and the other reference implementations:

Note: The loupe functions in DiamondLoupeFacet.sol MUST be added to a diamond and are required by the EIP-2535 Diamonds standard.

If this is the case, why is the DiamondLoupeFacet not being added in the constructor, as is done with the DiamondCut facet?

I am a grad student and researcher working on a survey of current upgradeability patterns, so it would be great to connect with you!

-Bill

mudgen commented 2 years ago

Hi Bill, it would be great to connect with you.

I think it is a good idea to add the DiamondLoupeFacet functions in the constructor. It just isn't in this implementation.

The DiamondLoupeFacet functions are added to the diamond during deployment, right after the diamond proxy is deployed. The deployment script is here: https://github.com/mudgen/diamond-1-hardhat/blob/main/scripts/deploy.js

webthethird commented 2 years ago

I see that now, thanks!

In my research I am using the static analysis tool Slither to a) determine if a given contract is a proxy, b) determine if it is upgradeable, and c) extract relevant features in order to classify it according to the various patterns out there. So if we come across a Diamond in our data set but cannot find the Loupe functions in any of the (facet) contracts in the compilation unit, it would be correct to flag this as an issue right?

Also, given the fact that Diamonds can be used to create immutable contracts as well, would it be sufficient to check the constructor to determine whether a FacetCut with the function selector IDiamondCut.diamondCut.selector is being added? As I type this I think probably not, since I suppose a (non-standard) diamond could use a different name for the function which can add/remove/replace facets. So then we would presumably need to trace each function selector from the constructor to that function's implementation, then search it to see if it ever calls LibDiamond.addReplaceRemoveFacetSelectors. Does that sound right to you?

mudgen commented 2 years ago

So if we come across a Diamond in our data set but cannot find the Loupe functions in any of the (facet) contracts in the compilation unit, it would be correct to flag this as an issue right?

Yes, that is correct.

So then we would presumably need to trace each function selector from the constructor to that function's implementation, then search it to see if it ever calls LibDiamond.addReplaceRemoveFacetSelectors. Does that sound right to you?

Well maybe, someone could use or write a different internal function with another name that adds/replaces/removes functions.

webthethird commented 2 years ago

Well maybe, someone could use or write a different internal function with another name that adds/replaces/removes functions.

Yeah, I think I'll need to track down the mapping where the facets are actually stored and determine if any of the facets added in the constructor can modify that mapping, whether it be via LibDiamond.addReplaceRemoveFacetSelectors or any other function.

Thanks for the insights! Closing this as it's not really an issue. If I have more questions, what's the best way to reach out? Twitter?

mudgen commented 2 years ago

Yes, twitter, or contact me on Discord. My user name is mudgen#7418