The Graph L2 bridge contest - rotcivegaf's results

QA report

Low Risk

Non-critical

Low Risk

[L-01] Wrong event parameter emit

The _admin() will be the new admin and not the old, save the old admin in a var and emit this one, like in _setImplementation or _setPendingImplementation functions

File: /contracts/upgrades/GraphProxyStorage.sol

86        emit AdminUpdated(_admin(), _newAdmin);

[L-02] With only admin role can't acceptUpgrade or acceptUpgradeAndCall

The functions acceptUpgrade and acceptUpgradeAndCall have the ifAdminOrPendingImpl modifier, but it the sender it's the _admin the transcaction recerts the msg.sender == _pendingImplementation inside the _acceptUpgrade function

File: /contracts/upgrades/GraphProxy.sol

122    function acceptUpgrade() external ifAdminOrPendingImpl {

129    function acceptUpgradeAndCall(bytes calldata data) external ifAdminOrPendingImpl {

143            _pendingImplementation != address(0) && msg.sender == _pendingImplementation,

[L-03] Mark as abstract

If the Governed.sol contract is deployed, will fail to initialize

File: /contracts/governance/Governed.sol

9 contract Governed {

Same with the GraphTokenUpgradeable.sol

File: /contracts/l2/token/GraphTokenUpgradeable.sol

28 contract GraphTokenUpgradeable is GraphUpgradeable, Governed, ERC20BurnableUpgradeable {

File: /contracts/upgrades/GraphUpgradeable.sol

11 contract GraphUpgradeable {

File: /contracts/upgrades/GraphProxyStorage.sol

11 contract GraphProxyStorage {

File: /contracts/governance/Managed.sol

23 contract Managed {

Non-critical

[N-01] Missing error message in require statements

File: /contracts/upgrades/GraphProxy.sol

133        require(success);

File: /contracts/upgrades/GraphProxyAdmin.sol

34        require(success);

47        require(success);

59        require(success);

[N-02] Unused imports

File: /contracts/curation/ICuration.sol

5 import "./IGraphCurationToken.sol";

[N‑03] Non-library/interface files should use fixed compiler versions, not floating ones

File: /contracts/gateway/BridgeEscrow.sol

3 pragma solidity ^0.7.6;

File: /contracts/upgrades/GraphUpgradeable.sol

3 pragma solidity ^0.7.6;

File: /contracts/governance/Governed.sol

3 pragma solidity ^0.7.6;

File: /contracts/governance/Pausable.sol

3 pragma solidity ^0.7.6;

File: /contracts/l2/token/L2GraphToken.sol

3 pragma solidity ^0.7.6;

File: /contracts/upgrades/GraphProxyAdmin.sol

3 pragma solidity ^0.7.6;

File: /contracts/upgrades/GraphProxy.sol

3 pragma solidity ^0.7.6;

File: /contracts/governance/Managed.sol

3 pragma solidity ^0.7.6;

File: /contracts/l2/token/GraphTokenUpgradeable.sol

3 pragma solidity ^0.7.6;

File: /contracts/l2/gateway/L2GraphTokenGateway.sol

3 pragma solidity ^0.7.6;

File: /contracts/gateway/L1GraphTokenGateway.sol

3 pragma solidity ^0.7.6;

File: /contracts/gateway/GraphTokenGateway.sol

3 pragma solidity ^0.7.6;

File: /contracts/curation/IGraphCurationToken.sol

3 pragma solidity ^0.7.6;

File: /contracts/gateway/ICallhookReceiver.sol

9 pragma solidity ^0.7.6;

File: /contracts/upgrades/IGraphProxy.sol

3 pragma solidity ^0.7.6;

File: /contracts/epochs/IEpochManager.sol

3 pragma solidity ^0.7.6;

File: /contracts/governance/IController.sol

3 pragma solidity >=0.6.12 <0.8.0;

File: /contracts/token/IGraphToken.sol

3 pragma solidity ^0.7.6;

File: /contracts/rewards/IRewardsManager.sol

3 pragma solidity ^0.7.6;

File: /contracts/staking/IStakingData.sol

3 pragma solidity >=0.6.12 <0.8.0;

File: /contracts/curation/ICuration.sol

3 pragma solidity ^0.7.6;

File: /contracts/staking/IStaking.sol

3 pragma solidity >=0.6.12 <0.8.0;

[N‑04] Wrong function doc

Should be: * NOTE: Only the admin and implementation can call this function. like in admin function

File: /contracts/upgrades/GraphProxy.sol

76     * NOTE: Only the admin can call this function.

89     * NOTE: Only the admin can call this function.

Gas report

[G-01] Use function parameter to avoid MLOAD

File: /contracts/governance/Governed.sol

45        emit NewPendingOwnership(oldPendingGovernor, pendingGovernor);

File: /contracts/governance/Pausable.sol

31        if (_partialPaused) {

34        emit PartialPauseChanged(_partialPaused);

44        _paused = _toPause;

48        emit PauseChanged(_paused);

58        emit NewPauseGuardian(oldPauseGuardian, pauseGuardian);

File: /contracts/l2/token/L2GraphToken.sol

62        emit GatewaySet(gateway);

[G-02] Unused SafeMath.sol

File: /contracts/l2/token/L2GraphToken.sol

 5 import "@openzeppelin/contracts/math/SafeMath.sol";

16    using SafeMath for uint256;

File: /contracts/l2/gateway/L2GraphTokenGateway.sol

 7 import "@openzeppelin/contracts/math/SafeMath.sol";

24    using SafeMath for uint256;

File: /contracts/l2/token/GraphTokenUpgradeable.sol

 7 import "@openzeppelin/contracts/math/SafeMath.sol";

29    using SafeMath for uint256;

From:
97        nonces[_owner] = nonces[_owner].add(1);
To:
97        ++nonces[_owner];

[G-03] Use function saved var instead of SLOAD and address(0)

File: /contracts/governance/Governed.sol#L53-L67

     function acceptOwnership() external {
+        address oldPendingGovernor = pendingGovernor;
         require(
-            pendingGovernor != address(0) && msg.sender == pendingGovernor,
+            oldPendingGovernor != address(0) && msg.sender == oldPendingGovernor,
             "Caller must be pending governor"
         );

         address oldGovernor = governor;
-        address oldPendingGovernor = pendingGovernor;

-        governor = pendingGovernor;
+        governor = oldPendingGovernor;
         pendingGovernor = address(0);

-        emit NewOwnership(oldGovernor, governor);
-        emit NewPendingOwnership(oldPendingGovernor, pendingGovernor);
+        emit NewOwnership(oldGovernor, oldPendingGovernor);
+        emit NewPendingOwnership(oldPendingGovernor, address(0));
     }
 }

[G-04] Overcheck slots

Can check this slots in a test or you can calculate directly in the slot definition

File: /contracts/upgrades/GraphProxy.sol

47        assert(ADMIN_SLOT == bytes32(uint256(keccak256("eip1967.proxy.admin")) - 1));

48        assert(
49            IMPLEMENTATION_SLOT == bytes32(uint256(keccak256("eip1967.proxy.implementation")) - 1)
50        );

51        assert(
52            PENDING_IMPLEMENTATION_SLOT ==
53                bytes32(uint256(keccak256("eip1967.proxy.pendingImplementation")) - 1)
54        );

[G-05] Getters with ownership modifier and marked as external

These functions could be marked as view functions and the ifAdminOrPendingImpl modifier removed

File: /contracts/upgrades/GraphProxy.sol

69    function admin() external ifAdminOrPendingImpl returns (address) {

82    function implementation() external ifAdminOrPendingImpl returns (address) {

95    function pendingImplementation() external ifAdminOrPendingImpl returns (address) {

[G-06] Overcheck _pendingImplementation and pendingGovernor

If the msg.sender it's the _pendingImplementation => the _pendingImplementation it's not the address(0) The msg.sender == _pendingImplementation check it's enough

File: /contracts/upgrades/GraphProxy.sol

143            _pendingImplementation != address(0) && msg.sender == _pendingImplementation,

The same case to pendingGovernor

File: /governance/Governed.sol

55            pendingGovernor != address(0) && msg.sender == pendingGovernor,