MED: Holographer contract may be destructed using delegatecall to trusted address - Holographer's fallback(), HolographERC721's fallback()

[code423n4]

Lines of code

https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/contracts/enforcer/Holographer.sol#L229

Vulnerability details

Description

The Holograph contracts use delegatecall pattern to execute enforcers from Holographer. In the Holographer fallback, getHolographEnforcer is called to calculate a delegatecall destination:

 function getHolographEnforcer() public view returns (address) {
    HolographInterface holograph;
    bytes32 contractType;
    assembly {
      holograph := sload(_holographSlot)
      contractType := sload(_contractTypeSlot)
    }
    return HolographRegistryInterface(holograph.getRegistry()).getReservedContractTypeAddress(contractType);
  }

The issue is that a malicious / compromised admin can update the registry and make getReservedContractTypeAddress() return an arbitrary address. From there, they are able to do anything, from destroying the contract to re-distributing the balances as they see fit.

Impact

Rogue registry represents a major centralization risk which will always exist in each deployed contract.

Tools Used

Manual audit

Recommended Mitigation Steps

Unless explicitly asked by deployer, make the calculated Enforcer be set in stone after the initialization stage.

[gzeoneth]

Could be Low/QA, centralization risk

[alexanderattar]

This is a design decision and the admin key will be a multi-sig to manage upgrades securely

[gzeoneth]

Consider with #433