The Graph L2 bridge contest - ajtra's results

Low

Details

1. Outdated pragma

Description

The project is using the solidity version 0.7.6. It's a best practice to use the latest release version. You can consult it in the following link

Mitigation

Update the solidity version to 0.8.17

Lines in the code

BridgeEscrow.sol#L3 GraphUpgradeable.sol#L3 Governed.sol#L3 Pausable.sol#L3 L2GraphToken.sol#L3 GraphProxyAdmin.sol#L3 GraphProxyStorage.sol#L3 GraphProxy.sol#L3 Managed.sol#L3 GraphTokenUpgradeable.sol#L3 L2GraphTokenGateway.sol#L3 L1GraphTokenGateway.sol#L3 GraphTokenGateway.sol#L3 IGraphCurationToken.sol#L3 ICallhookReceiver.sol#L9 IGraphProxy.sol#L3 IEpochManager.sol#L3 IController.sol#L3 IGraphToken.sol#L3 IRewardsManager.sol#L3 IStakingData.sol#L3 ICuration.sol#L3 IStaking.sol#L3

2. Floating pragma

Description

Use a pragma ^ or >= <= is not a good practice.
The majority contracts have the pragma solidity directive ^0.7.6. It is recommended to specify a fixed compiler version to ensure that the bytecode produced does not vary between builds. This is especially important if you rely on bytecode-level verification of the code.

Mitigation

Lock the pragma version

Lines in the code

BridgeEscrow.sol#L3 GraphUpgradeable.sol#L3 Governed.sol#L3 Pausable.sol#L3 L2GraphToken.sol#L3 GraphProxyAdmin.sol#L3 GraphProxyStorage.sol#L3 GraphProxy.sol#L3 Managed.sol#L3 GraphTokenUpgradeable.sol#L3 L2GraphTokenGateway.sol#L3 L1GraphTokenGateway.sol#L3 GraphTokenGateway.sol#L3 IGraphCurationToken.sol#L3 ICallhookReceiver.sol#L9 IGraphProxy.sol#L3 IEpochManager.sol#L3 IController.sol#L3 IGraphToken.sol#L3 IRewardsManager.sol#L3 IStakingData.sol#L3 ICuration.sol#L3 IStaking.sol#L3

3. Missing checks for address(0x0) when assigning values to address state variables

Mitigation

Add check for address(0x0)

Lines in the code

Governed.sol#L32

Non Critical

4. public functions not called by the contract should be declared external instead

GraphProxyAdmin.sol#L30 GraphProxyAdmin.sol#L43 GraphProxyAdmin.sol#L55 GraphProxyAdmin.sol#L68 GraphProxyAdmin.sol#L77 GraphProxyAdmin.sol#L86

5. File is missing NatSpec

BridgeEscrow.sol GraphUpgradeable.sol Governed.sol Pausable.sol L2GraphToken.sol GraphProxyAdmin.sol GraphProxyStorage.sol GraphProxy.sol Managed.sol GraphTokenUpgradeable.sol L2GraphTokenGateway.sol L1GraphTokenGateway.sol GraphTokenGateway.sol IGraphCurationToken.sol ICallhookReceiver.sol#L9 IGraphProxy.sol IEpochManager.sol IController.sol IGraphToken.sol IRewardsManager.sol IStakingData.sol ICuration.sol IStaking.sol

6. Event is missing indexed fields

Description

Index event fields make the field more quickly accessible to off-chain tools that parse events. However, note that each index field costs extra gas during emission, so it's not necessarily best to index the maximum allowed per event (threefields). Each event should use three indexed fields if there are three or more fields, and gas usage is not particularly of concern for the events in question. If there are fewer than three fields, all of the fields should be indexed.

Lines in the code

L2GraphToken.sol#L24-L30 Pausable.sol#L19-L20 L1GraphTokenGateway.sol#L56-L66 Managed.sol#L33-L34 Managed.sol#L39 L2GraphTokenGateway.sol#L58-L62

Index

1. Call to KECCAK256 should use IMMUTABLE rather than constant
2. Require instead of &&
3. != 0 is cheaper than > 0 especially in state variables
4. Require / Revert strings longer than 32 bytes cost extra gas
5. Use custom errors rather than REVERT()/REQUIRE() strings to save deployment gas
6. Don't compare boolean expressions to boolean literals
7. Using bools for storage incurs overhead
8. ABI.ENCODE() is less efficient than ABI.ENCODEPACKED()
9. Use a more recent version of solidity
10. Cache storage variables in local variables
11. Internal functions used once could be inlined

Details

1. Call to KECCAK256 should use IMMUTABLE rather than constant

Description

Expressions for constant values such as a call to KECCAK256 should use IMMUTABLE rather than constant

Lines in the code

GraphTokenUpgradeable.sol#L38 GraphTokenUpgradeable.sol#L39

2. Require instead of &&

Description

Split of conditions of an require sentence in different requires sentences can save gas

Lines in the code

L1GraphTokenGateway.sol#L142

3. != 0 is cheaper than > 0 especially in state variables

Description

Replace all > 0 for != 0. In cases where we used a variable from state assigned to a local variable it's the same gas save.

Proof of concept

contract TestA {
    uint256 _paramA;

    function Test () public returns (bool) {
        return _paramA > 0;
    }

}

contract TestB {
    uint256 _paramA;

    function Test () public returns (bool) {
        return _paramA != 0;
    }

}

contract TestC {
    uint256 _paramA;

    function Test () public returns (bool) {
        uint256 aux = _paramA;
        return aux > 0;
    }

}

contract TestD {
    uint256 _paramA;

    function Test () public returns (bool) {
        uint256 aux = _paramA;
        return aux != 0;
    }

}

• Transaction cost of TestA/C is 23491/23504 gas
• Transaction cost of TestB/D is 23494/23507 gas
• After the test it's possible to save 3 gas per ocurrence with this optimization.

Lines in the code

L2GraphTokenGateway.sol#L146 L2GraphTokenGateway.sol#L238 L1GraphTokenGateway.sol#L201 L1GraphTokenGateway.sol#L217

4. Require / Revert strings longer than 32 bytes cost extra gas

Description

Each extra memory word of bytes past the original 32 incurs an MSTORE which costs 3 gas

Lines in the code

GraphUpgradeable.sol#L32 GraphProxy.sol#L105 GraphProxy.sol#L141 GraphProxy.sol#L142 Managed.sol#L53 GraphTokenGateway.sol#L19

5. Use custom errors rather than REVERT()/REQUIRE() strings to save deployment gas

Description

Custom errors are available from solidity version 0.8.4. Custom errors save ~50 gas each time they're hitby avoiding having to allocate and store the revert string. Not defining the strings also save deployment gas.

Lines in the code

GraphUpgradeable.sol#L24 GraphUpgradeable.sol#L32 Governed.sol#L24 Governed.sol#L41 Governed.sol#L54 L2GraphToken.sol#L36 L2GraphToken.sol#L49 L2GraphToken.sol#L60 L2GraphToken.sol#L70 GraphProxyAdmin.sol#L34 GraphProxyAdmin.sol#L47 GraphProxyAdmin.sol#L59 GraphProxyStorage.sol#L62 GraphProxy.sol#L105 GraphProxy.sol#L133 GraphProxy.sol#L141 GraphProxy.sol#L142 GraphProxy.sol#L157 Managed.sol#L44 Managed.sol#L45 Managed.sol#L49 Managed.sol#L53 Managed.sol#L57 Managed.sol#L104 GraphTokenUpgradeable.sol#L60 GraphTokenUpgradeable.sol#L94 GraphTokenUpgradeable.sol#L95 GraphTokenUpgradeable.sol#L106 GraphTokenUpgradeable.sol#L115 GraphTokenUpgradeable.sol#L123 L2GraphTokenGateway.sol#L69 L2GraphTokenGateway.sol#L98 L2GraphTokenGateway.sol#L108 L2GraphTokenGateway.sol#L118 L2GraphTokenGateway.sol#L126 L2GraphTokenGateway.sol#L145 L2GraphTokenGateway.sol#L146 L2GraphTokenGateway.sol#L147 L2GraphTokenGateway.sol#L148 L2GraphTokenGateway.sol#L153 L2GraphTokenGateway.sol#L233 L2GraphTokenGateway.sol#L234 L1GraphTokenGateway.sol#L74 L1GraphTokenGateway.sol#L78 L1GraphTokenGateway.sol#L82 L1GraphTokenGateway.sol#L110 L1GraphTokenGateway.sol#L111 L1GraphTokenGateway.sol#L122 L1GraphTokenGateway.sol#L132 L1GraphTokenGateway.sol#L142 L1GraphTokenGateway.sol#L153 L1GraphTokenGateway.sol#L154 L1GraphTokenGateway.sol#L165 L1GraphTokenGateway.sol#L166 L1GraphTokenGateway.sol#L200 L1GraphTokenGateway.sol#L201 L1GraphTokenGateway.sol#L202 L1GraphTokenGateway.sol#L213 L1GraphTokenGateway.sol#L217 L1GraphTokenGateway.sol#L224 L1GraphTokenGateway.sol#L271 L1GraphTokenGateway.sol#L275 GraphTokenGateway.sol#L19 GraphTokenGateway.sol#L31 GraphTokenGateway.sol#L40

6. Don't compare boolean expressions to boolean literals

Description

The following conditions could be represented in a way that save gas.

Example, - IF condition == true could be IF condition - IF contition == false could be IF !condition

Proof of concept

contract TestA {
    function Test (bool paramA) public {
        require(paramA == true, "Test bool");
    }
}

contract TestB {
    function Test (bool paramA) public {
        require(paramA, "Test bool");
    }
}

• Transaction cost of TestA is 21629 gas
• Transaction cost of TestB is 21611 gas
• After the test it's possible to save 18 gas per ocurrence with this optimization.

Lines in the code

L1GraphTokenGateway.sol#L214

7. Using bools for storage incurs overhead

Description

Use uint256(1) and uint256(2) for true/false to avoid a Gwarmaccess (100 gas), and to avoid Gsset (20000 gas) when changing from 'false' to 'true', after having been 'true' in the past

Lines in the code

Pausable.sol#L8 Pausable.sol#L10 GraphProxyAdmin.sol#L33 GraphProxyAdmin.sol#L46 GraphProxyAdmin.sol#L58 GraphProxy.sol#L132 GraphTokenUpgradeable.sol#L51 L1GraphTokenGateway.sol#L35

8. ABI.ENCODE() is less efficient than ABI.ENCODEPACKED()

Description

abi.encode will apply ABI encoding rules. Therefore all elementary types are padded to 32 bytes and dynamic arrays include their length. Therefore it is possible to also decode this data again (with abi.decode) when the type are known.

abi.encodePacked will only use the only use the minimal required memory to encode the data. E.g. an address will only use 20 bytes and for dynamic arrays only the elements will be stored without length. For more info see the Solidity docs for packed mode

For the input of the keccak method it is important that you can ensure that the resulting bytes of the encoding are unique. So if you always encode the same types and arrays always have the same length then there is no problem. But if you switch the parameters that you encode or encode multiple dynamic arrays you might have conflicts.

For example:

abi.encodePacked(address(0x0000000000000000000000000000000000000001), uint(0)) encodes to the same as abi.encodePacked(uint(0x0000000000000000000000000000000000000001000000000000000000000000), address(0))

and

abi.encodePacked(uint, uint) encodes to the same as abi.encodePacked(uint, uint)

Therefore these examples will also generate the same hashes even so they are different inputs.

On the other hand you require less memory and therefore in most cases abi.encodePacked uses less gas than abi.encode.

Lines in the code

GraphTokenUpgradeable.sol#L88 GraphTokenUpgradeable.sol#L162 L2GraphTokenGateway.sol#L174 L2GraphTokenGateway.sol#L275 L1GraphTokenGateway.sol#L249 L1GraphTokenGateway.sol#L342

9. Use a more recent version of solidity

Description

Use a solidity version of at least 0.8.0 to get overflow protection without SafeMath Use a solidity version of at least 0.8.2 to get compiler automatic inlining Use a solidity version of at least 0.8.3 to get better struct packing and cheaper multiple storage reads Use a solidity version of at least 0.8.4 to get custom errors, which are cheaper at deployment than revert()/require() strings Use a solidity version of at least 0.8.10 to have external calls skip contract existence checks if the external call has a return value.

Lines in the code

BridgeEscrow.sol#L3 GraphUpgradeable.sol#L3 Governed.sol#L3 Pausable.sol#L3 L2GraphToken.sol#L3 GraphProxyAdmin.sol#L3 GraphProxyStorage.sol#L3 GraphProxy.sol#L3 Managed.sol#L3 GraphTokenUpgradeable.sol#L3 L2GraphTokenGateway.sol#L3 L1GraphTokenGateway.sol#L3 GraphTokenGateway.sol#L3 IGraphCurationToken.sol#L3 ICallhookReceiver.sol#L9 IGraphProxy.sol#L3 IEpochManager.sol#L3 IController.sol#L3 IGraphToken.sol#L3 IRewardsManager.sol#L3 IStakingData.sol#L3 ICuration.sol#L3 IStaking.sol#L3

10. Cache storage variables in local variables

Description

To avoid access to the storage to save gas is better to store the value in a local variable and use it.

Governed.acceptOwnership

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

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

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

Lines in the code

Governed.sol#L53-L67

11. Internal functions used once could be inlined

Managed._notPartialPaused

Managed.sol#L43

Managed._onlyGovernor

Managed.sol#L52

Managed._onlyController

Managed.sol#L56