fix: standardize function order in some contracts (C4 QA)

pcarranzav · pcarranzav · commit ae03579645da · 2022-10-28T17:36:32.000-03:00
diff --git a/contracts/gateway/GraphTokenGateway.sol b/contracts/gateway/GraphTokenGateway.sol
@@ -35,14 +35,6 @@ abstract contract GraphTokenGateway is GraphUpgradeable, Pausable, Managed, ITok
         _setPauseGuardian(_newPauseGuardian);
     }
 
-    /**
-     * @dev Override the default pausing from Managed to allow pausing this
-     * particular contract instead of pausing from the Controller.
-     */
-    function _notPaused() internal view override {
-        require(!_paused, "Paused (contract)");
-    }
-
     /**
      * @notice Change the paused state of the contract
      * @param _newPaused New value for the pause state (true means the transfers will be paused)
@@ -62,6 +54,14 @@ abstract contract GraphTokenGateway is GraphUpgradeable, Pausable, Managed, ITok
         return _paused;
     }
 
+    /**
+     * @dev Override the default pausing from Managed to allow pausing this
+     * particular contract instead of pausing from the Controller.
+     */
+    function _notPaused() internal view override {
+        require(!_paused, "Paused (contract)");
+    }
+
     /**
      * @dev Runs state validation before unpausing, reverts if
      * something is not set properly
diff --git a/contracts/gateway/L1GraphTokenGateway.sol b/contracts/gateway/L1GraphTokenGateway.sol
@@ -223,7 +223,7 @@ contract L1GraphTokenGateway is Initializable, GraphTokenGateway, L1ArbitrumMess
             bytes memory outboundCalldata;
             {
                 bytes memory extraData;
-                (from, maxSubmissionCost, extraData) = parseOutboundData(_data);
+                (from, maxSubmissionCost, extraData) = _parseOutboundData(_data);
                 require(
                     extraData.length == 0 || callhookAllowlist[msg.sender] == true,
                     "CALL_HOOK_DATA_NOT_ALLOWED"
@@ -285,37 +285,17 @@ contract L1GraphTokenGateway is Initializable, GraphTokenGateway, L1ArbitrumMess
     }
 
     /**
-     * @notice Decodes calldata required for migration of tokens
-     * @dev Data must include maxSubmissionCost, extraData can be left empty. When the router
-     * sends an outbound message, data also contains the from address.
-     * @param _data encoded callhook data
-     * @return Sender of the tx
-     * @return Base ether value required to keep retryable ticket alive
-     * @return Additional data sent to L2
+     * @notice Calculate the L2 address of a bridged token
+     * @dev In our case, this would only work for GRT.
+     * @param _l1ERC20 address of L1 GRT contract
+     * @return L2 address of the bridged GRT token
      */
-    function parseOutboundData(bytes calldata _data)
-        private
-        view
-        returns (
-            address,
-            uint256,
-            bytes memory
-        )
-    {
-        address from;
-        uint256 maxSubmissionCost;
-        bytes memory extraData;
-        if (msg.sender == l1Router) {
-            // Data encoded by the Gateway Router includes the sender address
-            (from, extraData) = abi.decode(_data, (address, bytes));
-        } else {
-            from = msg.sender;
-            extraData = _data;
+    function calculateL2TokenAddress(address _l1ERC20) external view override returns (address) {
+        IGraphToken token = graphToken();
+        if (_l1ERC20 != address(token)) {
+            return address(0);
         }
-        // User-encoded data contains the max retryable ticket submission cost
-        // and additional L2 calldata
-        (maxSubmissionCost, extraData) = abi.decode(extraData, (uint256, bytes));
-        return (from, maxSubmissionCost, extraData);
+        return l2GRT;
     }
 
     /**
@@ -349,20 +329,6 @@ contract L1GraphTokenGateway is Initializable, GraphTokenGateway, L1ArbitrumMess
             );
     }
 
-    /**
-     * @notice Calculate the L2 address of a bridged token
-     * @dev In our case, this would only work for GRT.
-     * @param _l1ERC20 address of L1 GRT contract
-     * @return L2 address of the bridged GRT token
-     */
-    function calculateL2TokenAddress(address _l1ERC20) external view override returns (address) {
-        IGraphToken token = graphToken();
-        if (_l1ERC20 != address(token)) {
-            return address(0);
-        }
-        return l2GRT;
-    }
-
     /**
      * @dev Runs state validation before unpausing, reverts if
      * something is not set properly
@@ -373,4 +339,38 @@ contract L1GraphTokenGateway is Initializable, GraphTokenGateway, L1ArbitrumMess
         require(l2Counterpart != address(0), "L2_COUNTERPART_NOT_SET");
         require(escrow != address(0), "ESCROW_NOT_SET");
     }
+
+    /**
+     * @notice Decodes calldata required for migration of tokens
+     * @dev Data must include maxSubmissionCost, extraData can be left empty. When the router
+     * sends an outbound message, data also contains the from address.
+     * @param _data encoded callhook data
+     * @return Sender of the tx
+     * @return Base ether value required to keep retryable ticket alive
+     * @return Additional data sent to L2
+     */
+    function _parseOutboundData(bytes calldata _data)
+        private
+        view
+        returns (
+            address,
+            uint256,
+            bytes memory
+        )
+    {
+        address from;
+        uint256 maxSubmissionCost;
+        bytes memory extraData;
+        if (msg.sender == l1Router) {
+            // Data encoded by the Gateway Router includes the sender address
+            (from, extraData) = abi.decode(_data, (address, bytes));
+        } else {
+            from = msg.sender;
+            extraData = _data;
+        }
+        // User-encoded data contains the max retryable ticket submission cost
+        // and additional L2 calldata
+        (maxSubmissionCost, extraData) = abi.decode(extraData, (uint256, bytes));
+        return (from, maxSubmissionCost, extraData);
+    }
 }
diff --git a/contracts/l2/gateway/L2GraphTokenGateway.sol b/contracts/l2/gateway/L2GraphTokenGateway.sol
@@ -122,6 +122,65 @@ contract L2GraphTokenGateway is GraphTokenGateway, L2ArbitrumMessenger, Reentran
         emit L1CounterpartAddressSet(_l1Counterpart);
     }
 
+    /**
+     * @notice Burns L2 tokens and initiates a transfer to L1.
+     * The tokens will be received on L1 only after the wait period (7 days) is over,
+     * and will require an Outbox.executeTransaction to finalize.
+     * @dev no additional callhook data is allowed
+     * @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 to send to L1
+     * @return ID of the withdraw tx
+     */
+    function outboundTransfer(
+        address _l1Token,
+        address _to,
+        uint256 _amount,
+        bytes calldata _data
+    ) external returns (bytes memory) {
+        return outboundTransfer(_l1Token, _to, _amount, 0, 0, _data);
+    }
+
+    /**
+     * @notice Receives token amount from L1 and mints the equivalent tokens to the receiving address
+     * @dev Only accepts transactions from the L1 GRT Gateway.
+     * The function is payable for ITokenGateway compatibility, but msg.value must be zero.
+     * Note that allowlisted 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 allowlist, 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.
+     * @param _l1Token L1 Address of GRT
+     * @param _from Address of the sender on L1
+     * @param _to Recipient address on L2
+     * @param _amount Amount of tokens transferred
+     * @param _data Extra callhook data, only used when the sender is allowlisted
+     */
+    function finalizeInboundTransfer(
+        address _l1Token,
+        address _from,
+        address _to,
+        uint256 _amount,
+        bytes calldata _data
+    ) external payable override nonReentrant notPaused onlyL1Counterpart {
+        require(_l1Token == l1GRT, "TOKEN_NOT_GRT");
+        require(msg.value == 0, "INVALID_NONZERO_VALUE");
+
+        L2GraphToken(calculateL2TokenAddress(l1GRT)).bridgeMint(_to, _amount);
+
+        if (_data.length > 0) {
+            bytes memory callhookData;
+            {
+                (, callhookData) = abi.decode(_data, (bytes, bytes));
+            }
+            ICallhookReceiver(_to).onTokenTransfer(_from, _amount, callhookData);
+        }
+
+        emit DepositFinalized(_l1Token, _from, _to, _amount);
+    }
+
     /**
      * @notice Burns L2 tokens and initiates a transfer to L1.
      * The tokens will be available on L1 only after the wait period (7 days) is over,
@@ -151,7 +210,7 @@ contract L2GraphTokenGateway is GraphTokenGateway, L2ArbitrumMessenger, Reentran
 
         OutboundCalldata memory outboundCalldata;
 
-        (outboundCalldata.from, outboundCalldata.extraData) = parseOutboundData(_data);
+        (outboundCalldata.from, outboundCalldata.extraData) = _parseOutboundData(_data);
         require(outboundCalldata.extraData.length == 0, "CALL_HOOK_DATA_NOT_ALLOWED");
 
         // from needs to approve this contract to burn the amount first
@@ -176,26 +235,6 @@ contract L2GraphTokenGateway is GraphTokenGateway, L2ArbitrumMessenger, Reentran
         return abi.encode(id);
     }
 
-    /**
-     * @notice Burns L2 tokens and initiates a transfer to L1.
-     * The tokens will be received on L1 only after the wait period (7 days) is over,
-     * and will require an Outbox.executeTransaction to finalize.
-     * @dev no additional callhook data is allowed
-     * @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 to send to L1
-     * @return ID of the withdraw tx
-     */
-    function outboundTransfer(
-        address _l1Token,
-        address _to,
-        uint256 _amount,
-        bytes calldata _data
-    ) external returns (bytes memory) {
-        return outboundTransfer(_l1Token, _to, _amount, 0, 0, _data);
-    }
-
     /**
      * @notice Calculate the L2 address of a bridged token
      * @dev In our case, this would only work for GRT.
@@ -209,45 +248,6 @@ contract L2GraphTokenGateway is GraphTokenGateway, L2ArbitrumMessenger, Reentran
         return address(graphToken());
     }
 
-    /**
-     * @notice Receives token amount from L1 and mints the equivalent tokens to the receiving address
-     * @dev Only accepts transactions from the L1 GRT Gateway.
-     * The function is payable for ITokenGateway compatibility, but msg.value must be zero.
-     * Note that allowlisted 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 allowlist, 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.
-     * @param _l1Token L1 Address of GRT
-     * @param _from Address of the sender on L1
-     * @param _to Recipient address on L2
-     * @param _amount Amount of tokens transferred
-     * @param _data Extra callhook data, only used when the sender is allowlisted
-     */
-    function finalizeInboundTransfer(
-        address _l1Token,
-        address _from,
-        address _to,
-        uint256 _amount,
-        bytes calldata _data
-    ) external payable override nonReentrant notPaused onlyL1Counterpart {
-        require(_l1Token == l1GRT, "TOKEN_NOT_GRT");
-        require(msg.value == 0, "INVALID_NONZERO_VALUE");
-
-        L2GraphToken(calculateL2TokenAddress(l1GRT)).bridgeMint(_to, _amount);
-
-        if (_data.length > 0) {
-            bytes memory callhookData;
-            {
-                (, callhookData) = abi.decode(_data, (bytes, bytes));
-            }
-            ICallhookReceiver(_to).onTokenTransfer(_from, _amount, callhookData);
-        }
-
-        emit DepositFinalized(_l1Token, _from, _to, _amount);
-    }
-
     /**
      * @notice Creates calldata required to send tx to L1
      * @dev encodes the target function with its params which
@@ -277,14 +277,24 @@ contract L2GraphTokenGateway is GraphTokenGateway, L2ArbitrumMessenger, Reentran
             );
     }
 
+    /**
+     * @dev Runs state validation before unpausing, reverts if
+     * something is not set properly
+     */
+    function _checksBeforeUnpause() internal view override {
+        require(l2Router != address(0), "ROUTER_NOT_SET");
+        require(l1Counterpart != address(0), "L1_COUNTERPART_NOT_SET");
+        require(l1GRT != address(0), "L1GRT_NOT_SET");
+    }
+
     /**
      * @notice Decodes calldata required for migration of tokens
      * @dev extraData can be left empty
      * @param _data Encoded callhook data
      * @return Sender of the tx
      * @return Any other data sent to L1
      */
-    function parseOutboundData(bytes calldata _data) private view returns (address, bytes memory) {
+    function _parseOutboundData(bytes calldata _data) private view returns (address, bytes memory) {
         address from;
         bytes memory extraData;
         if (msg.sender == l2Router) {
@@ -295,14 +305,4 @@ contract L2GraphTokenGateway is GraphTokenGateway, L2ArbitrumMessenger, Reentran
         }
         return (from, extraData);
     }
-
-    /**
-     * @dev Runs state validation before unpausing, reverts if
-     * something is not set properly
-     */
-    function _checksBeforeUnpause() internal view override {
-        require(l2Router != address(0), "ROUTER_NOT_SET");
-        require(l1Counterpart != address(0), "L1_COUNTERPART_NOT_SET");
-        require(l1GRT != address(0), "L1GRT_NOT_SET");
-    }
 }
diff --git a/contracts/upgrades/GraphProxy.sol b/contracts/upgrades/GraphProxy.sol
@@ -59,6 +59,22 @@ contract GraphProxy is GraphProxyStorage, IGraphProxy {
         _setPendingImplementation(_impl);
     }
 
+    /**
+     * @notice Fallback function that delegates calls to implementation. Will run if call data
+     * is empty.
+     */
+    receive() external payable {
+        _fallback();
+    }
+
+    /**
+     * @notice Fallback function that delegates calls to implementation. Will run if no other
+     * function in the contract matches the call data.
+     */
+    fallback() external payable {
+        _fallback();
+    }
+
     /**
      * @notice Get the current admin
      *
@@ -193,20 +209,4 @@ contract GraphProxy is GraphProxyStorage, IGraphProxy {
             }
         }
     }
-
-    /**
-     * @notice Fallback function that delegates calls to implementation. Will run if no other
-     * function in the contract matches the call data.
-     */
-    fallback() external payable {
-        _fallback();
-    }
-
-    /**
-     * @notice Fallback function that delegates calls to implementation. Will run if call data
-     * is empty.
-     */
-    receive() external payable {
-        _fallback();
-    }
 }
