pcarranzav
commented
1 year ago The one about storage gaps is dupe of #244 that might end up being Medium?
As for the rest of the issues, I would actually dispute some/most of them e.g:
- the referenced comments are not actual code, they are there intentionally to show that the value below is the hash of a string). -"Escrow funds could be drained" doesn't sound valid, as there's no specific paths for how the attacker could actually bypass the Arbitrum Outbox's proof system.
- Privileged roles being able to upgrade without a timelock is by design (at least for the time being) - governor is a Gnosis Safe controlled by a multi-party Council.
- I still stand by
^0.7.6as described in other issues
REMOVE COMMENTED CODE
In the code base of
GraphProxyAdmin.sol, there are a few lines of code that have been commented out with //. This can lead to confusion and not conducive to overall code readability. Consider removing/rewriting them. There are three instances found.Line 32 Line 45 Line 57
STORAGE GAP FOR UPGRADEABLE CONTRACTS
Consider adding a storage gap at the end of each upgradeable contract. In the event some contracts needed to inherit from them, there would not be an issue shifting down of storage in the inheritance chain. Generally, storage gaps are a novel way of reserving storage slots in a base contract, allowing future versions of that contract to use up those slots without affecting the storage layout of child contracts. If not, the variable in the child contract might be overridden whenever new variables are added to it. This could have unintended and vulnerable consequences to the child contracts.
UPGRADE TO NEWER VERSION OF SOLIDITY
Consider using a newer version of Solidity ^0.8.0 that features the latest security fixes other than breaking changes and new features introduced periodically.
REQUIRE STATEMENT WITH MISSING ERROR MESSAGE
Consider adding a RELEVANT message to all require statements that would be displayed in the event of a revert. There are two instances found in
GraphProxyAdmin.sol.Line 47 Line 59
Consider using the latest released version of Solidity. Apart from exceptional cases, only the latest version receives security fixes. Furthermore, breaking changes as well as new features are introduced regularly.
PRIVILEGED CALLS AND IMPLEMENTATIONS COULD FRONT RUN USERS UNEXPECTEDLY
Administrators can update or upgrade the contracts without warning, which violates the security goal of the system. Specifically, privileged roles could use front running to make malicious changes just ahead of incoming transactions, or purely accidental negative effects could occur due to the unfortunate timing of changes. Generally, users of the system should have assurances about the behavior of the action they are about to take.
Consider giving users advance notice of changes with a time lock. The first step merely broadcasts to users that a particular change is coming, and the second step commits that change after a suitable waiting period. This would allow users that do not accept the change to withdraw immediately.
COMPILER VERSION PRAGMA SPECIFICITY
Non-library contracts and interfaces should avoid using floating pragmas ^0.7.6. Doing this may be a security risk for the actual application implementation itself. For instance, a known vulnerable compiler version may accidentally be selected or a security tool might fallback to an older compiler version leading to checking a different EVM compilation that is ultimately deployed on the blockchain.
IMMUTABLE INSTEAD OF CONSTANT
Variables having expressions for calculated values should be linked with
immutableinstead ofconstant. There are two instances found in. There are quite a number of instances found inGraphTokenUpgradeable.sol.Lines 34-39 Lines 42-45
ESCROW FUND COULD BE DRAINED
Hackers could exploit via numerous attacking surfaces using low level proofs to hijack the tree system to receive GRT from the escrow simply because there is a long delay to synchronize states on both the EVM and the AVM chains. A more rigorous checks should be in place rather than just counting on the following require statement in
L1GraphTokenGateway.sol.Line 275
It would too late when the above require statement reverted when all funds would have been drained. It belies the cross-chain atomicity alleged in Arbitrum's documentations.