QA Report

[code423n4]

• Low
  • 1. Mixing and Outdated compiler
  • 2. Lack of checks address0
  • 3. Lack of checks supportsInterface
• Non critical
  • 4. Use abstract for base contracts
  • 5. Wrong comment
  • 6. Use package with known vulenerabilities
  • 7. It's possible to call initialize twice

Low

1. Mixing and Outdated compiler

The pragma version used are:

pragma solidity >=0.6.12 <0.8.0;
pragma solidity ^0.7.6;

Note that mixing pragma is not recommended. Because different compiler versions have different meanings and behaviors, it also significantly raises maintenance costs. As a result, depending on the compiler version selected for any given file, deployed contracts may have security issues.

The minimum required version must be 0.8.17; otherwise, contracts will be affected by the following important bug fixes:

0.8.3:

• Optimizer: Fix bug on incorrect caching of Keccak-256 hashes.

0.8.4:

• ABI Decoder V2: For two-dimensional arrays and specially crafted data in memory, the result of abi.decode can depend on data elsewhere in memory. Calldata decoding is not affected.

0.8.9:

• Immutables: Properly perform sign extension on signed immutables.
• User Defined Value Type: Fix storage layout of user defined value types for underlying types shorter than 32 bytes.

0.8.13:

• Code Generator: Correctly encode literals used in abi.encodeCall in place of fixed bytes arguments.

0.8.14:

• ABI Encoder: When ABI-encoding values from calldata that contain nested arrays, correctly validate the nested array length against calldatasize() in all cases.
• Override Checker: Allow changing data location for parameters only when overriding external functions.

0.8.15

• Code Generation: Avoid writing dirty bytes to storage when copying bytes arrays.
• Yul Optimizer: Keep all memory side-effects of inline assembly blocks.

0.8.16

• Code Generation: Fix data corruption that affected ABI-encoding of calldata values represented by tuples: structs at any nesting level; argument lists of external functions, events and errors; return value lists of external functions. The 32 leading bytes of the first dynamically-encoded value in the tuple would get zeroed when the last component contained a statically-encoded array.

0.8.17

• Yul Optimizer: Prevent the incorrect removal of storage writes before calls to Yul functions that conditionally terminate the external EVM call.

Apart from these, there are several minor bug fixes and improvements.

2. Lack of checks address(0)

The following methods have a lack of checks if the received argument is an address, it's good practice in order to reduce human error to check that the address specified in the constructor or initialize is different than address(0).

Affected source code:

• Pausable.sol:55
• Governed.sol:31

3. Lack of checks supportsInterface

The EIP-165 standard helps detect that a smart contract implements the expected logic, prevents human error when configuring smart contract bindings, so it is recommended to check that the received argument is a contract and supports the expected interface.

Reference:

• https://eips.ethereum.org/EIPS/eip-165

Affected source code:

• Managed.sol:105
• L2GraphToken.sol:71
• L2GraphToken.sol:61
• L2GraphTokenGateway.sol:99
• L2GraphTokenGateway.sol:109
• L2GraphTokenGateway.sol:119
• L1GraphTokenGateway.sol:112-113
• L1GraphTokenGateway.sol:123
• L1GraphTokenGateway.sol:133

---

Non critical

4. Use abstract for base contracts

abstract contracts are contracts that have at least one function without its implementation. An instance of an abstract cannot be created.

Reference:

• https://docs.soliditylang.org/en/v0.6.2/contracts.html#abstract-contracts

Affected source code:

• GraphProxyStorage.sol:11
• GraphUpgradeable.sol:11
• Pausable.sol:5
• Managed.sol:23
• Governed.sol:9

5. Wrong comment

The contract GraphProxy has the following comment:

NOTE: Only the admin can call this function.

But this is not true, because the following methods use the modifier ifAdminOrPendingImpl or ifAdmin, and these modifiers will invoke _fallback if is not an admin.

Affected source code:

• GraphProxy.sol:63
• GraphProxy.sol:76
• GraphProxy.sol:89
• GraphProxy.sol:102
• GraphProxy.sol:113

6. Use package with known vulenerabilities

Although not vulnerable, the version of openZeppelin used contains known vulnerabilities.

Reference:

• https://github.com/OpenZeppelin/openzeppelin-contracts/security/advisories/GHSA-4h98-2769-gh6h

Affected source code:

• GraphTokenUpgradeable.sol:93

7. It's possible to call initialize twice

The method initialize should be allowed to be called only once.

Affected source code:

• L1GraphTokenGateway.sol:99
• L2GraphToken.sol:48
• L2GraphTokenGateway.sol:87
• BridgeEscrow.sol:20

[trust1995]

Point 7 is dup of #149