improve: Make changes after running Slither on SpokePool contracts

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

@@ -83,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/Ovm_SpokePool.sol

@@ -59,6 +59,7 @@ contract Ovm_SpokePool is SpokePool {
         l1Gas = 5_000_000;
         __SpokePool_init(_initialDepositId, _crossDomainAdmin, _hubPool, _wrappedNativeToken, _timerAddress);
         messenger = Lib_PredeployAddresses.L2_CROSS_DOMAIN_MESSENGER;
+        //slither-disable-next-line missing-zero-check
         l2Eth = _l2Eth;
     }
 
@@ -147,6 +148,7 @@ contract Ovm_SpokePool is 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 }();
     }
 

contracts/PolygonTokenBridger.sol

@@ -90,9 +90,11 @@ contract PolygonTokenBridger is Lockable {
         uint256 _l1ChainId,
         uint256 _l2ChainId
     ) {
+        //slither-disable-next-line missing-zero-check
         destination = _destination;
         l1PolygonRegistry = _l1PolygonRegistry;
         l1Weth = _l1Weth;
+        //slither-disable-next-line missing-zero-check
         l2WrappedMatic = _l2WrappedMatic;
         l1ChainId = _l1ChainId;
         l2ChainId = _l2ChainId;
@@ -122,6 +124,7 @@ contract PolygonTokenBridger is Lockable {
     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)));

contracts/Polygon_SpokePool.sol

@@ -80,6 +80,7 @@ contract Polygon_SpokePool is IFxMessageProcessor, SpokePool {
         callValidated = false;
         __SpokePool_init(_initialDepositId, _crossDomainAdmin, _hubPool, _wmaticAddress, _timerAddress);
         polygonTokenBridger = _polygonTokenBridger;
+        //slither-disable-next-line missing-zero-check
         fxChild = _fxChild;
     }
 
@@ -92,6 +93,7 @@ contract Polygon_SpokePool is IFxMessageProcessor, SpokePool {
      * @param newFxChild New FxChild.
      */
     function setFxChild(address newFxChild) public onlyAdmin nonReentrant {
+        //slither-disable-next-line missing-zero-check
         fxChild = newFxChild;
         emit SetFxChild(newFxChild);
     }
@@ -127,8 +129,10 @@ contract Polygon_SpokePool is IFxMessageProcessor, SpokePool {
         // This uses delegatecall to take the information in the message and process it as a function call on this contract.
         /// This is a safe delegatecall because its made to address(this) so there is no risk of delegating to a
         /// selfdestruct().
+        //slither-disable-start low-level-calls
         /// @custom:oz-upgrades-unsafe-allow delegatecall
         (bool success, ) = address(this).delegatecall(data);
+        //slither-disable-end low-level-calls
         require(success, "delegatecall failed");
     }
 
@@ -227,6 +231,7 @@ contract Polygon_SpokePool is IFxMessageProcessor, SpokePool {
 
     function _wrap() internal {
         uint256 balance = address(this).balance;
+        //slither-disable-next-line arbitrary-send-eth
         if (balance > 0) wrappedNativeToken.deposit{ value: balance }();
     }
 

contracts/SpokePool.sol

@@ -290,6 +290,11 @@ abstract contract SpokePool is
      * to ensure that a small input range doesn't limit which indices this method is able to reach.
      */
     function emergencyDeleteRootBundle(uint256 rootBundleId) public override onlyAdmin nonReentrant {
+        // Deleting a struct containing a mapping does not delete the mapping in Solidity, therefore the bitmap's
+        // data will still remain potentially leading to vulnerabilities down the line. The way around this would
+        // be to iterate through every key in the mapping and resetting the value to 0, but this seems expensive and
+        // would require a new list in storage to keep track of keys.
+        //slither-disable-next-line mapping-deletion
         delete rootBundles[rootBundleId];
         emit EmergencyDeleteRootBundle(rootBundleId);
     }
@@ -332,11 +337,17 @@ abstract contract SpokePool is
         // buffer of this contract's block time to allow for this variance.
         // Note also that quoteTimestamp cannot be less than the buffer otherwise the following arithmetic can result
         // in underflow. This isn't a problem as the deposit will revert, but the error might be unexpected for clients.
+
+        //slither-disable-next-line timestamp
         require(
             getCurrentTime() >= quoteTimestamp - depositQuoteTimeBuffer &&
                 getCurrentTime() <= quoteTimestamp + depositQuoteTimeBuffer,
             "invalid quote time"
         );
+
+        // Increment count of deposits so that deposit ID for this spoke pool is unique.
+        uint32 newDepositId = numberOfDeposits++;
+
         // If the address of the origin token is a wrappedNativeToken contract and there is a msg.value with the
         // transaction then the user is sending ETH. In this case, the ETH should be deposited to wrappedNativeToken.
         if (originToken == address(wrappedNativeToken) && msg.value > 0) {
@@ -352,18 +363,12 @@ abstract contract SpokePool is
             chainId(),
             destinationChainId,
             relayerFeePct,
-            numberOfDeposits,
+            newDepositId,
             quoteTimestamp,
             originToken,
             recipient,
             msg.sender
         );
-
-        // Increment count of deposits so that deposit ID for this spoke pool is unique.
-        // @dev: Use pre-increment to save gas:
-        // i++ --> Load, Store, Add, Store
-        // ++i --> Load, Add, Store
-        ++numberOfDeposits;
     }
 
     /**
@@ -798,6 +803,7 @@ abstract contract SpokePool is
             IERC20Upgradeable(address(wrappedNativeToken)).safeTransfer(to, amount);
         } else {
             wrappedNativeToken.withdraw(amount);
+            //slither-disable-next-line arbitrary-send-eth
             to.transfer(amount);
         }
     }

contracts/interfaces/WETH9Interface.sol

@@ -8,5 +8,5 @@ interface WETH9Interface {
 
     function balanceOf(address guy) external view returns (uint256 wad);
 
-    function transfer(address guy, uint256 wad) external;
+    function transfer(address guy, uint256 wad) external returns (bool);
 }

contracts/upgradeable/MultiCallerUpgradeable.sol

@@ -18,18 +18,24 @@ contract MultiCallerUpgradeable {
 
     function multicall(bytes[] calldata data) external returns (bytes[] memory results) {
         results = new bytes[](data.length);
+
+        //slither-disable-start calls-loop
         for (uint256 i = 0; i < data.length; i++) {
             // Typically, implementation contracts used in the upgradeable proxy pattern shouldn't call `delegatecall`
             // because it could allow a malicious actor to call this implementation contract directly (rather than
             // through a proxy contract) and then selfdestruct() the contract, thereby freezing the upgradeable
             // proxy. However, since we're only delegatecall-ing into this contract, then we can consider this
             // use of delegatecall() safe.
+
+            //slither-disable-start low-level-calls
             /// @custom:oz-upgrades-unsafe-allow delegatecall
             (bool success, bytes memory result) = address(this).delegatecall(data[i]);
+            //slither-disable-end low-level-calls
 
             if (!success) {
                 // Next 5 lines from https://ethereum.stackexchange.com/a/83577
                 if (result.length < 68) revert();
+                //slither-disable-next-line assembly
                 assembly {
                     result := add(result, 0x04)
                 }
@@ -38,5 +44,6 @@ contract MultiCallerUpgradeable {
 
             results[i] = result;
         }
+        //slither-disable-end calls-loop
     }
 }

contracts/upgradeable/TestableUpgradeable.sol

@@ -26,6 +26,7 @@ abstract contract TestableUpgradeable is Initializable {
      * Must be set to 0x0 for production environments that use live time.
      */
     function __Testable_init(address _timerAddress) public onlyInitializing {
+        //slither-disable-next-line missing-zero-check
         timerAddress = _timerAddress;
     }