fix: improve NatSpec and a few other details (C4 QA)

Author: pcarranzav

contracts/gateway/BridgeEscrow.sol

@@ -16,23 +16,23 @@ import "../token/IGraphToken.sol";
  */
 contract BridgeEscrow is Initializable, GraphUpgradeable, Managed {
     /**
-     * @dev Initialize this contract.
+     * @notice Initialize the BridgeEscrow contract.
      * @param _controller Address of the Controller that manages this contract
      */
     function initialize(address _controller) external onlyImpl initializer {
         Managed._initialize(_controller);
     }
 
     /**
-     * @dev Approve a spender (i.e. a bridge that manages the GRT funds held by the escrow)
+     * @notice Approve a spender (i.e. a bridge that manages the GRT funds held by the escrow)
      * @param _spender Address of the spender that will be approved
      */
     function approveAll(address _spender) external onlyGovernor {
         graphToken().approve(_spender, type(uint256).max);
     }
 
     /**
-     * @dev Revoke a spender (i.e. a bridge that will no longer manage the GRT funds held by the escrow)
+     * @notice Revoke a spender (i.e. a bridge that will no longer manage the GRT funds held by the escrow)
      * @param _spender Address of the spender that will be revoked
      */
     function revokeAll(address _spender) external onlyGovernor {

contracts/gateway/GraphTokenGateway.sol

@@ -12,7 +12,7 @@ import "../governance/Managed.sol";
  * @dev This includes everything that's shared between the L1 and L2 sides of the bridge.
  */
 abstract contract GraphTokenGateway is GraphUpgradeable, Pausable, Managed, ITokenGateway {
-    // Storage gap added in case we need to add state variables to this contract
+    /// @dev Storage gap added in case we need to add state variables to this contract
     uint256[50] private __gap;
 
     /**
@@ -56,6 +56,7 @@ abstract contract GraphTokenGateway is GraphUpgradeable, Pausable, Managed, ITok
 
     /**
      * @notice Getter to access paused state of this contract
+     * @return True if the contract is paused, false otherwise
      */
     function paused() external view returns (bool) {
         return _paused;

contracts/gateway/ICallhookReceiver.sol

@@ -10,7 +10,7 @@ pragma solidity ^0.7.6;
 
 interface ICallhookReceiver {
     /**
-     * @dev Receive tokens with a callhook from the bridge
+     * @notice Receive tokens with a callhook from the bridge
      * @param _from Token sender in L1
      * @param _amount Amount of tokens that were transferred
      * @param _data ABI-encoded callhook data

contracts/gateway/L1GraphTokenGateway.sol

@@ -22,20 +22,20 @@ import "./GraphTokenGateway.sol";
 contract L1GraphTokenGateway is Initializable, GraphTokenGateway, L1ArbitrumMessenger {
     using SafeMath for uint256;
 
-    // Address of the Graph Token contract on L2
+    /// Address of the Graph Token contract on L2
     address public l2GRT;
-    // Address of the Arbitrum Inbox
+    /// Address of the Arbitrum Inbox
     address public inbox;
-    // Address of the Arbitrum Gateway Router on L1
+    /// Address of the Arbitrum Gateway Router on L1
     address public l1Router;
-    // Address of the L2GraphTokenGateway on L2 that is the counterpart of this gateway
+    /// Address of the L2GraphTokenGateway on L2 that is the counterpart of this gateway
     address public l2Counterpart;
-    // Address of the BridgeEscrow contract that holds the GRT in the bridge
+    /// Address of the BridgeEscrow contract that holds the GRT in the bridge
     address public escrow;
-    // Addresses for which this mapping is true are allowed to send callhooks in outbound transfers
+    /// Addresses for which this mapping is true are allowed to send callhooks in outbound transfers
     mapping(address => bool) public callhookWhitelist;
 
-    // Emitted when an outbound transfer is initiated, i.e. tokens are deposited from L1 to L2
+    /// Emitted when an outbound transfer is initiated, i.e. tokens are deposited from L1 to L2
     event DepositInitiated(
         address l1Token,
         address indexed from,
@@ -44,7 +44,7 @@ contract L1GraphTokenGateway is Initializable, GraphTokenGateway, L1ArbitrumMess
         uint256 amount
     );
 
-    // Emitted when an incoming transfer is finalized, i.e tokens are withdrawn from L2 to L1
+    /// Emitted when an incoming transfer is finalized, i.e tokens are withdrawn from L2 to L1
     event WithdrawalFinalized(
         address l1Token,
         address indexed from,
@@ -53,17 +53,17 @@ contract L1GraphTokenGateway is Initializable, GraphTokenGateway, L1ArbitrumMess
         uint256 amount
     );
 
-    // Emitted when the Arbitrum Inbox and Gateway Router addresses have been updated
+    /// Emitted when the Arbitrum Inbox and Gateway Router addresses have been updated
     event ArbitrumAddressesSet(address inbox, address l1Router);
-    // Emitted when the L2 GRT address has been updated
+    /// Emitted when the L2 GRT address has been updated
     event L2TokenAddressSet(address l2GRT);
-    // Emitted when the counterpart L2GraphTokenGateway address has been updated
+    /// Emitted when the counterpart L2GraphTokenGateway address has been updated
     event L2CounterpartAddressSet(address l2Counterpart);
-    // Emitted when the escrow address has been updated
+    /// Emitted when the escrow address has been updated
     event EscrowAddressSet(address escrow);
-    // Emitted when an address is added to the callhook whitelist
+    /// Emitted when an address is added to the callhook whitelist
     event AddedToCallhookWhitelist(address newWhitelisted);
-    // Emitted when an address is removed from the callhook whitelist
+    /// Emitted when an address is removed from the callhook whitelist
     event RemovedFromCallhookWhitelist(address notWhitelisted);
 
     /**
@@ -86,8 +86,8 @@ contract L1GraphTokenGateway is Initializable, GraphTokenGateway, L1ArbitrumMess
     }
 
     /**
-     * @dev Initialize this contract.
-     * The contract will be paused.
+     * @notice Initialize the L1GraphTokenGateway contract.
+     * @dev The contract will be paused.
      * Note some parameters have to be set separately as they are generally
      * not expected to be available at initialization time:
      * - inbox  and l1Router using setArbitrumAddresses
@@ -104,7 +104,7 @@ contract L1GraphTokenGateway is Initializable, GraphTokenGateway, L1ArbitrumMess
     }
 
     /**
-     * @dev Sets the addresses for L1 contracts provided by Arbitrum
+     * @notice Sets the addresses for L1 contracts provided by Arbitrum
      * @param _inbox Address of the Inbox that is part of the Arbitrum Bridge
      * @param _l1Router Address of the Gateway Router
      */
@@ -119,7 +119,7 @@ contract L1GraphTokenGateway is Initializable, GraphTokenGateway, L1ArbitrumMess
     }
 
     /**
-     * @dev Sets the address of the L2 Graph Token
+     * @notice Sets the address of the L2 Graph Token
      * @param _l2GRT Address of the GRT contract on L2
      */
     function setL2TokenAddress(address _l2GRT) external onlyGovernor {
@@ -129,7 +129,7 @@ contract L1GraphTokenGateway is Initializable, GraphTokenGateway, L1ArbitrumMess
     }
 
     /**
-     * @dev Sets the address of the counterpart gateway on L2
+     * @notice Sets the address of the counterpart gateway on L2
      * @param _l2Counterpart Address of the corresponding L2GraphTokenGateway on Arbitrum
      */
     function setL2CounterpartAddress(address _l2Counterpart) external onlyGovernor {
@@ -139,7 +139,7 @@ contract L1GraphTokenGateway is Initializable, GraphTokenGateway, L1ArbitrumMess
     }
 
     /**
-     * @dev Sets the address of the escrow contract on L1
+     * @notice Sets the address of the escrow contract on L1
      * @param _escrow Address of the BridgeEscrow
      */
     function setEscrowAddress(address _escrow) external onlyGovernor {
@@ -150,7 +150,7 @@ contract L1GraphTokenGateway is Initializable, GraphTokenGateway, L1ArbitrumMess
     }
 
     /**
-     * @dev Adds an address to the callhook whitelist.
+     * @notice Adds an address to the callhook whitelist.
      * This address will be allowed to include callhooks when transferring tokens.
      * @param _newWhitelisted Address to add to the whitelist
      */
@@ -163,7 +163,7 @@ contract L1GraphTokenGateway is Initializable, GraphTokenGateway, L1ArbitrumMess
     }
 
     /**
-     * @dev Removes an address from the callhook whitelist.
+     * @notice Removes an address from the callhook whitelist.
      * This address will no longer be allowed to include callhooks when transferring tokens.
      * @param _notWhitelisted Address to remove from the whitelist
      */

contracts/governance/Controller.sol

@@ -13,13 +13,14 @@ import "./Pausable.sol";
  * https://github.com/livepeer/protocol/blob/streamflow/contracts/Controller.sol
  */
 contract Controller is Governed, Pausable, IController {
-    // Track contract ids to contract proxy address
+    /// @dev Track contract ids to contract proxy address
     mapping(bytes32 => address) private registry;
 
+    /// Emitted when the proxy address for a protocol contract has been set
     event SetContractProxy(bytes32 indexed id, address contractAddress);
 
     /**
-     * @dev Contract constructor.
+     * @notice Controller contract constructor.
      */
     constructor() {
         Governed._initialize(msg.sender);
@@ -74,6 +75,7 @@ contract Controller is Governed, Pausable, IController {
     /**
      * @notice Get contract proxy address by its id
      * @param _id Contract id
+     * @return Address of the proxy contract for the provided id
      */
     function getContractProxy(bytes32 _id) public view override returns (address) {
         return registry[_id];
@@ -120,13 +122,15 @@ contract Controller is Governed, Pausable, IController {
 
     /**
      * @notice Getter to access paused
+     * @return True if the contracts are paused, false otherwise
      */
     function paused() external view override returns (bool) {
         return _paused;
     }
 
     /**
      * @notice Getter to access partial pause status
+     * @return True if the contracts are partially paused, false otherwise
      */
     function partialPaused() external view override returns (bool) {
         return _partialPaused;

contracts/governance/Governed.sol

@@ -9,12 +9,16 @@ pragma solidity ^0.7.6;
 contract Governed {
     // -- State --
 
+    /// Address of the governor
     address public governor;
+    /// Address of the new governor that is pending acceptance
     address public pendingGovernor;
 
     // -- Events --
 
+    /// Emitted when a new owner/governor has been set, but is pending acceptance
     event NewPendingOwnership(address indexed from, address indexed to);
+    /// Emitted when a new owner/governor has accepted their role
     event NewOwnership(address indexed from, address indexed to);
 
     /**
@@ -26,14 +30,15 @@ contract Governed {
     }
 
     /**
-     * @dev Initialize the governor to the contract caller.
+     * @dev Initialize the governor for this contract
+     * @param _initGovernor Address of the governor
      */
     function _initialize(address _initGovernor) internal {
         governor = _initGovernor;
     }
 
     /**
-     * @dev Admin function to begin change of governor. The `_newGovernor` must call
+     * @notice Admin function to begin change of governor. The `_newGovernor` must call
      * `acceptOwnership` to finalize the transfer.
      * @param _newGovernor Address of new `governor`
      */
@@ -47,7 +52,7 @@ contract Governed {
     }
 
     /**
-     * @dev Admin function for pending governor to accept role and update governor.
+     * @notice Admin function for pending governor to accept role and update governor.
      * This function must called by the pending governor.
      */
     function acceptOwnership() external {