Two inits for the same contract

[code423n4]

Lines of code

https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/contracts/enforcer/PA1D.sol#L173-L198
https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/contracts/enforcer/HolographERC721.sol#L259-L261

Vulnerability details

Impact

The PA1D contract have two function to initialize. The init and the initPA1D functions
The init sets _adminSlot, _ownerSlot and call setRoyalties but initPA1D function only call setRoyalties and don't initialize the _ownerSlot
As setRoyalties have onlyOwner modifier when the HolographERC721 call the initPA1D initialize reverts
Also the setRoyalties could be used by the owner without initialized restriction

Proof of Concept

init of HolographERC721 call initPA1D:

      (bool success, bytes memory returnData) = _royalties().delegatecall(
        abi.encodeWithSignature("initPA1D(bytes)", abi.encode(address(this), uint256(contractBps)))
      );

initPA1D of PA1D call setRoyalties:

  function initPA1D(bytes memory initPayload) external returns (bytes4) {
    uint256 initialized;
    assembly {
      initialized := sload(_initializedPaidSlot)
    }
    require(initialized == 0, "PA1D: already initialized");
    (address receiver, uint256 bp) = abi.decode(initPayload, (address, uint256));
    setRoyalties(0, payable(receiver), bp);
    initialized = 1;
    assembly {
      sstore(_initializedPaidSlot, initialized)
    }
    return InitializableInterface.init.selector;
  }

setRoyalties of PA1D have onlyOwner modifier:

  function setRoyalties(
    uint256 tokenId,
    address payable receiver,
    uint256 bp
  ) public onlyOwner {

Tools Used

Review

Recommended Mitigation Steps

Remove the initPA1D function and use only the init function

[alexanderattar]

init is used when deploying actual contract. All use of PA1D is via delegatecall, so the init is already called. initPA1D needs to be called to init even on deleatecall.