Lines of code

https://github.com/code-423n4/2022-10-thegraph/blob/309a188f7215fa42c745b136357702400f91b4ff/contracts/upgrades/GraphProxy.sol#L104-L107 https://github.com/code-423n4/2022-10-thegraph/blob/309a188f7215fa42c745b136357702400f91b4ff/contracts/upgrades/GraphProxyStorage.sol#L80-L87

Vulnerability details

Impact

The GraphProxy has a 2-step process for changing the current implementation. This acts as a safeguard to ensure that the address of the new implementation is set correctly, as it has to confirm that it is going to be used as a implementation for the proxy by calling a certain function.

No such 2-step process exists on the GraphProxy for setting the admin address. The new admin does not have to confirm that he is taking over as the admin and hence falsely setting the new admin address (for example caused by inattention) can cause the loss of all administrative power (such as upgrading the implementation or changing to another admin) over the proxy.

Proof of Concept

As can be seen GraphProxy.sol (and GraphProxyStorage.sol which it inherits from) the setAdmin function directly sets the new admin address in storage:

function setAdmin(address _newAdmin) external ifAdmin {
    require(_newAdmin != address(0), "Cannot change the admin of a proxy to the zero address");
    _setAdmin(_newAdmin);
}

function _setAdmin(address _newAdmin) internal {
    bytes32 slot = ADMIN_SLOT;
    assembly {
        sstore(slot, _newAdmin)
    }

    emit AdminUpdated(_admin(), _newAdmin);
}

If the admin address were to be changed to an address that the protocol has no control over, all administrative functions would be lost, as both changing to a new admin address as well as changing the implementation address require being called from the admin in the first place.

Tools Used

Manual review

Recommended Mitigation Steps

Turn the change of an admin address into a 2-step process, where the new admin has to confirm that he is accepting the role (analogous to changing the implementation).

code-423n4 / 2022-10-thegraph-findings