The Graph L2 bridge contest - delfin454000's results

Typos

---

L1GraphTokenGateway.sol: L185

     * @param _amount Amount of tokens to tranfer

Change tranfer to transfer

---

L1GraphTokenGateway.sol: L260

     * @param _to Recepient address on L1

Change Recepient to Recipient

---

---

Long comment lines

In theory, comments over 80 characters should wrap using multi-line comment syntax. Even if somewhat longer comments (up to 120 characters) are acceptable and a scroll bar is provided, there are cases where very long comment lines interfere with readability. Below are two sets of comments including extra-long lines whose readability could be improved, as shown.

---

GraphProxy.sol: L65-66

     * TIP: To get this value clients can read directly from the storage slot shown below (specified by EIP1967) using the
     * https://eth.wiki/json-rpc/API#eth_getstorageat[`eth_getStorageAt`] RPC call.

Suggestion:

     * TIP: To get this value clients can read directly from the storage slot shown below (specified by EIP1967) 
     * using the https://eth.wiki/json-rpc/API#eth_getstorageat[`eth_getStorageAt`] RPC call.

Similarly for the following sets of comments:

GraphProxy.sol: L78-79

GraphProxy.sol: L91-92

---

L2GraphTokenGateway.sol: L214-219

     * Note that whitelisted senders (some protocol contracts) can include additional calldata
     * for a callhook to be executed on the L2 side when the tokens are received. In this case, the L2 transaction
     * can revert if the callhook reverts, potentially locking the tokens on the bridge if the callhook
     * never succeeds. This requires extra care when adding contracts to the whitelist, but is necessary to ensure that
     * the tickets can be retried in the case of a temporary failure, and to ensure the atomicity of callhooks
     * with token transfers.

Suggestion:

     * Note that whitelisted senders (some protocol contracts) can include additional calldata
     * for a callhook to be executed on the L2 side when the tokens are received. In this case, 
     * the L2 transaction can revert if the callhook reverts, potentially locking the tokens on the bridge 
     * if the callhook never succeeds. This requires extra care when adding contracts to the whitelist,
     * but is necessary to ensure that the tickets can be retried in the case of a temporary failure,
     * and to ensure the atomicity of callhooks with token transfers.

Similarly for the following set of comments:

L1GraphTokenGateway.sol: L177-182

---

---

Open items

Code that contains open items should be addressed and modified or removed

L2GraphTokenGateway.sol: L131-144

     * @param _l1Token L1 Address of GRT (needed for compatibility with Arbitrum Gateway Router)
     * @param _to Recipient address on L1
     * @param _amount Amount of tokens to burn
     * @param _data Contains sender and additional data (always empty) to send to L1
     * @return ID of the withdraw transaction
     */
    function outboundTransfer(
        address _l1Token,
        address _to,
        uint256 _amount,
        uint256, // unused on L2
        uint256, // unused on L2
        bytes calldata _data
    ) public payable override notPaused nonReentrant returns (bytes memory) {

Suggestion: Remove the line uint256, // unused on L2, which occurs twice

---

---

Update sensitive terms in both comments and code

Terms incorporating "black," "white," "slave" or "master" are potentially problematic. Substituting more neutral terminology is becoming common practice. I see that "blacklist" has been eliminated by using denylist. However, the contracts still use whitelist and master, as shown in the two examples below:

---

L1GraphTokenGateway.sol: L152-157

    function addToCallhookWhitelist(address _newWhitelisted) external onlyGovernor {
        require(_newWhitelisted != address(0), "INVALID_ADDRESS");
        require(!callhookWhitelist[_newWhitelisted], "ALREADY_WHITELISTED");
        callhookWhitelist[_newWhitelisted] = true;
        emit AddedToCallhookWhitelist(_newWhitelisted);
    }

---

ICuration.sol: L16

    function setCurationTokenMaster(address _curationTokenMaster) external;

---

Suggestion: Replace whitelist with allowlist, and TokenMaster with TokenPrimary or TokenAdmin. Similarly for variations of these terms.

---

---

Capitalization of require revert strings should be consistent

For the sake of readability, revert strings should have consistent punctuation throughout The Graph L2

Some error messages are written using ALL CAPS with underscores between words, as the example below shows:

L2GraphTokenGateway.sol: L98

        require(_l2Router != address(0), "INVALID_L2_ROUTER");

Other error messages are written in a more conversational style without using ALL CAPS or underscores:

GraphUpgradeable.sol: L24

        require(msg.sender == _proxy.admin(), "Caller must be the proxy admin");

Suggestion: Convert ALL CAP + underscore messages to the more conversational capital + lower case without underscores style

---

---

Missing require revert string

A require message should be included to enable users to understand the reason for failure

Recommendation: Add a brief (<= 32 char) explanatory message to each of the four requires referenced below. Also, consider using custom error codes (which would be cheaper than revert strings).

---

GraphProxyAdmin.sol: L30-36

    function getProxyImplementation(IGraphProxy _proxy) public view returns (address) {
        // We need to manually run the static call since the getter cannot be flagged as view
        // bytes4(keccak256("implementation()")) == 0x5c60da1b
        (bool success, bytes memory returndata) = address(_proxy).staticcall(hex"5c60da1b");
        require(success);
        return abi.decode(returndata, (address));
    }

Similarly for the following:

GraphProxyAdmin.sol: L43-49

GraphProxyAdmin.sol: L55-61

GraphProxy.sol: L129-134

---

---

NatSpec is incomplete

Specific NatSpec statements are missing for the 15 functions below:

GraphUpgradeable.sol: L48-50

     * @dev Accept to be an implementation of proxy.
     */
    function acceptProxy(IGraphProxy _proxy) external onlyProxyAdmin(_proxy) {

Missing: @param _proxy

---

GraphUpgradeable.sol: L55-59

     * @dev Accept to be an implementation of proxy and then call a function from the new
     * implementation as specified by `_data`, which should be an encoded function call. This is
     * useful to initialize new storage variables in the proxied contract.
     */
    function acceptProxyAndCall(IGraphProxy _proxy, bytes calldata _data)

Missing: @param _proxy and @param _data

---

Governed.sol: L29-31

     * @dev Initialize the governor to the contract caller.
     */
    function _initialize(address _initGovernor) internal {

Missing: @param _initGovernor

---

Pausable.sol: L24-26

     * @notice Change the partial paused state of the contract
     */
    function _setPartialPaused(bool _toPause) internal {

Missing: @param _toPause

---

Pausable.sol: L38-40

     * @notice Change the paused state of the contract
     */
    function _setPaused(bool _toPause) internal {

Missing: @param _toPause

---

GraphProxyAdmin.sol: L26-30

     * @dev Returns the current implementation of a proxy.
     * This is needed because only the proxy admin can query it.
     * @return The address of the current implementation of the proxy.
     */
    function getProxyImplementation(IGraphProxy _proxy) public view returns (address) {

Missing: @param _proxy

---

GraphProxyAdmin.sol: L39-43

     * @dev Returns the pending implementation of a proxy.
     * This is needed because only the proxy admin can query it.
     * @return The address of the pending implementation of the proxy.
     */
    function getProxyPendingImplementation(IGraphProxy _proxy) public view returns (address) {

Missing: @param _proxy

---

GraphProxyAdmin.sol: L52-55

     * @dev Returns the admin of a proxy. Only the admin can query it.
     * @return The address of the current admin of the proxy.
     */
    function getProxyAdmin(IGraphProxy _proxy) public view returns (address) {

Missing: @param _proxy

---

GraphProxy.sol: L61-69

     * @dev Returns the current admin.
     *
     * NOTE: Only the admin and implementation can call this function.
     *
     * TIP: To get this value clients can read directly from the storage slot shown below (specified by EIP1967) using the
     * https://eth.wiki/json-rpc/API#eth_getstorageat[`eth_getStorageAt`] RPC call.
     * `0xb53127684a568b3173ae13b9f8a6016e243e63b6e8ee1178d6a717850b5d6103`
     */
    function admin() external ifAdminOrPendingImpl returns (address) {

Missing: @return. @dev should contain some of the information in the notes so that @return is not redundant

---

GraphProxy.sol: L74-82

     * @dev Returns the current implementation.
     *
     * NOTE: Only the admin can call this function.
     *
     * TIP: To get this value clients can read directly from the storage slot shown below (specified by EIP1967) using the
     * https://eth.wiki/json-rpc/API#eth_getstorageat[`eth_getStorageAt`] RPC call.
     * `0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc`
     */
    function implementation() external ifAdminOrPendingImpl returns (address) {

Missing: @return. Again, @dev should contain some of the information in the notes so that @return is not redundant

---

GraphProxy.sol: L100-104

     * @dev Changes the admin of the proxy.
     *
     * NOTE: Only the admin can call this function.
     */
    function setAdmin(address _newAdmin) external ifAdmin {

Missing: @param _newAdmin

---

GraphProxy.sol: L127-129

     * @dev Admin function for new implementation to accept its role as implementation.
     */
    function acceptUpgradeAndCall(bytes calldata data) external ifAdminOrPendingImpl {

Missing: @param data

---

Managed.sol: L85-87

     * @dev Initialize the controller.
     */
    function _initialize(address _controller) internal {

Missing: @param _controller

---

Managed.sol: L158-161

     * @dev Resolve a contract address from the cache or the Controller if not found.
     * @return Address of the contract
     */
    function _resolveContract(bytes32 _nameHash) internal view returns (address) {

Missing: @param _nameHash

---

GraphTokenGateway.sol: L52-54

     * @notice Getter to access paused state of this contract
     */
    function paused() external view returns (bool) {

Missing: @return

---

---

NatSpec statements are missing

NatSpec statements are entirely missing for the functions listed below. In some cases, a group of functions is proceeded by a comment such as // -- Configuration -- or // -- Getters --. However, such comments, which serve as explanatory headings for the functions, are not as helpful as would be @notice statements (to explain to end users what a function does) and @param and @return statements (which describe the roles of individual variables).

Managed.sol

L43; L48; L52; L56

IGraphCurationToken.sol

L8; L10; L12

IGraphProxy.sol

L6; L8; L10; L12; L14; L16; L18

IEpochManager.sol

L8; L12; L16; L18; L20; L22; L24; L26; L28; L30

IController.sol

L6; L10; L12; L14; L16; L20; L22; L24; L26; L28

IGraphToken.sol

L10; L12; L14; L18; L20; L22; L24; L28-36; L40; L42

IRewardsManager.sol

L18; L20; L24; L26; L28; L31; L35; L37; L39-42; L44-47; L49; L53; L55; L59; L61

ICuration.sol

L10; L12; L14; L16; L20-24; L26-30; L32; L36; L38-41; L43; L45; L47-50; L52-55; L57

IStaking.sol

L30; L32; L34; L36; L38; L40; L42; L44; L46-50; L52; L54; L56; L58; L60; L64; L66; L70; L72; L74; L76-81; L83; L85; L89; L91; L93; L97-103; L105-112; L114; L116; L118-127; L129; L131; L133; L137; L139; L141; L143; L145; L147; L149-152; L154-157; L159

---

---

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();
        _;
    }

---

---