The Graph L2 bridge contest - __141345__'s results

Expressions for constant values such as a call to keccak256(),` should use immutable rather than constant

Constant expressions are left as expressions, not constants.

Instances number of this issue: When a constant declared as: 4

// l2/token/GraphTokenUpgradeable.sol
34-45:
    bytes32 private constant DOMAIN_TYPE_HASH =
        keccak256(
            "EIP712Domain(string name,string version,uint256 chainId,address verifyingContract,bytes32 salt)"
        );
    bytes32 private constant DOMAIN_NAME_HASH = keccak256("Graph Token");
    bytes32 private constant DOMAIN_VERSION_HASH = keccak256("0");

    bytes32 private constant PERMIT_TYPEHASH =
        keccak256(
            "Permit(address owner,address spender,uint256 value,uint256 nonce,uint256 deadline)"
        );

It is expected that the value should be converted into a constant value at compile time. But the expression is re-calculated each time the constant is referenced.

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 )

Suggestion: Change these expressions from constant to immutable and implement the calculation in the constructor or hardcode these values in the constants and add a comment to say how the value was calculated.

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

Use custom errors rather than revert()/require() strings

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

The demo of the gas comparison can be seen here.

Duplicated require()/assert() checks could be refactored to a modifier or function

Instances number of this issue: 21

// gateway/GraphTokenGateway.sol
31:         require(_newPauseGuardian != address(0), "PauseGuardian must be set");

// gateway/L1GraphTokenGateway.sol
74:         require(inbox != address(0), "INBOX_NOT_SET");
110:        require(_inbox != address(0), "INVALID_INBOX");
111:        require(_l1Router != address(0), "INVALID_L1_ROUTER");
122:        require(_l2GRT != address(0), "INVALID_L2_GRT");
132:        require(_l2Counterpart != address(0), "INVALID_L2_COUNTERPART");
142:        require(_escrow != address(0) && Address.isContract(_escrow), "INVALID_ESCROW");
153:        require(_newWhitelisted != address(0), "INVALID_ADDRESS");
165:        require(_notWhitelisted != address(0), "INVALID_ADDRESS");
202:        require(_to != address(0), "INVALID_DESTINATION");

// governance/Governed.sol
41:         require(_newGovernor != address(0), "Governor must be set");

// governance/Managed.sol
104:        require(_controller != address(0), "Controller must be set");

// l2/gateway/L2GraphTokenGateway.sol
98:         require(_l2Router != address(0), "INVALID_L2_ROUTER");
108:        require(_l1GRT != address(0), "INVALID_L1_GRT");
118:        require(_l1Counterpart != address(0), "INVALID_L1_COUNTERPART");
148:        require(_to != address(0), "INVALID_DESTINATION");

// l2/token/GraphTokenUpgradeable.sol
106:        require(_account != address(0), "INVALID_MINTER");

// l2/token/L2GraphToken.sol
49:         require(_owner != address(0), "Owner must be set");
60:         require(_gw != address(0), "INVALID_GATEWAY");
70:         require(_addr != address(0), "INVALID_L1_ADDRESS");

// upgrades/GraphProxy.sol
105:        require(_newAdmin != address(0), "Cannot change the admin of a proxy to the zero address");

Splitting require() statements that use &&

REQUIRE() statements with multiple conditions can be split.

See this issue which describes the fact that there is a larger deployment gas cost, but with enough runtime calls, the change ends up being cheaper.

The demo of the gas comparison can be seen here.

Instances number of this issue: 3

// gateway/L1GraphTokenGateway.sol
142:        require(_escrow != address(0) && Address.isContract(_escrow), "INVALID_ESCROW");

// governance/Governed.sol
54-57:
        require(
            pendingGovernor != address(0) && msg.sender == pendingGovernor,
            "Caller must be pending governor"
        );

// upgrades/GraphProxy.sol
142-145:
        require(
            _pendingImplementation != address(0) && msg.sender == _pendingImplementation,
            "Caller must be the pending implementation"
        );

require() strings longer than 32 bytes cost extra gas

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

Instances number of this issue: 6

// gateway/GraphTokenGateway.sol
19-22:
        require(
            msg.sender == controller.getGovernor() || msg.sender == pauseGuardian,
            "Only Governor or Guardian can call"
        );

// governance/Managed.sol
53:     require(msg.sender == controller.getGovernor(), "Caller must be Controller governor");

// upgrades/GraphProxy.sol
105:    require(_newAdmin != address(0), "Cannot change the admin of a proxy to the zero address");
141:    require(Address.isContract(_pendingImplementation), "Implementation must be a contract");
142-144:
        require(
            _pendingImplementation != address(0) && msg.sender == _pendingImplementation,
            "Caller must be the pending implementation"

//upgrades/GraphUpgradeable.sol
32:     require(msg.sender == _implementation(), "Caller must be the implementation");

Making some variables as non-public

Changing the visibility from public to private or internal can save gas when a variable isn’t used outside of its contract.

Savings are due to the compiler not having to create non-payable getter functions for deployment calldata, and not adding another entry to the method ID table.

Instances number of this issue: 1 The followings can be changed from public to internal or private:

// governance/Governed.sol
13:     address public pendingGovernor;

Update value order can be adjusted to simplify the code and save gas

For example, to update the num variable with newVal, the current way is as following:

    uint oldVal = num;
    num = newVal;
    emit update(oldVal, newVal);

If the execution order is adjusted, some operations can be saved (memory space allocation, variable assignment), reducing both the deployment and run time gas cost.

    emit update(num, newVal);
    num = newVal;

The demo of the gas comparison can be seen here.

There are multiple places can use this trick for optimization, since the updates of parameters are widely and frequently used, the optimization can be beneficial.

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

        address oldGovernor = governor;
        address oldPendingGovernor = pendingGovernor;

        governor = pendingGovernor;
        pendingGovernor = address(0);

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

can be changed to

    function acceptOwnership() external {
        emit NewOwnership(governor, pendingGovernor);
        emit NewPendingOwnership(pendingGovernor, address(0));

        governor = pendingGovernor;
        pendingGovernor = address(0);

    }

Using bool for storage incurs overhead

// Booleans are more expensive than uint256 or any type that takes up a full

// word because each write operation emits an extra SLOAD to first read the
// slot's contents, replace the bits taken up by the boolean, and then write
// back. This is the compiler's defense against contract upgrades and
// pointer aliasing, and it cannot be disabled.

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/58f635312aa21f947cae5f8578638a85aa2519f5/contracts/security/ReentrancyGuard.sol#L23-L27 Use uint256(1) and uint256(2) for true/false to avoid a Gwarmaccess (100 gas) for the extra SLOAD, and to avoid Gsset (20000 gas) when changing from false to true, after having been true in the past

Instances number of this issue:

// governance/Pausable.sol
8:      bool internal _partialPaused;
10:     bool internal _paused;

Functions guaranteed to revert when called by normal users can be marked payable

If a function modifier such as onlyOwner() is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided.

The extra opcodes avoided are

CALLVALUE(2), DUP1(3), ISZERO(3), PUSH2(3), JUMPI(10), PUSH1(3), DUP1(3), REVERT(0), JUMPDEST(1), POP(2)

which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost.

Instances number of this issue:

// gateway/BridgeEscrow.sol
20:     function initialize(address _controller) external onlyImpl {
28:     function approveAll(address _spender) external onlyGovernor {
36:     function revokeAll(address _spender) external onlyGovernor {

// gateway/GraphTokenGateway.sol
30:     function setPauseGuardian(address _newPauseGuardian) external onlyGovernor {
47:     function setPaused(bool _newPaused) external onlyGovernorOrGuardian {

// gateway/L1GraphTokenGateway.sol
99:     function initialize(address _controller) external onlyImpl {
109:    function setArbitrumAddresses(address _inbox, address _l1Router) external onlyGovernor {
121:    function setL2TokenAddress(address _l2GRT) external onlyGovernor {
131:    function setL2CounterpartAddress(address _l2Counterpart) external onlyGovernor {
141:    function setEscrowAddress(address _escrow) external onlyGovernor {
152:    function addToCallhookWhitelist(address _newWhitelisted) external onlyGovernor {
164:    function removeFromCallhookWhitelist(address _notWhitelisted) external onlyGovernor {

// governance/Governed.sol
40:     function transferOwnership(address _newGovernor) external onlyGovernor {

// governance/Managed.sol
95:     function setController(address _controller) external onlyController {

// l2/gateway/L2GraphTokenGateway.sol
87:     function initialize(address _controller) external onlyImpl {
97:     function setL2Router(address _l2Router) external onlyGovernor {
107:    function setL1TokenAddress(address _l1GRT) external onlyGovernor {
117:    function setL1CounterpartAddress(address _l1Counterpart) external onlyGovernor {

// l2/token/GraphTokenUpgradeable.sol
132:    function mint(address _to, uint256 _amount) external onlyMinter {

// l2/token/L2GraphToken.sol
48:     function initialize(address _owner) external onlyImpl {
59:     function setGateway(address _gw) external onlyGovernor {
69:     function setL1Address(address _addr) external onlyGovernor {
80:     function bridgeMint(address _account, uint256 _amount) external override onlyGateway {
90:     function bridgeBurn(address _account, uint256 _amount) external override onlyGateway {

// upgrades/GraphProxy.sol
69:     function admin() external ifAdminOrPendingImpl returns (address) {
82:     function implementation() external ifAdminOrPendingImpl returns (address) {
95:     function pendingImplementation() external ifAdminOrPendingImpl returns (address) {
104:      function setAdmin(address _newAdmin) external ifAdmin {
115:      function upgradeTo(address _newImplementation) external ifAdmin {
122:      function acceptUpgrade() external ifAdminOrPendingImpl {
129:      function acceptUpgradeAndCall(bytes calldata data) external ifAdminOrPendingImpl {

// upgrades/GraphProxyAdmin.sol
68:       function changeProxyAdmin(IGraphProxy _proxy, address _newAdmin) public onlyGovernor {
77:       function upgrade(IGraphProxy _proxy, address _implementation) public onlyGovernor {
86:       function acceptProxy(GraphUpgradeable _implementation, IGraphProxy _proxy) public onlyGovernor {
96-100:
        function acceptProxyAndCall(
            GraphUpgradeable _implementation,
            IGraphProxy _proxy,
            bytes calldata _data
        ) external onlyGovernor {

// upgrades/GraphUpgradeable.sol
50:     function acceptProxy(IGraphProxy _proxy) external onlyProxyAdmin(_proxy) {
59-61:
    function acceptProxyAndCall(IGraphProxy _proxy, bytes calldata _data)
        external
        onlyProxyAdmin(_proxy)