L2 GRAPH TOKEN CONTRACT AND L1 AND L2 GATEWAY CONTRACTS ARE NOT FULLY UPGRADEABLE BECAUSE GraphTokenUpgradeable AND GraphTokenGateway CONTRACTS DO NOT INCLUDE STORAGE GAPS

[code423n4]

Lines of code

https://github.com/code-423n4/2022-10-thegraph/blob/main/contracts/l2/token/L2GraphToken.sol#L15
https://github.com/code-423n4/2022-10-thegraph/blob/main/contracts/gateway/L1GraphTokenGateway.sol#L21
https://github.com/code-423n4/2022-10-thegraph/blob/main/contracts/l2/gateway/L2GraphTokenGateway.sol#L23
https://github.com/code-423n4/2022-10-thegraph/blob/main/contracts/l2/token/GraphTokenUpgradeable.sol#L28-L50
https://github.com/code-423n4/2022-10-thegraph/blob/main/contracts/gateway/GraphTokenGateway.sol#L14

Vulnerability details

Impact

As https://hackmd.io/@N6uqeJqKRhS_geEwjyATnQ/rJoKEmvrq specifies, the L2 Graph Token contract and L1 and L2 Gateway contracts should be upgradeable. As the code below show, the L2GraphToken contract inherits from the GraphTokenUpgradeable contract, and the L1GraphTokenGateway and L2GraphTokenGateway contracts inherit from the GraphTokenGateway contract. Meanwhile, the GraphTokenUpgradeable contract inherits from the ERC20BurnableUpgradeable contract, and the GraphTokenGateway contract inherits from the Managed contract.

https://github.com/code-423n4/2022-10-thegraph/blob/main/contracts/l2/token/L2GraphToken.sol#L15

contract L2GraphToken is GraphTokenUpgradeable, IArbToken {

https://github.com/code-423n4/2022-10-thegraph/blob/main/contracts/gateway/L1GraphTokenGateway.sol#L21

contract L1GraphTokenGateway is GraphTokenGateway, L1ArbitrumMessenger {

https://github.com/code-423n4/2022-10-thegraph/blob/main/contracts/l2/gateway/L2GraphTokenGateway.sol#L23

contract L2GraphTokenGateway is GraphTokenGateway, L2ArbitrumMessenger, ReentrancyGuardUpgradeable {

https://github.com/code-423n4/2022-10-thegraph/blob/main/contracts/l2/token/GraphTokenUpgradeable.sol#L28-L50

contract GraphTokenUpgradeable is GraphUpgradeable, Governed, ERC20BurnableUpgradeable {
    using SafeMath for uint256;

    // -- EIP712 --
    // https://github.com/ethereum/EIPs/blob/master/EIPS/eip-712.md#definition-of-domainseparator

    bytes32 private constant DOMAIN_TYPE_HASH =
        keccak256(
            "EIP712Domain(string name,string version,uint256 chainId,address verifyingContract,bytes32 salt)"
        );
    bytes32 private constant DOMAIN_NAME_HASH = keccak256("Graph Token");
    bytes32 private constant DOMAIN_VERSION_HASH = keccak256("0");
    bytes32 private constant DOMAIN_SALT =
        0xe33842a7acd1d5a1d28f25a931703e5605152dc48d64dc4716efdae1f5659591; // Randomly generated salt
    bytes32 private constant PERMIT_TYPEHASH =
        keccak256(
            "Permit(address owner,address spender,uint256 value,uint256 nonce,uint256 deadline)"
        );

    // -- State --

    // solhint-disable-next-line var-name-mixedcase
    bytes32 private DOMAIN_SEPARATOR;

https://github.com/code-423n4/2022-10-thegraph/blob/main/contracts/gateway/GraphTokenGateway.sol#L14

abstract contract GraphTokenGateway is GraphUpgradeable, Pausable, Managed, ITokenGateway {

Although the ERC20BurnableUpgradeable and Managed contracts include storage gaps, the GraphTokenUpgradeable and GraphTokenGateway contracts do not have these.

As https://docs.openzeppelin.com/contracts/3.x/upgradeable#storage_gaps mentions, the ERC20BurnableUpgradeable contract's __gap allows the OpenZeppelin team to "freely add new state variables in the future without compromising the storage compatibility with existing deployments"; in other words, this storage gap is reserved for updating the ERC20BurnableUpgradeable contract by the OpenZeppelin team. It is possible that the ERC20BurnableUpgradeable contract is updated to include more state variables through using most or all of its __gap in the future. When this occurs, upgrading the GraphTokenUpgradeable contract to include more state variables can possibly end up in storage collisions if wanting to use the updated ERC20BurnableUpgradeable contract.

Although the Managed contract is controlled by the protocol team, the similar issue also exists for the GraphTokenGateway contract. If the GraphTokenGateway contract needs to include new state variables in the future, the Managed contract's __gap size needs to be reduced to accommodate that. However, since the Managed contract is a shared contract that other contracts inherit from, reducing its __gap size can cause storage collisions for proxies that use these affected contracts.

Because both of the GraphTokenUpgradeable and GraphTokenGateway contracts do not include their own storage gaps, their upgradabilities are limited. As a result, the L2 Graph Token contract and L1 and L2 Gateway contracts are not fully upgradeable, which does not fully comply with the specification.

Proof of Concept

The following steps can occur for the case involving the GraphTokenUpgradeable contract. The case that involves the GraphTokenGateway contract is similar to this.

1. The ERC20BurnableUpgradeable contract is updated by the OpenZeppelin team to include more state variables, which reduces its __gap size to 3.
2. 5 more state variables need to be added in the GraphTokenUpgradeable contract while wanting to use the updated ERC20BurnableUpgradeable contract.
3. Adding 5 more state variables in the GraphTokenUpgradeable contract while using the updated ERC20BurnableUpgradeable contract will cause storage collisions.
4. To accommodate the needed change for the GraphTokenUpgradeable contract, the protocol team is forced to modify a copy of the old ERC20BurnableUpgradeable contract, which was in use, by decreasing its __gap by 5 and use this copy. As a result, some or all of the new features provided by the updated ERC20BurnableUpgradeable contract become unavailable.

Tools Used

VSCode

Recommended Mitigation Steps

The GraphTokenUpgradeable and GraphTokenGateway contracts can be updated to include their own __gap state variables with reasonable sizes.

[trust1995]

Dup of #306

[0xean]

Well written issue and articulates the risk and impacts well. I am going to leave this open for sponsor review, because I do think its important its not missed in the noise of closed/ duped QA issues, but probably will lean towards it also being marked as QA.

The warden does argue effectively that this should be M

2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

I do see the argument that functionality or availability could be impacted.

(note to self, re-mark #306 to match final severity )

[pcarranzav]

This is a good catch and I agree with the M severity. We'll work on a fix.

Just a note on the POC and issue description: I think we would actually have storage collisions independently to whether we use an updated ERC20BurnableUpgradeable or not - given the order of inheritance, any storage variable added to GraphTokenUpgradeable or GraphTokenGateway would cause a storage collision, as it would move the starting slot for the other contracts' storage.

[trust1995]

Some judges had a lengthy discussion on the exact issue here code-423n4/org#55.

[pcarranzav]

Fix PRd in graphprotocol/contracts#739

[trust1995]

All the many of dups of this finding were rightfully closed as invalid, no reason this one should be different.

Lack of storage gaps has been discussed to be QA / Low severity , see here: code-423n4/org#55

[0xean]

per previous c4 discussions, going to go ahead and downgrade to QA/Low severity.

[0xean]

marking as dupe of wardens QA report.

[0xean]

dupe of #263

[0xean]

dupe of #263

[0xean]

dupe of #263