The Graph L2 bridge contest - 0x1f8b's results

• 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

• Gas
  • 1. Use require instead of assert
  • 2. Reduce boolean comparison
    • Total gas saved: 18 * 1 + 15 * 0 = 18
  • 3. Reduce the size of error messages Long revert Strings
    • Total gas saved: 18 * 9 = 162
  • 4. Use Custom Errors instead of Revert Strings to save Gas
    • Total gas saved: 9 * 64 = 576
  • 5. delete optimization
    • Total gas saved: 5 * 2 = 10
  • 6. Change bool to uint256 can save gas
  • 7. There's no need to set default values for variables
    • Total gas saved: 8 * 3 = 24
  • 8. Gas saving using immutable
  • 9. Optimize Governed.transferOwnership
  • 10. Optimize Governed.acceptOwnership
  • 11. constants expressions are expressions, not constants
  • 12. Optimize GraphTokenUpgradeable.permit

Gas

1. Use require instead of assert

The assert() and require() functions are a part of the error handling aspect in Solidity. Solidity makes use of state-reverting error handling exceptions. This means all changes made to the contract on that call or any sub-calls are undone if an error is thrown. It also flags an error.

They are quite similar as both check for conditions and if they are not met, would throw an error.

The big difference between the two is that the assert() function when false, uses up all the remaining gas and reverts all the changes made.

Meanwhile, a require() function when false, also reverts back all the changes made to the contract but does refund all the remaining gas fees we offered to pay. This is the most common Solidity function used by developers for debugging and error handling.

Affected source code:

• GraphProxy.sol:47
• GraphProxy.sol:48-50
• GraphProxy.sol:51-54

2. Reduce boolean comparison

Compare a boolean value using == true or == false instead of using the boolean value is more expensive.

NOT opcode it's cheaper than using EQUAL or NOTEQUAL when the value it's false, or just the value without == true when it's true, because it will use less opcodes inside the VM.

Proof of concept (without optimizations):

pragma solidity 0.8.16;

contract TesterA {
function testEqual(bool a) public view returns (bool) { return a == true; }
}

contract TesterB {
function testNot(bool a) public view returns (bool) { return a; }
}

Gas saving executing: 18 per entry for == true

TesterA.testEqual:   21814
TesterB.testNot:     21796   

pragma solidity 0.8.16;

contract TesterA {
function testEqual(bool a) public view returns (bool) { return a == false; }
}

contract TesterB {
function testNot(bool a) public view returns (bool) { return !a; }
}

Gas saving executing: 15 per entry for == false

TesterA.testEqual:   21814
TesterB.testNot:     21799

Affected source code:

Use the value instead of == true:

• L1GraphTokenGateway.sol:214

Total gas saved: (18 * 1) + (15 * 0) = 18

3. Reduce the size of error messages (Long revert Strings)

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met.

Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.

Proof of concept (without optimizations):

pragma solidity 0.8.15;

contract TesterA {
function testShortRevert(bool path) public view {
require(path, "test error");
}
}

contract TesterB {
function testLongRevert(bool path) public view {
require(path, "test big error message, more than 32 bytes");
}
}

Gas saving executing: 18 per entry

TesterA.testShortRevert: 21886
TesterB.testLongRevert:  21904

Affected source code:

• GraphTokenGateway.sol:19-22
• Governed.sol:54-57
• Managed.sol:53
• GraphProxy.sol:105
• GraphProxy.sol:141
• GraphProxy.sol:142-145
• GraphProxy.sol:157
• GraphUpgradeable.sol:24
• GraphUpgradeable.sol:32

Total gas saved: 18 * 9 = 162

4. Use Custom Errors instead of Revert Strings to save Gas

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)

Source Custom Errors in Solidity:

Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.

Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).

Proof of concept (without optimizations):

pragma solidity 0.8.15;

contract TesterA {
function testRevert(bool path) public view {
 require(path, "test error");
}
}

contract TesterB {
error MyError(string msg);
function testError(bool path) public view {
 if(path) revert MyError("test error");
}
}

Gas saving executing: 9 per entry

TesterA.testRevert: 21611
TesterB.testError:  21602     

Affected source code:

• GraphTokenGateway.sol:19-22
• GraphTokenGateway.sol:31
• GraphTokenGateway.sol:40
• L1GraphTokenGateway.sol:74
• L1GraphTokenGateway.sol:78
• L1GraphTokenGateway.sol:82
• L1GraphTokenGateway.sol:110
• L1GraphTokenGateway.sol:111
• L1GraphTokenGateway.sol:122
• L1GraphTokenGateway.sol:132
• L1GraphTokenGateway.sol:142
• L1GraphTokenGateway.sol:153
• L1GraphTokenGateway.sol:154
• L1GraphTokenGateway.sol:165
• L1GraphTokenGateway.sol:166
• L1GraphTokenGateway.sol:200
• L1GraphTokenGateway.sol:201
• L1GraphTokenGateway.sol:202
• L1GraphTokenGateway.sol:213-216
• L1GraphTokenGateway.sol:217
• L1GraphTokenGateway.sol:224
• L1GraphTokenGateway.sol:271
• L1GraphTokenGateway.sol:275
• Governed.sol:24
• Governed.sol:41
• Governed.sol:54-57
• Managed.sol:44
• Managed.sol:45
• Managed.sol:49
• Managed.sol:53
• Managed.sol:57
• Managed.sol:104
• L2GraphTokenGateway.sol:69-72
• L2GraphTokenGateway.sol:98
• L2GraphTokenGateway.sol:108
• L2GraphTokenGateway.sol:118
• L2GraphTokenGateway.sol:145
• L2GraphTokenGateway.sol:146
• L2GraphTokenGateway.sol:147
• L2GraphTokenGateway.sol:148
• L2GraphTokenGateway.sol:153
• L2GraphTokenGateway.sol:233
• L2GraphTokenGateway.sol:234
• GraphTokenUpgradeable.sol:60
• GraphTokenUpgradeable.sol:94
• GraphTokenUpgradeable.sol:95
• GraphTokenUpgradeable.sol:106
• GraphTokenUpgradeable.sol:115
• GraphTokenUpgradeable.sol:123
• L2GraphToken.sol:36
• L2GraphToken.sol:49
• L2GraphToken.sol:60
• L2GraphToken.sol:70
• GraphProxy.sol:105
• GraphProxy.sol:133
• GraphProxy.sol:141
• GraphProxy.sol:142-145
• GraphProxy.sol:157
• GraphProxyAdmin.sol:34
• GraphProxyAdmin.sol:47
• GraphProxyAdmin.sol:59
• GraphProxyStorage.sol:62
• GraphUpgradeable.sol:24
• GraphUpgradeable.sol:32

Total gas saved: 9 * 64 = 576

5. delete optimization

Use delete instead of set to default value (false or 0).

5 gas could be saved per entry in the following affected lines:

Affected source code:

• L1GraphTokenGateway.sol:167
• GraphTokenUpgradeable.sol:187

Total gas saved: 5 * 2 = 10

6. Change bool to uint256 can save gas

Because each write operation requires an additional SLOAD to read the slot's contents, replace the bits occupied by the boolean, and then write back, booleans are more expensive than uint256 or any other type that uses a complete word. This cannot be turned off because it is the compiler's defense against pointer aliasing and contract upgrades.

Reference:

• https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/security/ReentrancyGuard.sol#L23-L27

Also, this is applicable to integer types different from uint256 or int256.

Affected source code for booleans:

• L1GraphTokenGateway.sol:35
• GraphTokenUpgradeable.sol:51
• Pausable.sol:8
• Pausable.sol:10

7. There's no need to set default values for variables

If a variable is not set/initialized, the default value is assumed (0, false, 0x0 ... depending on the data type). You are simply wasting gas if you directly initialize it with its default value.

Proof of concept (without optimizations):

pragma solidity 0.8.15;

contract TesterA {
function testInit() public view returns (uint) { uint a = 0; return a; }
}

contract TesterB {
function testNoInit() public view returns (uint) { uint a; return a; }
}

Gas saving executing: 8 per entry

TesterA.testInit:   21392
TesterB.testNoInit: 21384

Affected source code:

• L1GraphTokenGateway.sol:167
• Governed.sol:63
• GraphTokenUpgradeable.sol:187

Total gas saved: 8 * 3 = 24

8. Gas saving using immutable

It's possible to avoid storage access a save gas using immutable keyword for the following variables:

It's also better to remove the initial values, because they will be set during the constructor.

Affected source code:

• Managed.sol:114
• Managed.sol:122
• Managed.sol:130
• Managed.sol:138
• Managed.sol:146
• Managed.sol:154

9. Optimize Governed.transferOwnership

It's possible to save a variable emitting the event before set, like that:

    function transferOwnership(address _newGovernor) external onlyGovernor {
        require(_newGovernor != address(0), "Governor must be set");

+       emit NewPendingOwnership(pendingGovernor, _newGovernor);
-       address oldPendingGovernor = pendingGovernor;
        pendingGovernor = _newGovernor;

-       emit NewPendingOwnership(oldPendingGovernor, pendingGovernor);
    }

Affected source code:

• Governed.sol:41-46

10. Optimize Governed.acceptOwnership

It's possible to save two variables emitting the event before set, like that:

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

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

        governor = pendingGovernor;
        pendingGovernor = address(0);

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

Affected source code:

• Governed.sol:53-67

11. constants expressions are expressions, not constants

Due to how constant variables are implemented (replacements at compile-time), an expression assigned to a constant variable is recomputed each time that the variable is used, which wastes some gas.

If the variable was immutable instead: the calculation would only be done once at deploy time (in the constructor), and then the result would be saved and read directly at runtime rather than being recalculated.

Reference:

• https://github.com/ethereum/solidity/issues/9232

Consequences: each usage of a "constant" costs ~100gas more on each access (it is still a little better than storing the result in storage, but not much..). since these are not real constants, they can't be referenced from a real constant environment (e.g. from assembly, or from another library).

Affected source code:

• GraphTokenUpgradeable.sol:34-37
• GraphTokenUpgradeable.sol:38
• GraphTokenUpgradeable.sol:39
• GraphTokenUpgradeable.sol:40-41
• GraphTokenUpgradeable.sol:42-45

12. Optimize GraphTokenUpgradeable.permit

It's possible to save a method call like that:

    function permit(
        address _owner,
        address _spender,
        uint256 _value,
        uint256 _deadline,
        uint8 _v,
        bytes32 _r,
        bytes32 _s
    ) external {
        bytes32 digest = keccak256(
            abi.encodePacked(
                "\x19\x01",
                DOMAIN_SEPARATOR,
                keccak256(
                    abi.encode(PERMIT_TYPEHASH, _owner, _spender, _value, nonces[_owner], _deadline)
                )
            )
        );

        address recoveredAddress = ECDSA.recover(digest, _v, _r, _s);
        require(_owner == recoveredAddress, "GRT: invalid permit");
        require(_deadline == 0 || block.timestamp <= _deadline, "GRT: expired permit");

-       nonces[_owner] = nonces[_owner].add(1);
+       nonces[_owner] = nonces[_owner] + 1;
        _approve(_owner, _spender, _value);
    }

Affected source code:

• GraphTokenUpgradeable.sol:97