Merge pull request #1784 from coolhill/develop

Slither github action fix and ignore database
ethereum-optimism · Dec 1, 2021 · 639e5b1 · 639e5b1
2 parents 4e30cd9 + 8f91dde
commit 639e5b1
Show file tree

Hide file tree

Showing 24 changed files with 122 additions and 5 deletions.
diff --git a/.github/workflows/static-analysis.yml b/.github/workflows/static-analysis.yml
@@ -58,4 +58,4 @@ jobs:
         working-directory: ./packages/contracts
         shell: bash
         run: yarn test:slither
-        continue-on-error: true
+        continue-on-error: false
diff --git a/package.json b/package.json
@@ -57,6 +57,8 @@
   },
   "dependencies": {
     "@changesets/cli": "^2.16.0",
+    "@openzeppelin/contracts": "^4.3.2",
+    "@openzeppelin/contracts-upgradeable": "^4.3.2",
     "@codechecks/client": "^0.1.11"
   }
 }
diff --git a/packages/contracts/contracts/L1/deployment/AddressDictator.sol b/packages/contracts/contracts/L1/deployment/AddressDictator.sol
@@ -62,6 +62,7 @@ contract AddressDictator {
      * Called to finalize the transfer, this function is callable by anyone, but will only result in
      * an upgrade if this contract is the owner Address Manager.
      */
+    // slither-disable-next-line calls-loop
     function setAddresses() external {
         for (uint256 i = 0; i < namedAddresses.length; i++) {
             manager.setAddress(namedAddresses[i].name, namedAddresses[i].addr);

diff --git a/packages/contracts/contracts/L1/deployment/ChugSplashDictator.sol b/packages/contracts/contracts/L1/deployment/ChugSplashDictator.sol
@@ -14,6 +14,7 @@ contract ChugSplashDictator is iL1ChugSplashDeployer {
      * Variables *
      *************/
 
+    // slither-disable-next-line constable-states
     bool public isUpgrading = true;
     L1ChugSplashProxy public target;
     address public finalOwner;

diff --git a/packages/contracts/contracts/L1/messaging/L1CrossDomainMessenger.sol b/packages/contracts/contracts/L1/messaging/L1CrossDomainMessenger.sol
@@ -77,6 +77,7 @@ contract L1CrossDomainMessenger is
     /**
      * @param _libAddressManager Address of the Address Manager.
      */
+    // slither-disable-next-line external-function
     function initialize(address _libAddressManager) public initializer {
         require(
             address(libAddressManager) == address(0),
@@ -117,6 +118,7 @@ contract L1CrossDomainMessenger is
         emit MessageAllowed(_xDomainCalldataHash);
     }
 
+    // slither-disable-next-line external-function
     function xDomainMessageSender() public view returns (address) {
         require(
             xDomainMsgSender != Lib_DefaultValues.DEFAULT_XDOMAIN_SENDER,
@@ -131,6 +133,7 @@ contract L1CrossDomainMessenger is
      * @param _message Message to send to the target.
      * @param _gasLimit Gas limit for the provided message.
      */
+    // slither-disable-next-line external-function
     function sendMessage(
         address _target,
         bytes memory _message,
@@ -147,15 +150,18 @@ contract L1CrossDomainMessenger is
             nonce
         );
 
+        // slither-disable-next-line reentrancy-events
         _sendXDomainMessage(ovmCanonicalTransactionChain, xDomainCalldata, _gasLimit);
 
+        // slither-disable-next-line reentrancy-events
         emit SentMessage(_target, msg.sender, _message, nonce, _gasLimit);
     }
 
     /**
      * Relays a cross domain message to a contract.
      * @inheritdoc IL1CrossDomainMessenger
      */
+    // slither-disable-next-line external-function
     function relayMessage(
         address _target,
         address _sender,
@@ -193,28 +199,35 @@ contract L1CrossDomainMessenger is
         );
 
         xDomainMsgSender = _sender;
+        // slither-disable-next-line reentrancy-no-eth, reentrancy-events, reentrancy-benign
         (bool success, ) = _target.call(_message);
+        // slither-disable-next-line reentrancy-benign
         xDomainMsgSender = Lib_DefaultValues.DEFAULT_XDOMAIN_SENDER;
 
         // Mark the message as received if the call was successful. Ensures that a message can be
         // relayed multiple times in the case that the call reverted.
         if (success == true) {
+            // slither-disable-next-line reentrancy-no-eth
             successfulMessages[xDomainCalldataHash] = true;
+            // slither-disable-next-line reentrancy-events
             emit RelayedMessage(xDomainCalldataHash);
         } else {
+            // slither-disable-next-line reentrancy-events
             emit FailedRelayedMessage(xDomainCalldataHash);
         }
 
         // Store an identifier that can be used to prove that the given message was relayed by some
         // user. Gives us an easy way to pay relayers for their work.
         bytes32 relayId = keccak256(abi.encodePacked(xDomainCalldata, msg.sender, block.number));
+        // slither-disable-next-line reentrancy-benign
         relayedMessages[relayId] = true;
     }
 
     /**
      * Replays a cross domain message to the target messenger.
      * @inheritdoc IL1CrossDomainMessenger
      */
+    // slither-disable-next-line external-function
     function replayMessage(
         address _target,
         address _sender,
@@ -354,6 +367,7 @@ contract L1CrossDomainMessenger is
         bytes memory _message,
         uint256 _gasLimit
     ) internal {
+        // slither-disable-next-line reentrancy-events
         ICanonicalTransactionChain(_canonicalTransactionChain).enqueue(
             Lib_PredeployAddresses.L2_CROSS_DOMAIN_MESSENGER,
             _gasLimit,

diff --git a/packages/contracts/contracts/L1/messaging/L1StandardBridge.sol b/packages/contracts/contracts/L1/messaging/L1StandardBridge.sol
@@ -47,6 +47,7 @@ contract L1StandardBridge is IL1StandardBridge, CrossDomainEnabled {
      * @param _l1messenger L1 Messenger address being used for cross-chain communications.
      * @param _l2TokenBridge L2 standard bridge address.
      */
+    // slither-disable-next-line external-function
     function initialize(address _l1messenger, address _l2TokenBridge) public {
         require(messenger == address(0), "Contract has already been initialized.");
         messenger = _l1messenger;
@@ -122,8 +123,10 @@ contract L1StandardBridge is IL1StandardBridge, CrossDomainEnabled {
         );
 
         // Send calldata into L2
+        // slither-disable-next-line reentrancy-events
         sendCrossDomainMessage(l2TokenBridge, _l2Gas, message);
 
+        // slither-disable-next-line reentrancy-events
         emit ETHDepositInitiated(_from, _to, msg.value, _data);
     }
 
@@ -180,6 +183,7 @@ contract L1StandardBridge is IL1StandardBridge, CrossDomainEnabled {
         // When a deposit is initiated on L1, the L1 Bridge transfers the funds to itself for future
         // withdrawals. safeTransferFrom also checks if the contract has code, so this will fail if
         // _from is an EOA or address(0).
+        // slither-disable-next-line reentrancy-events, reentrancy-benign
         IERC20(_l1Token).safeTransferFrom(_from, address(this), _amount);
 
         // Construct calldata for _l2Token.finalizeDeposit(_to, _amount)
@@ -194,10 +198,13 @@ contract L1StandardBridge is IL1StandardBridge, CrossDomainEnabled {
         );
 
         // Send calldata into L2
+        // slither-disable-next-line reentrancy-events, reentrancy-benign
         sendCrossDomainMessage(l2TokenBridge, _l2Gas, message);
 
+        // slither-disable-next-line reentrancy-benign
         deposits[_l1Token][_l2Token] = deposits[_l1Token][_l2Token] + _amount;
 
+        // slither-disable-next-line reentrancy-events
         emit ERC20DepositInitiated(_l1Token, _l2Token, _from, _to, _amount, _data);
     }
 
@@ -214,9 +221,11 @@ contract L1StandardBridge is IL1StandardBridge, CrossDomainEnabled {
         uint256 _amount,
         bytes calldata _data
     ) external onlyFromCrossDomainAccount(l2TokenBridge) {
+        // slither-disable-next-line reentrancy-events
         (bool success, ) = _to.call{ value: _amount }(new bytes(0));
         require(success, "TransferHelper::safeTransferETH: ETH transfer failed");
 
+        // slither-disable-next-line reentrancy-events
         emit ETHWithdrawalFinalized(_from, _to, _amount, _data);
     }
 
@@ -234,8 +243,10 @@ contract L1StandardBridge is IL1StandardBridge, CrossDomainEnabled {
         deposits[_l1Token][_l2Token] = deposits[_l1Token][_l2Token] - _amount;
 
         // When a withdrawal is finalized on L1, the L1 Bridge transfers the funds to the withdrawer
+        // slither-disable-next-line reentrancy-events
         IERC20(_l1Token).safeTransfer(_to, _amount);
 
+        // slither-disable-next-line reentrancy-events
         emit ERC20WithdrawalFinalized(_l1Token, _l2Token, _from, _to, _amount, _data);
     }
 

diff --git a/packages/contracts/contracts/L1/rollup/CanonicalTransactionChain.sol b/packages/contracts/contracts/L1/rollup/CanonicalTransactionChain.sol
@@ -39,9 +39,12 @@ contract CanonicalTransactionChain is ICanonicalTransactionChain, Lib_AddressRes
 
     // Encoding-related (all in bytes)
     uint256 internal constant BATCH_CONTEXT_SIZE = 16;
+    // slither-disable-next-line unused-state
     uint256 internal constant BATCH_CONTEXT_LENGTH_POS = 12;
     uint256 internal constant BATCH_CONTEXT_START_POS = 15;
+    // slither-disable-next-line unused-state
     uint256 internal constant TX_DATA_HEADER_SIZE = 3;
+    // slither-disable-next-line unused-state
     uint256 internal constant BYTES_TILL_TX_DATA = 65;
 
     /*************
@@ -131,6 +134,7 @@ contract CanonicalTransactionChain is ICanonicalTransactionChain, Lib_AddressRes
      * Retrieves the total number of batches submitted.
      * @return _totalBatches Total submitted batches.
      */
+    // slither-disable-next-line external-function
     function getTotalBatches() public view returns (uint256 _totalBatches) {
         return batches().length();
     }
@@ -139,6 +143,7 @@ contract CanonicalTransactionChain is ICanonicalTransactionChain, Lib_AddressRes
      * Returns the index of the next element to be enqueued.
      * @return Index for the next queue element.
      */
+    // slither-disable-next-line external-function
     function getNextQueueIndex() public view returns (uint40) {
         return _nextQueueIndex;
     }
@@ -147,6 +152,7 @@ contract CanonicalTransactionChain is ICanonicalTransactionChain, Lib_AddressRes
      * Returns the timestamp of the last transaction.
      * @return Timestamp for the last transaction.
      */
+    // slither-disable-next-line external-function
     function getLastTimestamp() public view returns (uint40) {
         (, , uint40 lastTimestamp, ) = _getBatchExtraData();
         return lastTimestamp;
@@ -156,6 +162,7 @@ contract CanonicalTransactionChain is ICanonicalTransactionChain, Lib_AddressRes
      * Returns the blocknumber of the last transaction.
      * @return Blocknumber for the last transaction.
      */
+    // slither-disable-next-line external-function
     function getLastBlockNumber() public view returns (uint40) {
         (, , , uint40 lastBlockNumber) = _getBatchExtraData();
         return lastBlockNumber;
@@ -166,6 +173,7 @@ contract CanonicalTransactionChain is ICanonicalTransactionChain, Lib_AddressRes
      * @param _index Index of the queue element to access.
      * @return _element Queue element at the given index.
      */
+    // slither-disable-next-line external-function
     function getQueueElement(uint256 _index)
         public
         view
@@ -178,6 +186,7 @@ contract CanonicalTransactionChain is ICanonicalTransactionChain, Lib_AddressRes
      * Get the number of queue elements which have not yet been included.
      * @return Number of pending queue elements.
      */
+    // slither-disable-next-line external-function
     function getNumPendingQueueElements() public view returns (uint40) {
         return uint40(queueElements.length) - _nextQueueIndex;
     }
@@ -187,6 +196,7 @@ contract CanonicalTransactionChain is ICanonicalTransactionChain, Lib_AddressRes
      * both pending and canonical transactions.
      * @return Length of the queue.
      */
+    // slither-disable-next-line external-function
     function getQueueLength() public view returns (uint40) {
         return uint40(queueElements.length);
     }
@@ -348,6 +358,7 @@ contract CanonicalTransactionChain is ICanonicalTransactionChain, Lib_AddressRes
         }
 
         // Cache the previous blockhash to ensure all transaction data can be retrieved efficiently.
+        // slither-disable-next-line reentrancy-no-eth, reentrancy-events
         _appendBatch(
             blockhash(block.number - 1),
             totalElementsToAppend,
@@ -356,13 +367,15 @@ contract CanonicalTransactionChain is ICanonicalTransactionChain, Lib_AddressRes
             blockNumber
         );
 
+        // slither-disable-next-line reentrancy-events
         emit SequencerBatchAppended(
             nextQueueIndex - numQueuedTransactions,
             numQueuedTransactions,
             getTotalElements()
         );
 
         // Update the _nextQueueIndex storage variable.
+        // slither-disable-next-line reentrancy-no-eth
         _nextQueueIndex = nextQueueIndex;
     }
 
@@ -377,6 +390,7 @@ contract CanonicalTransactionChain is ICanonicalTransactionChain, Lib_AddressRes
      */
     function _getBatchContext(uint256 _index) internal pure returns (BatchContext memory) {
         uint256 contextPtr = 15 + _index * BATCH_CONTEXT_SIZE;
+        // slither-disable-next-line similar-names
         uint256 numSequencedTransactions;
         uint256 numSubsequentQueueTransactions;
         uint256 ctxTimestamp;
@@ -513,6 +527,7 @@ contract CanonicalTransactionChain is ICanonicalTransactionChain, Lib_AddressRes
             _blockNumber
         );
 
+        // slither-disable-next-line reentrancy-no-eth, reentrancy-events
         batchesRef.push(batchHeaderHash, latestBatchContext);
     }
 }
diff --git a/packages/contracts/contracts/L1/rollup/ChainStorageContainer.sol b/packages/contracts/contracts/L1/rollup/ChainStorageContainer.sol
@@ -67,55 +67,63 @@ contract ChainStorageContainer is IChainStorageContainer, Lib_AddressResolver {
     /**
      * @inheritdoc IChainStorageContainer
      */
+    // slither-disable-next-line external-function
     function setGlobalMetadata(bytes27 _globalMetadata) public onlyOwner {
         return buffer.setExtraData(_globalMetadata);
     }
 
     /**
      * @inheritdoc IChainStorageContainer
      */
+    // slither-disable-next-line external-function
     function getGlobalMetadata() public view returns (bytes27) {
         return buffer.getExtraData();
     }
 
     /**
      * @inheritdoc IChainStorageContainer
      */
+    // slither-disable-next-line external-function
     function length() public view returns (uint256) {
         return uint256(buffer.getLength());
     }
 
     /**
      * @inheritdoc IChainStorageContainer
      */
+    // slither-disable-next-line external-function
     function push(bytes32 _object) public onlyOwner {
         buffer.push(_object);
     }
 
     /**
      * @inheritdoc IChainStorageContainer
      */
+    // slither-disable-next-line external-function
     function push(bytes32 _object, bytes27 _globalMetadata) public onlyOwner {
         buffer.push(_object, _globalMetadata);
     }
 
     /**
      * @inheritdoc IChainStorageContainer
      */
+    // slither-disable-next-line external-function
     function get(uint256 _index) public view returns (bytes32) {
         return buffer.get(uint40(_index));
     }
 
     /**
      * @inheritdoc IChainStorageContainer
      */
+    // slither-disable-next-line external-function
     function deleteElementsAfterInclusive(uint256 _index) public onlyOwner {
         buffer.deleteElementsAfterInclusive(uint40(_index));
     }
 
     /**
      * @inheritdoc IChainStorageContainer
      */
+    // slither-disable-next-line external-function
     function deleteElementsAfterInclusive(uint256 _index, bytes27 _globalMetadata)
         public
         onlyOwner