Add slither for l1 contracts

.github/workflows/slither.yaml

@@ -0,0 +1,43 @@
+name: Slither scanner
+
+on: pull_request
+
+jobs:
+  slither-l1:
+    name: Slither check for L1 contracts
+    runs-on: ubuntu-latest
+
+    steps:
+      - name: Checkout the repository
+        uses: actions/checkout@v4
+        with:
+          submodules: recursive
+
+      - name: Use Node.js
+        uses: actions/setup-node@v3
+        with:
+          node-version: 18.18.0
+          cache: yarn
+
+      - name: Install dependencies
+        run: yarn
+
+      - name: Setup Python
+        uses: actions/setup-python@v5
+        with:
+          python-version: 3.8
+
+      - name: Install Slither
+        run: |
+          pip install slither-analyzer
+
+      - name: Use Foundry
+        uses: foundry-rs/foundry-toolchain@v1
+
+      - name: Remove non-compiled files
+        run: rm -rf ./l1-contracts/contracts/state-transition/utils/ |
+          rm -rf ./l1-contracts/contracts/state-transition/Verifier.sol |
+          rm -rf ./l1-contracts/contracts/state-transition/TestnetVerifier.sol
+
+      - name: Run Slither
+        run: slither --config ./l1-contracts/slither.config.json ./l1-contracts

.gitignore

@@ -19,4 +19,5 @@ yarn-error.log*
 l1-contracts/yarn-error.log*
 l1-contracts/lcov.info
 l1-contracts/report/*
-l1-contracts/coverage/*
+l1-contracts/coverage/*
+l1-contracts/out/*

l1-contracts/README.md

@@ -0,0 +1,43 @@
+# zkSync Era: L1 Contracts
+
+[![Logo](../eraLogo.svg)](https://zksync.io/)
+
+zkSync Era is a layer 2 rollup that uses zero-knowledge proofs to scale Ethereum without compromising on security or
+decentralization. Since it's EVM compatible (Solidity/Vyper), 99% of Ethereum projects can redeploy without refactoring
+or re-auditing a single line of code. zkSync Era also uses an LLVM-based compiler that will eventually let developers
+write smart contracts in C++, Rust and other popular languages.
+
+## L1 Contracts
+
+### Building
+
+Compile the solidity and yul contracts: `yarn l1 build`
+
+### Testing
+
+To run uint tests, execute `yarn l1 test`.
+
+Similarly, to run tests in Foundry execute `yarn l1 test:foundry`.
+
+To run the fork test, use `yarn l1 test:fork`
+
+### Security Testing and Linting
+
+Our CI/CD pipelines are equipped with multiple security tests and linting tools.
+For security checks, we employ `slither`, while `solhint` is used for code linting.
+It's important to note that both tools might sometimes flag issues that are not actually problematic,
+known as false positives. In cases where you're confident an issue flagged by `slither` or `solhint` is a false positive,
+you have the option to mark it as such.
+
+This can be done by using specific directives provided by each tool.
+
+For `slither`, you can find more information on marking false positives in their [triage mode documentation](https://github.com/crytic/slither/wiki/Usage#triage-mode).
+
+Similarly, for `solhint`, guidance on configuring the linter to ignore specific issues can be found in their [README](https://github.com/protofire/solhint?tab=readme-ov-file#configure-the-linter-with-comments).
+
+If you identify a false positive in your code, please make sure to highlight this to your colleagues during the code review process.
+
+### Typos
+
+We also utilize `typos` and `codespell` spell checkers to minimize the occurrence of accidental typos.
+If you need to add a word to the databases of these tools please insert it into `../codespell/wordlist.txt` and `../_typos.toml`.

l1-contracts/contracts/bridge/L1ERC20Bridge.sol

@@ -24,6 +24,7 @@ contract L1ERC20Bridge is IL1ERC20Bridge, ReentrancyGuard {
 
     /// @dev A mapping L2 batch number => message number => flag.
     /// @dev Used to indicate that L2 -> L1 message was already processed for zkSync Era withdrawals.
+    // slither-disable-next-line uninitialized-state
     mapping(uint256 l2BatchNumber => mapping(uint256 l2ToL1MessageNumber => bool isFinalized))
         public isWithdrawalFinalized;
 
@@ -33,12 +34,15 @@ contract L1ERC20Bridge is IL1ERC20Bridge, ReentrancyGuard {
         public depositAmount;
 
     /// @dev The address that is used as a L2 bridge counterpart in zkSync Era.
+    // slither-disable-next-line uninitialized-state
     address public l2Bridge;
 
     /// @dev The address that is used as a beacon for L2 tokens in zkSync Era.
+    // slither-disable-next-line uninitialized-state
     address public l2TokenBeacon;
 
     /// @dev Stores the hash of the L2 token proxy contract's bytecode on zkSync Era.
+    // slither-disable-next-line uninitialized-state
     bytes32 public l2TokenProxyBytecodeHash;
 
     /// @dev Deprecated storage variable related to withdrawal limitations.

l1-contracts/contracts/bridge/L1SharedBridge.sol

@@ -67,6 +67,7 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Initializable, Owna
         public isWithdrawalFinalized;
 
     /// @dev Indicates whether the hyperbridging is enabled for a given chain.
+    // slither-disable-next-line uninitialized-state
     mapping(uint256 chainId => bool enabled) internal hyperbridgingEnabled;
 
     /// @dev Maps token balances for each chain to prevent unauthorized spending across hyperchains.
@@ -187,6 +188,7 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Initializable, Owna
     /// @return The difference between the contract balance before and after the transferring of funds.
     function _depositFunds(address _from, IERC20 _token, uint256 _amount) internal returns (uint256) {
         uint256 balanceBefore = _token.balanceOf(address(this));
+        // slither-disable-next-line arbitrary-send-erc20
         _token.safeTransferFrom(_from, address(this), _amount);
         uint256 balanceAfter = _token.balanceOf(address(this));
 
@@ -603,6 +605,7 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Initializable, Owna
             // If the recipient is a contract on L1, the address alias will be applied.
             address refundRecipient = _refundRecipient;
             if (_refundRecipient == address(0)) {
+                // slither-disable-next-line tx-origin
                 refundRecipient = _prevMsgSender != tx.origin
                     ? AddressAliasHelper.applyL1ToL2Alias(_prevMsgSender)
                     : _prevMsgSender;

l1-contracts/contracts/governance/Governance.sol

@@ -164,13 +164,15 @@ contract Governance is IGovernance, Ownable2Step {
     /// @notice Executes the scheduled operation after the delay passed.
     /// @dev Both the owner and security council may execute delayed operations.
     /// @param _operation The operation parameters will be executed with the upgrade.
+    //  slither-disable-next-line reentrancy-eth
     function execute(Operation calldata _operation) external payable onlyOwnerOrSecurityCouncil {
         bytes32 id = hashOperation(_operation);
         // Check if the predecessor operation is completed.
         _checkPredecessorDone(_operation.predecessor);
         // Ensure that the operation is ready to proceed.
         require(isOperationReady(id), "Operation must be ready before execution");
         // Execute operation.
+        // slither-disable-next-line reentrancy-eth
         _execute(_operation.calls);
         // Reconfirming that the operation is still ready after execution.
         // This is needed to avoid unexpected reentrancy attacks of re-executing the same operation.
@@ -183,13 +185,15 @@ contract Governance is IGovernance, Ownable2Step {
     /// @notice Executes the scheduled operation with the security council instantly.
     /// @dev Only the security council may execute an operation instantly.
     /// @param _operation The operation parameters will be executed with the upgrade.
+    //  slither-disable-next-line reentrancy-eth
     function executeInstant(Operation calldata _operation) external payable onlySecurityCouncil {
         bytes32 id = hashOperation(_operation);
         // Check if the predecessor operation is completed.
         _checkPredecessorDone(_operation.predecessor);
         // Ensure that the operation is in a pending state before proceeding.
         require(isOperationPending(id), "Operation must be pending before execution");
         // Execute operation.
+        // slither-disable-next-line reentrancy-eth
         _execute(_operation.calls);
         // Reconfirming that the operation is still pending before execution.
         // This is needed to avoid unexpected reentrancy attacks of re-executing the same operation.
@@ -223,6 +227,7 @@ contract Governance is IGovernance, Ownable2Step {
     /// @param _calls The array of calls to be executed.
     function _execute(Call[] calldata _calls) internal {
         for (uint256 i = 0; i < _calls.length; ++i) {
+            // slither-disable-next-line arbitrary-send-eth
             (bool success, bytes memory returnData) = _calls[i].target.call{value: _calls[i].value}(_calls[i].data);
             if (!success) {
                 // Propagate an error if the call fails.

l1-contracts/contracts/state-transition/StateTransitionManager.sol

@@ -276,6 +276,7 @@ contract StateTransitionManager is IStateTransitionManager, ReentrancyGuard, Own
 
         diamondCut.initCalldata = initData;
         // deploy stateTransitionContract
+        // slither-disable-next-line reentrancy-no-eth
         DiamondProxy stateTransitionContract = new DiamondProxy{salt: bytes32(0)}(block.chainid, diamondCut);
 
         // save data

l1-contracts/contracts/state-transition/chain-deps/facets/Executor.sol

@@ -141,8 +141,11 @@ contract ExecutorFacet is ZkSyncStateTransitionBase, IExecutor {
         // linear traversal of the logs
         for (uint256 i = 0; i < emittedL2Logs.length; i = i.uncheckedAdd(L2_TO_L1_LOG_SERIALIZE_SIZE)) {
             // Extract the values to be compared to/used such as the log sender, key, and value
+            // slither-disable-next-line unused-return
             (address logSender, ) = UnsafeBytes.readAddress(emittedL2Logs, i + L2_LOG_ADDRESS_OFFSET);
+            // slither-disable-next-line unused-return
             (uint256 logKey, ) = UnsafeBytes.readUint256(emittedL2Logs, i + L2_LOG_KEY_OFFSET);
+            // slither-disable-next-line unused-return
             (bytes32 logValue, ) = UnsafeBytes.readBytes32(emittedL2Logs, i + L2_LOG_VALUE_OFFSET);
 
             // Ensure that the log hasn't been processed already

l1-contracts/contracts/state-transition/chain-deps/facets/Mailbox.sol

@@ -252,6 +252,7 @@ contract MailboxFacet is ZkSyncStateTransitionBase, IMailbox {
         // Change the sender address if it is a smart contract to prevent address collision between L1 and L2.
         // Please note, currently zkSync address derivation is different from Ethereum one, but it may be changed in the future.
         address l2Sender = _request.sender;
+        // slither-disable-next-line tx-origin
         if (l2Sender != tx.origin) {
             l2Sender = AddressAliasHelper.applyL1ToL2Alias(_request.sender);
         }
@@ -263,15 +264,18 @@ contract MailboxFacet is ZkSyncStateTransitionBase, IMailbox {
         // CHANGING THIS CONSTANT SHOULD BE A CLIENT-SIDE CHANGE.
         require(_request.l2GasPerPubdataByteLimit == REQUIRED_L2_GAS_PRICE_PER_PUBDATA, "qp");
 
-        // Here we manually assign fields for the struct to prevent "stack too deep" error
-        WritePriorityOpParams memory params;
-
-        params.sender = l2Sender;
-        params.l2Value = _request.l2Value;
-        params.contractAddressL2 = _request.contractL2;
-        params.l2GasLimit = _request.l2GasLimit;
-        params.l2GasPricePerPubdata = _request.l2GasPerPubdataByteLimit;
-        params.refundRecipient = _request.refundRecipient;
+        WritePriorityOpParams memory params = WritePriorityOpParams({
+            sender: l2Sender,
+            txId: 0,
+            l2Value: _request.l2Value,
+            contractAddressL2: _request.contractL2,
+            expirationTimestamp: 0,
+            l2GasLimit: _request.l2GasLimit,
+            l2GasPrice: 0,
+            l2GasPricePerPubdata: _request.l2GasPerPubdataByteLimit,
+            valueToMint: 0,
+            refundRecipient: _request.refundRecipient
+        });
 
         canonicalTxHash = _requestL2Transaction(_request.mintValue, params, _request.l2Calldata, _request.factoryDeps);
     }

l1-contracts/foundry.toml

@@ -1,8 +1,8 @@
 [profile.default]
 src = 'contracts'
-out = 'artifacts-forge'
+out = "out"
 libs = ['node_modules', 'lib']
 cache_path = 'cache-forge'
-test = 'test/foundry'
+test = "test"
 
 # See more config options https://github.com/foundry-rs/foundry/tree/master/crates/config

l1-contracts/slither.config.json

@@ -0,0 +1,11 @@
+{
+  "filter_paths": "(contracts/dev-contracts|lib|node_modules)",
+  "detectors_to_exclude": "assembly,solc-version,low-level-calls,conformance-to-solidity-naming-conventions,incorrect-equality,uninitialized-local",
+  "exclude_dependencies": true,
+  "compile_force_framework": "foundry",
+  "exclude_medium": false,
+  "exclude_high": false,
+  "exclude_low": true,
+  "exclude_informational": true,
+  "exclude_optimization": true
+}