Closed code423n4 closed 1 year ago
Dup of #306
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 )
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.
Some judges had a lengthy discussion on the exact issue here https://github.com/code-423n4/org/issues/55.
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: https://github.com/code-423n4/org/issues/55
per previous c4 discussions, going to go ahead and downgrade to QA/Low severity.
marking as dupe of wardens QA report.
dupe of #263
dupe of #263
dupe of #263
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 theGraphTokenUpgradeable
contract, and theL1GraphTokenGateway
andL2GraphTokenGateway
contracts inherit from theGraphTokenGateway
contract. Meanwhile, theGraphTokenUpgradeable
contract inherits from theERC20BurnableUpgradeable
contract, and theGraphTokenGateway
contract inherits from theManaged
contract.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
Although the
ERC20BurnableUpgradeable
andManaged
contracts include storage gaps, theGraphTokenUpgradeable
andGraphTokenGateway
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 theERC20BurnableUpgradeable
contract by the OpenZeppelin team. It is possible that theERC20BurnableUpgradeable
contract is updated to include more state variables through using most or all of its__gap
in the future. When this occurs, upgrading theGraphTokenUpgradeable
contract to include more state variables can possibly end up in storage collisions if wanting to use the updatedERC20BurnableUpgradeable
contract.Although the
Managed
contract is controlled by the protocol team, the similar issue also exists for theGraphTokenGateway
contract. If theGraphTokenGateway
contract needs to include new state variables in the future, theManaged
contract's__gap
size needs to be reduced to accommodate that. However, since theManaged
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
andGraphTokenGateway
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 theGraphTokenGateway
contract is similar to this.ERC20BurnableUpgradeable
contract is updated by the OpenZeppelin team to include more state variables, which reduces its__gap
size to 3.GraphTokenUpgradeable
contract while wanting to use the updatedERC20BurnableUpgradeable
contract.GraphTokenUpgradeable
contract while using the updatedERC20BurnableUpgradeable
contract will cause storage collisions.GraphTokenUpgradeable
contract, the protocol team is forced to modify a copy of the oldERC20BurnableUpgradeable
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 updatedERC20BurnableUpgradeable
contract become unavailable.Tools Used
VSCode
Recommended Mitigation Steps
The
GraphTokenUpgradeable
andGraphTokenGateway
contracts can be updated to include their own__gap
state variables with reasonable sizes.