loading...

Addressing Josselin Feist's Concern's of EIP-2535 Diamond Standard

mudgen profile image Nick Mudge Updated on ・11 min read

Josselin Feist, from Trail of Bits, wrote an article with a number of concerns about EIP-2535 Diamond Standard and an implementation of it. I'll address concerns he brought up and correct some things along the way.

One thing to mention is that EIP-2535 Diamond Standard and its implementations and tools and information is a community effort. There is a growing group of people making the standard and its growing ecosystem what it is.

I invite Trail of Bits and other people to help fill in the gaps of information, tests, procedures, ideas and tools to help make diamonds and upgrades better and easier to use for everyone. Some of the things mentioned below are good opportunities to contribute value to diamonds and their use.

Josslin says an upgradability standard should have the following things:

A clear, simple implementation. Standards should be easy to read to simplify integration with third-party applications.

Understood, this has been done with the diamond-1 implementation.

A thorough checklist of upgrade procedures. Upgrading is a risky process that must be thoroughly explained.

A checklist and more published information of this kind is valuable and welcome.

On-chain mitigations against the most common upgradeability mistakes, including function shadowing and collisions. Several mistakes, though easy to detect, can lead to severe issues.

The standard prevents function selector clashes. Diamond storage helps prevent contract storage corruption or collusions. Tools and ways to do this are welcome. There is also information about upgradeable contracts and their pitfalls that can be found online.

A list of associated risks. Upgradeability is difficult; it can conceal security considerations or imply that risks are trivial.

Any such information would be valuable and I welcome such contributions.

Tests integrated with the most common testing platforms. The tests should highlight how to deploy the system, how to upgrade a new implementation, and how an upgrade can fail.

The diamond reference implementations have a suite of tests to test them. More tests from more platforms are welcome. Also, people can easily write their own tests for diamonds because they are easy to test. The reference diamond implementations and other implementations show ways to deploy and upgrade diamonds. Articles and tutorials and libraries are welcome.

hardhat-deploy provides builtin support for deploying and upgrading diamonds and includes documentation about it.

Contract Standardization

One of the main purposes of a contract standard is to provide standard interfaces with implementation details so that implementations can be made that interoperate with other contracts and tools and software.

EIP-2535 Diamond Standard does this by providing a standard function diamondCut for upgrades, a standard event DiamondCut for recording what functions are added/replaced/removed and 4 standard functions that are used to show what functions a diamond has.

The standard provides information about how to implement these functions and other useful information. All of this is useful and being used in development and production successfully.

Diamond Implementation

Josselin shows part of an implementation of the diamondCut function that is used for upgrades. In several places he mentions that gas-optimizations make the code more complicated:

Additionally, much of the code complexity is due to optimization in locations that don’t need it.

The gas-optimizations that were made in that implementation save gas and it is really up to the people building an application whether or not the gas optimizations are needed or wanted or not.

Besides that the code shown for the upgrade function was removed and replaced by simplified code 7 weeks before the date Josselin's article was published. The code he shows does not exist in any of the current diamond reference implementations. I have asked Josselin to update his article with code from a current implementation.

Josselin was hired from Trail of Bits to help us find bugs and improve our code. And he did and we fixed and improved things. Then he published an article featuring our old, unfixed code to represent EIP-2535 Diamond Standard. To be accurate he should provide information and code for current implementations.

Diamond Terminology

  • Diamond A contract that gets its external functions from other contracts called facets. A diamond has a fallback function that delegates function calls to facets. A diamond implements the Specification section of EIP-2535 Diamond Standard.
  • Facet A contract that supplies external functions to a diamond. In the diamond industry a facet is a side of a diamond.
  • Loupe Four standard functions that show what functions and facets a diamond has. In the diamond industry a loupe is a magnifying glass that is used to look at diamonds.
  • Upgradeable Diamond A diamond that has the diamondCut external function so functions can be added/replaced/removed.
  • Finished Diamond A diamond that was an upgradeable diamond but then its diamondCut function was removed so functions can no longer be add/replaced/removed. It can't be upgraded anymore.
  • Single Cut Diamond A diamond that adds all of its functions and facets in its constructor function, and does not add the diamondCut or other upgrade function. A single cut diamond is fully formed upon deployment, is immutable and can't be upgraded.

Josselin says:

EIP 2535 introduces “diamond terminology,” wherein the word “diamond” means a proxy contract, “facet” means an implementation, and so on. It’s unclear why this terminology was introduced.

Let me clear this up.

Originally EIP-2535 Diamond Standard was called the Transparent Contract Standard, but that name clashed with something from OpenZeppelin and they asked me to change the name.

When making transparent contracts it was noticed that each of its facets/implementations could handle a different aspect or set of functionality and it was useful to organize code this way. It was like each facet/implementation was a different side or facet of a transparent contract.

Physical diamonds have facets that share a common center. EIP-2535 diamonds have facets that share the same contract storage space and a common Ethereum address.

The terminology helps conceptualize how diamonds work and how to build applications with them.

The Loupe Functions

The loupe functions are four standard functions defined by EIP-2535 Diamond Standard that return what functions a diamond has and the facet addresses they come from.

The verified source code of a diamond does not show what functions it has or where it gets them. And a diamond's ABI doesn't include what functions it has. To help get this information the DiamondCut event or the loupe functions are used.

One of the loupe functions is facetFunctionSelectors. Josselin says this about it:

facetFunctionSelectors returns all the function selectors of an implementation. This information will only be useful for off-chain components, which can already extract the information from the contract’s events. There’s no need for such a feature on-chain

Currently the loupe functions are being used in unit tests to test diamonds. They are being used in code that deploys and upgrades diamonds. They are used by hardhat-deploy to deploy and upgrade diamonds.

It is true that DiamondCut events can be queried and processed to get the same data that the loupe functions return. This means there are two ways to get data about diamonds. Both ways are good ways to get data about diamonds and either can be used.

So why have two ways? Because in some cases it will be easier and better to use function calls, and in other cases it will be easier and better to use events.

For example a blockchain explorer website may want to use events to show facet and function information about diamonds because it is already using event tooling and infrastructure to capture and process events for contracts.

An upgrade script may want to check that functions don't already exist in a diamond before trying to add them. Using the facets loupe function for this is simple and easy.

A simple static website can show information about any diamond by calling the loupe functions and then using the returned data to query services like Etherscan to get and show verified source code, ABI information and other information. This could be done with events, but it would be easier, simpler and probably run faster if done with loupe functions.

Having loupe functions and events give people and software choice to use what is best for their use.

There is also a question about reliability. In all cases querying and processing events may not be as reliable, available and performant as contract function calls. Likewise, a connection to an Ethereum node may not always be available in every system but there may be a connection to an events database that can be used to retrieve diamond information. Having both events and loupe functions makes diamonds robust.

The loupe functions add some complexity to diamonds. Generally it is a good idea to push complexity out of contracts into off-chain software. But every case is different and requires evaluation. In the case of diamonds there is another principle which is optimize for simplicity for common use cases. A diamond is implemented and deployed once but will be queried many times and may be used by or integrated with a lot of software. The loupe functions are a simple and direct way to query diamonds and integrate with software.

Some people have suggested the idea of making the loupe functions optional in the standard. The problem with that is it would break interoperability. Some diamonds would work with some software and not with other software.

I wrote another article about loupe functions that provides more information: Diamonds Loupe Functions

Understanding What Complexity Loupe Function Add to A Diamond

Let's discuss the diamond-1 implementation to understand what complexity the loupe functions add to see if they are worth it.

To support the loupe functions the diamond-1 implementation stores function selectors in the ds.selectors array.

  1. Adding new functions to a diamond. New function selectors are added to the ds.selectors array. The position of each new selector in the array is also stored so the functions can possibly be removed later.

  2. Replacing functions There is no code or logic to support diamond loupe functions when replacing functions because none are needed.

  3. Removing functions Function selectors are removed from the ds.selectors array. This is done in the usual way items are removed from arrays in contracts. The item to remove is replaced with the last item in the array and the last item in the array is popped off (deleted).

That's it for adding/replacing/removing functions. Now we move on to the four standard loupe functions.

The loupe functions are read-only, view functions. This immediately simplifies things because you never have to worry about your loupe functions modifying anything.

The loupe functions loop over the ds.selectors array mentioned above to get various information and return it. This requires some amount of logic because the diamond-1 implementation stores little data for the loupe functions to work with. The loupe functions are implemented in their own DiamondLoupeFacet.

The diamond-3 implementation is implemented in a similar way as diamond-1 but in addition to storing selectors in an array it also stores facet addresses in an array. This simplifies the loupe functions which are each implemented in a few lines of very simple code.

In all three reference implementations the loupe functions are isolated in their own facet, so they do not affect the complexity of an application's code. In addition the DiamondLoupeFacet facet can be deployed once and be reused by any number of diamonds.

Addressing More Things in the Article

Many Interfaces

While the proposal only needs a lookup table, from the function signature to the implementation’s address, the EIP defines many interfaces that require storage of additional data:

Josselin says the EIP "defines many interfaces". If you look at EIP-2535 you will see that the EIP only defines two interfaces. Only one of them requires storage of additional data, which is the one he shows in his article, IDiamondLoupe. As mentioned above an array is used to store the function selectors used by a diamond. And the position of each selector in the array is stored. This is necessary for the loupe functions.


Safe Struct Storage Pointers with Diamond Storage

EIP-2535 Diamond Standard shows and encourages a contract storage technique called Diamond Storage.

Diamond Storage enables different facets to share or use their own contract storage space within a diamond and access it with a namespace. The namespace is a human-readable and meaningful string that represents the area of data. For example "com.mywebsite.myproject.mytoken". The string is hashed and used as a pointer to a struct in contract storage. More information about this and examples are in this article: How Diamond Storage Works.

There is no risk of accidentally or maliciously colliding diamond storage with other contract storage because of using hashes of unique and meaningful strings for struct storage pointers.

Josselin's article shows an example of colliding contract storage data. However his storage pointer is not a hash of a meaningful string. His storage pointer is not even a hash of a string but is a hash of a hex literal of a specific but random looking value. It is easy to see that it is not a namespace to an area of contract storage.

Malicious storage pointers are obvious and easy to spot. Here is a comparison:

Namespace to an area of contract storage:

bytes32 storagePosition = keccak256("myproject.mytoken")
Enter fullscreen mode Exit fullscreen mode

Malicious pointer that collides with existing contract storage data:

bytes32 storagePosition = keccak256(
  hex"78c8663007d5434a0acd246a3c741b54aecf2fefff4284f2d3604b72f26"
);
Enter fullscreen mode Exit fullscreen mode

Diamond Storage Not Required

The EIP proposes that every implementation have an associated structure to hold the implementation variables, and a pointer to an arbitrary storage location where the structure will be stored.

EIP-2535 Diamond Standard does show, explain and encourage using diamond storage. But it does not require it.

Diamonds and facets are free to use or mix any contract storage strategy such as inherited storage etc.

To comply with the standard and be a diamond an implementation must implement the Specification section of the standard, which does not mandate any contract storage strategy or mechanism.


Function Shadowing

Upgradeable contracts often have functions in the proxy that shadow the functions that should be delegated. Calls to these functions will never be delegated, as they will be executed in the proxy. Additionally, the associated code will not be upgradeable.

This is a good point. All the diamonds from the reference implementations do not have external functions in them, so this is not a problem for them.

However it is possible to have external functions directly in a diamond in a safe way. They are called immutable functions in EIP-2535 Diamond Standard. They are functions that can't be replaced or removed. The diamond loupe functions provide information about them and they are emitted in the DiamondCut event. In the reference implementations calling diamondCut to replace or remove immutable functions will revert. See the standard for more info.


Contract Existence Check

Another common mistake is the absence of an existence check for the contract’s code. If the proxy delegates to an incorrect address, or implementation that has been destructed, the call to the implementation will return success even though no code was executed (see the Solidity documentation). As a result, the caller will not notice the issue, and such behavior is likely to break third-party contract integration.

Note: Josselin's article shows a fallback function without the contract code check he talks about.

The fallback function in the diamond reference implementations do not check for contract code. It does not do this because of the gas cost and because the diamondCut function prevents any functions from being added from facets that do not have code.

I don't write selfdestruct in the facets I write and I don't use facets with selfdestruct in them unless it specifically makes sense for some use case, which hasn't happened for me.

OpenZeppelin's proxy contracts also do not perform the check in the fallback function. Like the diamond reference implementations they prevent implementation contracts from being added that don't have code.

EIP-2535 Diamond Standard does not prevent people from adding this check in the diamond fallback function so it can be added by people who want it.

Discussion

pic
Editor guide
Collapse
montyly profile image
Feist Josselin

Hello Readers,

I am the author of the original post from Trail of Bits. Nick: thank you for acknowledging that you agree with most of the issues pointed out in our review of your proposal.

Regarding the new reference implementations, I think the complexity has got even worse than what we initially reviewed and blogged about. There are now three reference implementations, which makes everything even more confusing for users, and further review of the proposal more difficult.

The "low complexity" version is far from having a low complexity. For example, see the following code and then compare it to the upgrade function in OpenZeppelin's library. This level of complexity is not necessary to update a key-value pair in a mapping.

The direction that the proposal is taking now highlights its primary fundamental issues:

  • It is excessively complex
  • It assumes that users will use it correctly, and no guidance on risks is provided

The proposal and implementations would be suitable for a custom solution but do not meet the maturity required for a standard.

I think it would be beneficial to re-start the proposal, create a simple API, use existing common vocabulary, and avoid the pitfalls shown in our blogpost.

-Josselin

Collapse
mudgen profile image
Nick Mudge Author

Hi Josselin,
I don't agree with the points of your article that are outdated or inaccurate which I address in my article. I think your article is unnecessarily negative. I think you can help with some of the informational/procedure parts you mention in your article and that would be great.

EIP-2535 Diamond Standard is a good and useful standard that applications can use.

OpenZeppelin's upgrade function can't be compared to the Diamond Standard's because they do different things. The Diamond Standard provides more functionality than OpenZeppelin's proxy. With the Diamond Standard you can have functions from multiple implementations/facets, which is a core part of what the Diamond Standard is about.

The diamondCut function that is linked to provides an important safety feature: You have to be explicit about what functions you are adding or replacing or removing. The function then checks and enforces that you don't make a mistake about that. This safety feature is a good thing and protects users.

The diamondCut code that is linked to is easy to read and is straightforward.

  1. The functions and facets are looped over.
  2. The logic is separated into three parts: Add, Replace, Remove
  3. Some safety checks are done (require statements) and then the appropriate logic is execute to add or replace or remove.

Having three different implementations does not affect the complexity of the code, since each implementation is separate.

I think that if the most complicated thing about the Diamond Standard is that people have to choose which implementation to use, that's pretty good.

There are three different implementations to meet the needs and wants of different people. It is not hard to sort out which implementation to use, and in many cases I don't think it matters much which one is used because they do the same things. These are implementations of the standard, not the standard itself. People are free to write their own implementations or use other implementations if they want to.

People that want a simple implementation should use the diamond-1 implementation:
github.com/mudgen/diamond-1

People that want a more gas-efficient implementation should use the diamond-2 implementation: github.com/mudgen/diamond-1

People willing to pay a little more gas and want the simplest loupe functions should use the diamond-3 implementation: github.com/mudgen/diamond-3

More info about this here: github.com/mudgen/diamond

Also people can contact me if they have questions.