`L2GraphToken` proxy owner cannot be relinquished (deviation from the spec)

[code423n4]

Lines of code

https://github.com/code-423n4/2022-10-thegraph/blob/7ea88cc41f17f2d49961aafec7ebe72daeaad3f9/contracts/upgrades/GraphProxy.sol#L104-L107

Vulnerability details

Impact

The proxy admin cannot be relinquished (cannot be set to address zero), which deviates from the proposal

Note: it is reported as the README writes:

The contracts that are in scope for this audit were added with this spec in mind, and any deviation between the implementation and this spec can be considered a valid submission. link

Proof of Concept

In the GraphProxy's admin is set in the constructor and it can be the zero address. And the proxy admin (or owner) can update the implementation by calling GraphProxy::upgradeTo.

The GraphProxy::setAdmin will revert if the _newAdmin is the zero address.

// GraphProxy
// setAdmin

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

The proxy admin cannot fallback to implementation, and there is no function in L2GraphToken and its parent contracts to set the proxy owner to the zero address anyway.

Thus, unless the proxy owner is set to the zero address in the proxy's constructor, it is not possible to set the proxy owner to zero address, which deviates from the the proposal:

We propose for the L2 Graph Token contract to also be upgradeable, for these same reasons, but the upgradeability can be relinquished (by setting the proxy owner to address zero) when moving to the post-devnet phase.

Tools Used

Manual review

Recommended Mitigation Steps

Add a function to renounce the proxy owner.

[trust1995]

IMO it is probably a good idea that setAdmin cannot set admin to 0, to prevent irrecoverable errors. Owner can still be relinquished by sending to 0x1 or uint160.MAX_INT.

[0xean]

Low risk (e.g. assets are not at risk: state handling, function incorrect as to spec, issues with comments)

maybe valid, but certainly not medium risk.

[0xean]

closing as dupe of #198 which is being used as the wardens QA report.

[pcarranzav]

Agree this is QA, but also a good find! I've updated GIP-0031 to specify the correct way to make it non-upgradeable, it now says:

the upgradeability can be relinquished (by setting the proxy owner to an invalid address) when the community is more confident with the L2 deployment.