Zellic - Audit Reports

Potential storage-collision issue

The ProxyManager contract manages deploying and upgrading clones of master contracts. In its setImplementation function, it handles the actual upgrade of the clones. However, it does not check if the new implementation is a valid upgrade of the clones. This means that if by mistake the owner of the contract sets a new implementation that is not compatible with the clones, the clones will be left in an inconsistent state, possibly having their storage corrupted by the newer, totally different implementation:

function setImplementation(
    bytes32 implementation_id,
    address implementation,
    bytes calldata upgrade_data,
    bool callUpdate
) public onlyOwner {
    require(implementation.code.length > 0, "#075051C6");
    implementations[implementation_id] = implementation;

    // for each item in instances, change the implementation
    for (uint i = 0; i < instances[implementation_id].length; i++) {

        // @audit no check on whether the new implementation is a direct upgrade of the old one, or a different contract altogether.

        IUpgradable(instances[implementation_id][i]).updateImplementation(
            implementation
        );
        if (callUpdate)
            IUpgradable(instances[implementation_id][i]).upgrade(
                upgrade_data
            );
    }
}

Although this issue cannot be remediated in code, we recommend simulating the contract upgrade by using the OpenZeppelin plug-ins, which check and warn in case of storage collisions. More information can be found here↗.