fix: add storage gaps to GraphTokenGateway and GraphTokenUpgradeable (C4 M-244) #739
Conversation
Codecov ReportBase: 91.58% // Head: 91.58% // No change to project coverage 👍
Additional details and impacted files@@ Coverage Diff @@
## pcv/700-699-n01-gas-check #739 +/- ##
==========================================================
Coverage 91.58% 91.58%
==========================================================
Files 42 42
Lines 1996 1996
Branches 356 356
==========================================================
Hits 1828 1828
Misses 168 168
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
pcarranzav
force-pushed
the
pcv/c4-699-m244-gaps
branch
from
94520f9
to
fa52952
Compare
| @@ -50,6 +50,8 @@ contract GraphTokenUpgradeable is GraphUpgradeable, Governed, ERC20BurnableUpgra | |||
| bytes32 private DOMAIN_SEPARATOR; | |||
| mapping(address => bool) private _minters; | |||
| mapping(address => uint256) public nonces; | |||
| // Storage gap added in case we need to add state variables to this contract | |||
| uint256[47] private __gap; | |||
There was a problem hiding this comment.
We could just keep the gap at 50 until an upgrade happens. But it is nothing critical
There was a problem hiding this comment.
yeah, I chose 47 so that for both contracts we use a standard number of total used slots, i.e. 50
|
Consider futureproofing and lining up the Managed.sol to be a 50 slot contract as well. |
It's a good call, but those other contracts are used by several protocol contracts that are already deployed, so adding a storage gap would break updates in all of those. Luckily the scope for Managed, Pausable and Governed is limited and the behavior is well-defined, so we don't expect to need to add any storage variables to those. |
Resolves code-423n4/2022-10-thegraph-findings#244