We've published the release candidate for the next version of OpenZeppelin Contracts. There are a ton of changes and additions!
This begins the Open Review Period for the release, during which we ask the community to review these changes and let us know your feedback. We are of course particularly interested in security issues and we invite everyone to participate in our bug bounty with rewards of up to USD $25,000 and a special POAP for submitting a valid issue. This period will extend for at least 3 weeks, until February 8th.
Take a look at the changelog for the full list, but here are some of our highlights:
ERC2981 and ERC721Royalty: implementations of ERC-2981, a recently finalized standard for communicating royalty information to NFT marketplaces.
ERC721Votes: An extension of ERC721 with vote tracking and delegation, for NFT-based governance.
Base64: A library for Base64 encoding, very useful for fully on-chain NFT metadata.
GovernorPreventLateQuorum: A security module for governance that ensures a minimum voting duration is available.
SignedMath: A few math utilities, now available for signed integers.
UUPSUpgradeable simplifications: We reviewed our initial design decisions and went for a simpler security mechanism with less attack surface.
ERC20 infinite approval: We adopted an optimization that results in less storage accesses when using dapps.
I wanted to start a, hopefully constructive, discussion around EIP1967 and since you guys have modified it for this release I think this might be a good opportunity for it - so here it goes:
I noticed that the ERC1967Upgrade contract is defined as abstract, which I'm not really against from an architectural/Inheritance point of view since it makes sense BUT I think it makes the implementation less robust and, IMHO, leads to some sort of "disconnection" from what I understand the intention of the standard is and the actual implementation.
Let me elaborate...
What I understand the standard tries to solve is defining a bunch of storage slots that all/most proxies could adopt and since the unstructured storage pattern is being used this ultimately means that the selection of these slots is of upmost importance - a malicious actor could define a slot that could potentially clash with the implementation contract sotrage slots.
For this reason (i.e safety) I would have expected the ERC1967Upgrade contract to perform the slot validation in it's constructor (which is not the case since its abstract). I think that delegating the slot validation to the implementation (and thus to the developer) can potentially lead to people forgetting to perform said validation - especially so if the documentation does not make it very clear that this is a job of the implementer (I can see there are some comments above each of the slots mentioning validation in constructor but I think those are not clear enough) - or could lead to cases in which a malicious actor modifies one of the slots to create a storage clash and the user is not inmediatly aware of this because looking at the current implementation does not make this obvious.
So my suggestion is to consider making ERC1967Upgrade contract non-abstract and perform the slot validation or, at the very least, make it very clear that these slots have to be validated by the implementations for both developers and any user looking at the library to compare to a contract they have found in the wild.
would love to hear your thoughts.
Apologies if this is not the right place for the discussion
I had some trouble understanding what you meant by "validation" but I see now that you mean validating that the slot numbers are actually the hashes of the strings they claim to be (minus 1). Do you want to open an issue about this in the repository? Note that whether the contract is abstract is not really related to your point. The contract could have a constructor and be abstract.
That said, I don't think we would want to add the validation in the constructor for ERC1967Upgrade... In fact I'm thinking we should remove the validation entirely. But this would be better discussed in an issue!
Thats very interesting - I would have thought, since they cannot be instantiated, that you could not declare a constructor on an abstract contract - I guess I haven't seen enough abstract contracts (I learned something new - thx for that ).
I'll happily open an issue - I actually thought about that in the first place but was unsure if github was the best place for the discussion, expect it some day during this week when I have some bandwidth.
@kingjerod Tomorrow Wednesday! We were finalizing some important improvements to our upgradeability transpiler that we wanted done before the release, and that was accomplished today.
