SHN-02C: Non-Standard Storage Declaration

JayWelsh commented 2 years ago

SHN-02C: Non-Standard Storage Declaration

Type	Severity	Location
Language Specific	Informational	SeenHausNFT.sol:L27, L32, L35

Description:

The SeenHausNFT contract is meant to be an upgrade-able one yet has its storage layout bundled with its logic blocks.

Example:

contract SeenHausNFT is ISeenHausNFT, MarketClientBase, ERC1155Upgradeable {

    address private _owner;

    event OwnershipTransferred(address indexed previousOwner, address indexed newOwner);

    /// @dev token id => Token struct
    mapping (uint256 => Token) internal tokens;

    // Next token number
    uint256 internal nextToken;

Recommendation:

We advise the two items to be split into separate contracts to increase the maintainability of the codebase as the order of storage is crucial in upgradeable contracts.

cliffhall commented 2 years ago

I vote no on performing this particular refactor. Here's why:

Taking this advice, we could refactor/extract these state vars into a storage struct, along with a getter, in a separate contract, similar to MarketControllerLib.sol or DiamondLib.sol then fetching and referencing that struct in the logic contract SeenHausNFT.sol.

However, it doesn't really clean the whole thing up because the inherited ERC1155Upgradeable contract (infuriatingly) declares its own storage in this same way. Which is to say, although this is reasonable advice, OpenZeppelin hasn't even followed it in their code.

If you tried to put SeenHausNFT.sol behind a Diamond where other facets would access the same proxy storage, you'd be in a pickle; they'd absolutely overwrite each other with unpredictable results.

But this isn't behind a Diamond, it's a 1-to-1 upgradeable proxy / implementation relationship.

The important things to remember are:

If you upgrade SeenHausNFT.sol, you cannot remove any of these state vars.
If you add any, you must do it at the end of the list.
You must not reorder the declarations.
You must not upgrade ERC1155Upgradeable itself, because if OZ changes the inherited storage they declare, then our existing storage in the proxy will be corrupted unpredictably by the new code.

Those same rules would still apply even if you tucked all this away inside a storage struct declared in another inherited contract. You'd have one contract dedicated to defining the storage and one the logic. A reasonable separation of concerns, but not a necessary or compelling one. IMO, the cons outweigh the pros: more discrete contracts (complexity) with no objective improvement in operation or security.

JayWelsh commented 2 years ago

Resolved by https://github.com/seen-haus/seen-contracts/pull/60

Please note that _owner needed to change from private to internal

seen-haus / seen-contracts