Lack of gap variable in GraphTokenUpgradeable

[code423n4]

Lines of code

https://github.com/code-423n4/2022-10-thegraph/blob/309a188f7215fa42c745b136357702400f91b4ff/contracts/l2/token/GraphTokenUpgradeable.sol#L47-L53

Vulnerability details

Impact

Gap variables are essential for upgradeability of base contracts in a Proxy implementation. Without them, adding new variables to a base contract will compromise storage compatibility.

A more detailed explanation can be found here: https://docs.openzeppelin.com/upgrades-plugins/1.x/writing-upgradeable#storage-gaps

Proof of Concept

There are multiple base contracts that are inherited from in implementation contracts and used behind proxies which do not have a gap variable, such as Pausable, Governed and GraphTokenUpgradeable. The former 2 contracts are already used in deployed implementation contracts and hence should not be changed to include a gap variable as that would also break storage compatibility. Also they do look more "complete" in the regard that they only fulfill one specific task.

Having a gap variable in GraphTokenUpgradeable on the other hand would be beneficial as both it and its child contract L2GraphToken define their own variables. So any future use cases and upgrades where it would make sense to add new variables to GraphTokenUpgradeable would be hindered.

There are other workarounds such as flattening the parent and its child into one contract and just adding the new variables at the end, however that goes against good coding practices and would hurt readability and maintainability of the code base.

Tools Used

Manual review

Recommended Mitigation Steps

Add a gap variable to GraphTokenUpgradeable.sol as mentioned above.

[trust1995]

Dup of #306

[pcarranzav]

(Note #244 is also dupe and might end up being Medium if judge agrees)

[Minh-Trng]

@0xean I would like to ask this issue to be reviewed, as it is a dup of #244 (as also kindly pointed out by the sponsor above) and currently being marked invalid, even though it describes the same issue at hand. Mine seems less verbose (and is admittedly not written as well as the other), but the reason GraphTokenGateway is not included here is because it currently does not contain any variables to begin with, so the case for adding a gap variable there is weaker than for GraphTokenUpgradeable, which has been identified by both reports

[0xean]

dupe of #244