improve: Make changes after running Slither on SpokePool contracts

.gitignore

@@ -12,5 +12,8 @@ artifacts
 cache-zk
 artifacts-zk
 
+# Upgradeability files
+.openzeppelin
+
 # IDE
 .idea

README.md

@@ -41,6 +41,25 @@ NODE_URL_1=https://mainnet.infura.com/xxx yarn hardhat deploy --tags HubPool --n
 ETHERSCAN_API_KEY=XXX yarn hardhat etherscan-verify --network mainnet --license AGPL-3.0 --force-license --solc-input
 ```
 
+## Slither
+
+[Slither](https://github.com/crytic/slither) is a Solidity static analysis framework written in Python 3. It runs a
+suite of vulnerability detectors, prints visual information about contract details, and provides an API to easily write
+custom analyses. Slither enables developers to find vulnerabilities, enhance their code comprehension, and quickly
+prototype custom analyses.
+
+Spire-Contracts has been analyzed using `Slither@0.9.2` and no major bugs was found. To rerun the analytics, run:
+
+```sh
+slither contracts/SpokePool.sol
+\ --solc-remaps @=node_modules/@
+\ --solc-args "--optimize --optimize-runs 1000000"
+\ --filter-paths "node_modules"
+\ --exclude naming-convention
+```
+
+You can replace `SpokePool.sol` with the specific contract you want to analyze.
+
 ## ZK Sync Adapter
 
 These are special instructions for compiling and deploying contracts on `zksync`. The compile command will create `artifacts-zk` and `cache-zk` directories.

contracts/Arbitrum_SpokePool.sol

@@ -35,16 +35,17 @@ contract Arbitrum_SpokePool is SpokePool {
      * @param _crossDomainAdmin Cross domain admin to set. Can be changed by admin.
      * @param _hubPool Hub pool address to set. Can be changed by admin.
      * @param _wethAddress Weth address for this network to set.
-     * @param timerAddress Timer address to set.
+     * @param _timerAddress Timer address to set.
      */
-    constructor(
+    function initialize(
         uint32 _initialDepositId,
         address _l2GatewayRouter,
         address _crossDomainAdmin,
         address _hubPool,
         address _wethAddress,
-        address timerAddress
-    ) SpokePool(_initialDepositId, _crossDomainAdmin, _hubPool, _wethAddress, timerAddress) {
+        address _timerAddress
+    ) public initializer {
+        __SpokePool_init(_initialDepositId, _crossDomainAdmin, _hubPool, _wethAddress, _timerAddress);
         _setL2GatewayRouter(_l2GatewayRouter);
     }
 
@@ -82,6 +83,7 @@ contract Arbitrum_SpokePool is SpokePool {
         // Check that the Ethereum counterpart of the L2 token is stored on this contract.
         address ethereumTokenToBridge = whitelistedTokens[relayerRefundLeaf.l2TokenAddress];
         require(ethereumTokenToBridge != address(0), "Uninitialized mainnet token");
+        //slither-disable-next-line unused-return
         StandardBridgeLike(l2GatewayRouter).outboundTransfer(
             ethereumTokenToBridge, // _l1Token. Address of the L1 token to bridge over.
             hubPool, // _to. Withdraw, over the bridge, to the l1 hub pool contract.

contracts/Boba_SpokePool.sol

@@ -13,21 +13,21 @@ contract Boba_SpokePool is Ovm_SpokePool {
      * relay hash collisions.
      * @param _crossDomainAdmin Cross domain admin to set. Can be changed by admin.
      * @param _hubPool Hub pool address to set. Can be changed by admin.
-     * @param timerAddress Timer address to set.
+     * @param _timerAddress Timer address to set.
      */
-    constructor(
+    function initialize(
         uint32 _initialDepositId,
         address _crossDomainAdmin,
         address _hubPool,
-        address timerAddress
-    )
-        Ovm_SpokePool(
+        address _timerAddress
+    ) public initializer {
+        __OvmSpokePool_init(
             _initialDepositId,
             _crossDomainAdmin,
             _hubPool,
             0x4200000000000000000000000000000000000006,
             0xDeadDeAddeAddEAddeadDEaDDEAdDeaDDeAD0000,
-            timerAddress
-        )
-    {}
+            _timerAddress
+        );
+    }
 }

contracts/Ethereum_SpokePool.sol

@@ -2,15 +2,13 @@
 pragma solidity ^0.8.0;
 
 import "./SpokePool.sol";
-import "@openzeppelin/contracts/access/Ownable.sol";
-import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
-import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
+import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";
 
 /**
  * @notice Ethereum L1 specific SpokePool. Used on Ethereum L1 to facilitate L2->L1 transfers.
  */
-contract Ethereum_SpokePool is SpokePool, Ownable {
-    using SafeERC20 for IERC20;
+contract Ethereum_SpokePool is SpokePool, OwnableUpgradeable {
+    using SafeERC20Upgradeable for IERC20Upgradeable;
 
     /**
      * @notice Construct the Ethereum SpokePool.
@@ -19,21 +17,24 @@ contract Ethereum_SpokePool is SpokePool, Ownable {
      * relay hash collisions.
      * @param _hubPool Hub pool address to set. Can be changed by admin.
      * @param _wethAddress Weth address for this network to set.
-     * @param timerAddress Timer address to set.
+     * @param _timerAddress Timer address to set.
      */
-    constructor(
+    function initialize(
         uint32 _initialDepositId,
         address _hubPool,
         address _wethAddress,
-        address timerAddress
-    ) SpokePool(_initialDepositId, _hubPool, _hubPool, _wethAddress, timerAddress) {}
+        address _timerAddress
+    ) public initializer {
+        __Ownable_init();
+        __SpokePool_init(_initialDepositId, _hubPool, _hubPool, _wethAddress, _timerAddress);
+    }
 
     /**************************************
      *          INTERNAL FUNCTIONS           *
      **************************************/
 
     function _bridgeTokensToHubPool(RelayerRefundLeaf memory relayerRefundLeaf) internal override {
-        IERC20(relayerRefundLeaf.l2TokenAddress).safeTransfer(hubPool, relayerRefundLeaf.amountToReturn);
+        IERC20Upgradeable(relayerRefundLeaf.l2TokenAddress).safeTransfer(hubPool, relayerRefundLeaf.amountToReturn);
     }
 
     // The SpokePool deployed to the same network as the HubPool must be owned by the HubPool.

contracts/Optimism_SpokePool.sol

@@ -14,21 +14,21 @@ contract Optimism_SpokePool is Ovm_SpokePool {
      * relay hash collisions.
      * @param _crossDomainAdmin Cross domain admin to set. Can be changed by admin.
      * @param _hubPool Hub pool address to set. Can be changed by admin.
-     * @param timerAddress Timer address to set.
+     * @param _timerAddress Timer address to set.
      */
-    constructor(
+    function initialize(
         uint32 _initialDepositId,
         address _crossDomainAdmin,
         address _hubPool,
-        address timerAddress
-    )
-        Ovm_SpokePool(
+        address _timerAddress
+    ) public initializer {
+        __OvmSpokePool_init(
             _initialDepositId,
             _crossDomainAdmin,
             _hubPool,
             Lib_PredeployAddresses.OVM_ETH,
             0x4200000000000000000000000000000000000006,
-            timerAddress
-        )
-    {}
+            _timerAddress
+        );
+    }
 }

contracts/Ovm_SpokePool.sol

@@ -4,7 +4,7 @@ pragma solidity ^0.8.0;
 import "./SpokePool.sol";
 import "./interfaces/WETH9Interface.sol";
 
-import "@eth-optimism/contracts/libraries/bridge/CrossDomainEnabled.sol";
+import "@openzeppelin/contracts-upgradeable/crosschain/optimism/LibOptimismUpgradeable.sol";
 import "@eth-optimism/contracts/libraries/constants/Lib_PredeployAddresses.sol";
 import "@eth-optimism/contracts/L2/messaging/IL2ERC20Bridge.sol";
 
@@ -16,13 +16,21 @@ interface SynthetixBridgeToBase {
 /**
  * @notice OVM specific SpokePool. Uses OVM cross-domain-enabled logic to implement admin only access to functions. * Optimism and Boba each implement this spoke pool and set their chain specific contract addresses for l2Eth and l2Weth.
  */
-contract Ovm_SpokePool is CrossDomainEnabled, SpokePool {
+contract Ovm_SpokePool is SpokePool {
     // "l1Gas" parameter used in call to bridge tokens from this contract back to L1 via IL2ERC20Bridge. Currently
     // unused by bridge but included for future compatibility.
-    uint32 public l1Gas = 5_000_000;
+    uint32 public l1Gas;
 
     // ETH is an ERC20 on OVM.
-    address public immutable l2Eth;
+    address public l2Eth;
+
+    // Address of the Optimism L2 messenger.
+    address public messenger;
+
+    // Reserve storage slots for future versions of this base contract to add state variables without
+    // affecting the storage layout of child contracts. Decrement the size of __gap whenever state variables
+    // are added.
+    uint256[1000] private __gap;
 
     // Stores alternative token bridges to use for L2 tokens that don't go over the standard bridge. This is needed
     // to support non-standard ERC20 tokens on Optimism, such as DIA and SNX which both use custom bridges.
@@ -38,19 +46,20 @@ contract Ovm_SpokePool is CrossDomainEnabled, SpokePool {
      * relay hash collisions.
      * @param _crossDomainAdmin Cross domain admin to set. Can be changed by admin.
      * @param _hubPool Hub pool address to set. Can be changed by admin.
-     * @param timerAddress Timer address to set.
+     * @param _timerAddress Timer address to set.
      */
-    constructor(
+    function __OvmSpokePool_init(
         uint32 _initialDepositId,
         address _crossDomainAdmin,
         address _hubPool,
         address _l2Eth,
         address _wrappedNativeToken,
-        address timerAddress
-    )
-        CrossDomainEnabled(Lib_PredeployAddresses.L2_CROSS_DOMAIN_MESSENGER)
-        SpokePool(_initialDepositId, _crossDomainAdmin, _hubPool, _wrappedNativeToken, timerAddress)
-    {
+        address _timerAddress
+    ) public onlyInitializing {
+        l1Gas = 5_000_000;
+        __SpokePool_init(_initialDepositId, _crossDomainAdmin, _hubPool, _wrappedNativeToken, _timerAddress);
+        messenger = Lib_PredeployAddresses.L2_CROSS_DOMAIN_MESSENGER;
+        require(_l2Eth != address(0), "Invalid L2 ETH address");
         l2Eth = _l2Eth;
     }
 
@@ -139,6 +148,7 @@ contract Ovm_SpokePool is CrossDomainEnabled, SpokePool {
     // this logic inside a fallback method that executes when this contract receives ETH because ETH is an ERC20
     // on the OVM.
     function _depositEthToWeth() internal {
+        //slither-disable-next-line arbitrary-send-eth
         if (address(this).balance > 0) wrappedNativeToken.deposit{ value: address(this).balance }();
     }
 
@@ -172,5 +182,10 @@ contract Ovm_SpokePool is CrossDomainEnabled, SpokePool {
     }
 
     // Apply OVM-specific transformation to cross domain admin address on L1.
-    function _requireAdminSender() internal override onlyFromCrossDomainAccount(crossDomainAdmin) {}
+    function _requireAdminSender() internal view override {
+        require(
+            LibOptimismUpgradeable.crossChainSender(messenger) == crossDomainAdmin,
+            "OVM_XCHAIN: wrong sender of cross-domain message"
+        );
+    }
 }

contracts/PolygonTokenBridger.sol

@@ -1,11 +1,11 @@
 // SPDX-License-Identifier: AGPL-3.0-only
 pragma solidity ^0.8.0;
 
-import "./Lockable.sol";
 import "./interfaces/WETH9Interface.sol";
 
-import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
-import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
+import "@openzeppelin/contracts-upgradeable/token/ERC20/IERC20Upgradeable.sol";
+import "@openzeppelin/contracts-upgradeable/token/ERC20/utils/SafeERC20Upgradeable.sol";
+import "@openzeppelin/contracts/security/ReentrancyGuard.sol";
 
 // Polygon Registry contract that stores their addresses.
 interface PolygonRegistry {
@@ -18,7 +18,7 @@ interface PolygonERC20Predicate {
 }
 
 // ERC20s (on polygon) compatible with polygon's bridge have a withdraw method.
-interface PolygonIERC20 is IERC20 {
+interface PolygonIERC20Upgradeable is IERC20Upgradeable {
     function withdraw(uint256 amount) external;
 }
 
@@ -38,9 +38,9 @@ interface MaticToken {
  *  This ultimately allows create2 to generate deterministic addresses that don't depend on the transaction count of the
  * sender.
  */
-contract PolygonTokenBridger is Lockable {
-    using SafeERC20 for PolygonIERC20;
-    using SafeERC20 for IERC20;
+contract PolygonTokenBridger is ReentrancyGuard {
+    using SafeERC20Upgradeable for PolygonIERC20Upgradeable;
+    using SafeERC20Upgradeable for IERC20Upgradeable;
 
     // Gas token for Polygon.
     MaticToken public constant maticToken = MaticToken(0x0000000000000000000000000000000000001010);
@@ -90,6 +90,8 @@ contract PolygonTokenBridger is Lockable {
         uint256 _l1ChainId,
         uint256 _l2ChainId
     ) {
+        require(_destination != address(0), "invalid dest address");
+        require(_l2WrappedMatic != address(0), "invalid wmatic address");
         destination = _destination;
         l1PolygonRegistry = _l1PolygonRegistry;
         l1Weth = _l1Weth;
@@ -104,7 +106,7 @@ contract PolygonTokenBridger is Lockable {
      * @param token Token to bridge.
      * @param amount Amount to bridge.
      */
-    function send(PolygonIERC20 token, uint256 amount) public nonReentrant onlyChainId(l2ChainId) {
+    function send(PolygonIERC20Upgradeable token, uint256 amount) public nonReentrant onlyChainId(l2ChainId) {
         token.safeTransferFrom(msg.sender, address(this), amount);
 
         // In the wMatic case, this unwraps. For other ERC20s, this is the burn/send action.
@@ -119,9 +121,10 @@ contract PolygonTokenBridger is Lockable {
      * @notice Called by someone to send tokens to the destination, which should be set to the HubPool.
      * @param token Token to send to destination.
      */
-    function retrieve(IERC20 token) public nonReentrant onlyChainId(l1ChainId) {
+    function retrieve(IERC20Upgradeable token) public nonReentrant onlyChainId(l1ChainId) {
         if (address(token) == address(l1Weth)) {
             // For WETH, there is a pre-deposit step to ensure any ETH that has been sent to the contract is captured.
+            //slither-disable-next-line arbitrary-send-eth
             l1Weth.deposit{ value: address(this).balance }();
         }
         token.safeTransfer(destination, token.balanceOf(address(this)));