Vulnerable contract can be deployed if developer forgets to properly configure admin while extending ERC721H/ERC20H

[code423n4]

Lines of code

https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/abstract/ERC721H.sol#L140-L154

Vulnerability details

Impact

Source contract can be uninitialized with owner value that will lead to vulnerable contract that is not able to call functions with onlyOwner modifier.

Proof of Concept

To create source contract you need to extend ERC721H/ERC20H abstract contract. It has onlyOwner modifier. This modifier is needed to restrict some functions to the owner of source contract as you can see for example here.

Function ERC721H.init is responsible for initializing of source contract. Source contract can also override it. The problem is that ERC721H.init doesn't set owner for the contract.

function init(bytes memory initPayload) external virtual override returns (bytes4) {
    return _init(initPayload);
  }

  function _init(
    bytes memory /* initPayload*/
  ) internal returns (bytes4) {
    require(!_isInitialized(), "ERC721: already initialized");
    address _holographer = msg.sender;
    assembly {
      sstore(_holographerSlot, _holographer)
    }
    _setInitialized();
    return InitializableInterface.init.selector;
  }

Because of this, it's possible that source contract will not provide owner and will not work properly.

Tools Used

Recommended Mitigation Steps

Consider adding hook for extender to provide owner and then check that they did it.

function init(bytes memory initPayload) external virtual override returns (bytes4) {
    return _init(initPayload);
  }

  function _init(
    bytes memory /* initPayload*/
  ) internal returns (bytes4) {
    require(!_isInitialized(), "ERC721: already initialized");
    address _holographer = msg.sender;
    assembly {
      sstore(_holographerSlot, _holographer)
    }
    _setInitialized();
    //you can do like this
    //they should ovveride this
    contractCreatorSetOwner();
    //and then check
    checkThatOwnerIsSet();
    return InitializableInterface.init.selector;
  }

[gzeoneth]

not a bug

[alexanderattar]

This is more of a design suggestion than an issue with the code mentioned