Add slither for l1 contracts

.github/workflows/slither.yaml

@@ -0,0 +1,46 @@
+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
+          rm -rf ./l1-contracts/contracts/dev-contracts/test/VerifierTest.sol
+          rm -rf ./l1-contracts/contracts/dev-contracts/test/VerifierRecursiveTest.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 unit 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

@@ -20,10 +20,11 @@
     using SafeERC20 for IERC20;
 
     /// @dev The shared bridge that is now used for all bridging, replacing the legacy contract.
     IL1SharedBridge public immutable override sharedBridge;
 
     /// @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 @@
         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

@@ -31,22 +31,22 @@
     using SafeERC20 for IERC20;
 
     /// @dev The address of the WETH token on L1.
     address public immutable override l1WethAddress;
 
     /// @dev Bridgehub smart contract that is used to operate with L2 via asynchronous L2 <-> L1 communication.
     IBridgehub public immutable override bridgehub;
 
     /// @dev Legacy bridge smart contract that used to hold ERC20 tokens.
     IL1ERC20Bridge public immutable override legacyBridge;
 
     /// @dev Era's chainID
     uint256 immutable eraChainId;
 
     /// @dev The address of legacy L1 ERC20 bridge.
     address immutable eraErc20BridgeAddress;
 
     /// @dev The address of zkSync Era diamond proxy contract.
     address immutable eraDiamondProxy;
 
     /// @dev Stores the first batch number on the zkSync Era Diamond Proxy that was settled after Shared Bridge upgrade.
     /// This variable is used to differentiate between pre-upgrade and post-upgrade withdrawals. Withdrawals from batches older
@@ -67,6 +67,7 @@
         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 @@
     /// @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,7 +605,8 @@
             // 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

@@ -303,6 +303,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

@@ -143,8 +143,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

@@ -34,7 +34,7 @@
     string public constant override getName = "MailboxFacet";
 
     /// @dev Era's chainID
     uint256 immutable eraChainId;
 
     constructor(uint256 _eraChainId) {
         eraChainId = _eraChainId;
@@ -167,9 +167,11 @@
             s.baseTokenGasPriceMultiplierDenominator;
         uint256 pubdataPriceBaseToken;
         if (feeParams.pubdataPricingMode == PubdataPricingMode.Rollup) {
+            // slither-disable-next-line divide-before-multiply
             pubdataPriceBaseToken = L1_GAS_PER_PUBDATA_BYTE * l1GasPriceConverted;
         }
 
+        // slither-disable-next-line divide-before-multiply
         uint256 batchOverheadBaseToken = uint256(feeParams.batchOverheadL1Gas) * l1GasPriceConverted;
         uint256 fullPubdataPriceBaseToken = pubdataPriceBaseToken +
             batchOverheadBaseToken /
@@ -252,6 +254,7 @@
         // 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 +266,18 @@
         // 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/contracts/state-transition/chain-deps/facets/ZkSyncStateTransitionBase.sol

@@ -9,6 +9,7 @@ import {ReentrancyGuard} from "../../../common/ReentrancyGuard.sol";
 /// @author Matter Labs
 /// @custom:security-contact [email protected]
 contract ZkSyncStateTransitionBase is ReentrancyGuard {
+    // slither-disable-next-line uninitialized-state
     ZkSyncStateTransitionStorage internal s;
 
     /// @notice Checks that the message sender is an active admin

l1-contracts/foundry.toml

@@ -1,6 +1,6 @@
 [profile.default]
 src = 'contracts'
-out = 'artifacts-forge'
+out = 'out'
 libs = ['node_modules', 'lib']
 cache_path = 'cache-forge'
 test = 'test/foundry'

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
+}