code-423n4 / 2022-10-thegraph-findings

0 stars 0 forks source link

Gas Optimizations #260

Open code423n4 opened 1 year ago

code423n4 commented 1 year ago

Require revert string is too long

The revert strings below can be shortened to 32 characters or fewer (as shown) to save gas (or else consider using custom error codes, which could save even more gas)


GraphUpgradeable.sol: L32

        require(msg.sender == _implementation(), "Caller must be the implementation");

Change message to Caller must be the impl


GraphProxy.sol: L141

        require(Address.isContract(_pendingImplementation), "Implementation must be a contract");

Change message to Implementation must be contract


GraphProxy.sol: L142-145

        require(
            _pendingImplementation != address(0) && msg.sender == _pendingImplementation,
            "Caller must be the pending implementation"
        );

Change message to Caller must be the pending impl


Managed.sol: L53

        require(msg.sender == controller.getGovernor(), "Caller must be Controller governor");

Change message to Caller must be Controller gov


GraphTokenGateway.sol: L21

            "Only Governor or Guardian can call"

Change message to Only Gov or Guardian can call


The revert string below is also longer than 32 characters but it's not clear how to shorten it

GraphProxy.sol: L105

        require(_newAdmin != address(0), "Cannot change the admin of a proxy to the zero address");


Avoid use of '&&' within a require function

Splitting into separate require() statements saves gas


Governed.sol: L54-57

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

Recommendation:

        require(pendingGovernor != address(0), "Caller must be pending governor");
        require(msg.sender == pendingGovernor, "Caller must be pending governor");

GraphProxy.sol: L142-145

        require(
            _pendingImplementation != address(0) && msg.sender == _pendingImplementation,
            "Caller must be the pending implementation"
        );

Recommendation:

        require(_pendingImplementation != address(0), "Caller must be the pending impl");
        require(msg.sender == _pendingImplementation, "Caller must be the pending impl");

L1GraphTokenGateway.sol: L142

        require(_escrow != address(0) && Address.isContract(_escrow), "INVALID_ESCROW");

Recommendation:

        require(_escrow != address(0), "Invalid escrow");
        require(Address.isContract(_escrow), "Invalid escrow");


Use != 0 instead of > 0 in a require statement if variable is an unsigned integer

!= 0 should be used where possible since > 0 costs more gas


L1GraphTokenGateway.sol: L217

                require(maxSubmissionCost > 0, "NO_SUBMISSION_COST");

Change maxSubmissionCost > 0 to maxSubmissionCost != 0



Inline a function/modifier that’s only used once

It costs less gas to inline the code of functions that are only called once. Doing so saves the cost of allocating the function selector, and the cost of the jump when the function is called.


Managed.sol: L70-74

    // Check if sender is controller.
    modifier onlyController() {
        _onlyController();
        _;
    }

Since onlyController() is used only once in this contract (in function setController()) as shown below, it should be inlined to save gas

Managed.sol: L95-97

    function setController(address _controller) external onlyController {
        _setController(_controller);
    }

GraphTokenUpgradeable.sol: L59-62

    modifier onlyMinter() {
        require(isMinter(msg.sender), "Only minter can call");
        _;
    }

Since onlyMinter() is used only once in this contract (in function mint()) as shown below, it should be inlined to save gas

GraphTokenUpgradeable.sol: L132-134

    function mint(address _to, uint256 _amount) external onlyMinter {
        _mint(_to, _amount);
    }

L2GraphTokenGateway.sol: L68-74

    modifier onlyL1Counterpart() {
        require(
            msg.sender == AddressAliasHelper.applyL1ToL2Alias(l1Counterpart),
            "ONLY_COUNTERPART_GATEWAY"
        );
        _;
    }

Since onlyL1Counterpart() is used only once in this contract (in function finalizeInboundTransfer()) as shown below, it should be inlined to save gas

L2GraphTokenGateway.sol: L226-232

    function finalizeInboundTransfer(
        address _l1Token,
        address _from,
        address _to,
        uint256 _amount,
        bytes calldata _data
    ) external payable override notPaused onlyL1Counterpart nonReentrant {

L1GraphTokenGateway.sol: L73-74

    modifier onlyL2Counterpart() {
        require(inbox != address(0), "INBOX_NOT_SET");

Since onlyL2Counterpart() is used only once in this contract (in function finalizeInboundTransfer()) as shown below, it should be inlined to save gas

L1GraphTokenGateway.sol: L263-269

    function finalizeInboundTransfer(
        address _l1Token,
        address _from,
        address _to,
        uint256 _amount,
        bytes calldata // _data, contains exitNum, unused by this contract
    ) external payable override notPaused onlyL2Counterpart {

GraphTokenGateway.sol: L18-22

    modifier onlyGovernorOrGuardian() {
        require(
            msg.sender == controller.getGovernor() || msg.sender == pauseGuardian,
            "Only Governor or Guardian can call"
        );

Since onlyGovernorOrGuardian() is used only once in this contract (in function setPaused()) as shown below, it should be inlined to save gas

GraphTokenGateway.sol: L47-49

    function setPaused(bool _newPaused) external onlyGovernorOrGuardian {
        _setPaused(_newPaused);
    }

The following two modifiers are never used and should be reviewed:

GraphProxyStorage.sol: L59-64

     * @dev Modifier to check whether the `msg.sender` is the admin.
     */
    modifier onlyAdmin() {
        require(msg.sender == _admin(), "Caller must be admin");
        _;
    }

Managed.sol: L60-63

    modifier notPartialPaused() {
        _notPartialPaused();
        _;
    }