ci: add Slither static-analysis (#103)

mssassii · Thegaram · zimpha · commit eae626d234ca · 2025-06-12T10:14:17.000+08:00
Co-authored-by: Péter Garamvölgyi &lt;peter@scroll.io&gt;
diff --git a/.github/workflows/contracts.yml b/.github/workflows/contracts.yml
@@ -138,4 +138,59 @@ jobs:
         run: npx hardhat compile
 
       - name: Run hardhat tests
-        run: npx hardhat test
+        run: npx hardhat test
+
+  slither:
+    if: github.event.pull_request.draft == false
+    needs: foundry
+    runs-on: ubuntu-latest
+    permissions:
+      statuses: write
+      security-events: write
+
+    steps:
+      - uses: actions/checkout@v4
+        with:
+          submodules: recursive
+          persist-credentials: false
+
+      - uses: actions/setup-node@v4
+        with:
+          node-version: '18'
+
+      - run: yarn install --frozen-lockfile
+
+      - uses: foundry-rs/foundry-toolchain@82dee4ba654bd2146511f85f0d013af94670c4de # v1.4.0
+        with:
+          version: nightly
+
+      - name: Build contracts
+        run: forge build --build-info --out out --evm-version cancun
+
+      - uses: actions/setup-python@v5
+        with:
+          python-version: '3.11'
+
+      - run: |
+          python -m pip install --upgrade pip
+          pip install slither-analyzer==0.11.3
+
+      - name: Run Slither (High severity gate)
+        run: |
+          slither . \
+            --filter-paths "src/test/*|src/mocks/*|scripts/*|node_modules" \
+            --foundry-out-directory out \
+            --exclude-dependencies \
+            --exclude-medium \
+            --exclude-low \
+            --exclude-informational \
+            --fail-high \
+            --json slither-report.json \
+            --markdown-root slither-report.md
+
+      - uses: actions/upload-artifact@v4
+        with:
+          name: slither-static-analysis
+          path: |
+            slither-report.json
+            slither-report.md
diff --git a/src/L1/L1ScrollMessenger.sol b/src/L1/L1ScrollMessenger.sol
@@ -203,6 +203,8 @@ contract L1ScrollMessenger is ScrollMessengerBase, IL1ScrollMessenger {
         require(_from != xDomainMessageSender, "Invalid message sender");
 
         xDomainMessageSender = _from;
+        // xDomainMessageSender serves as reentrancy guard (notInExecution modifier).
+        // slither-disable-next-line reentrancy-eth
         (bool success, ) = _to.call{value: _value}(_message);
         // reset value to refund gas.
         xDomainMessageSender = ScrollConstants.DEFAULT_XDOMAIN_MESSAGE_SENDER;
@@ -316,6 +318,8 @@ contract L1ScrollMessenger is ScrollMessengerBase, IL1ScrollMessenger {
         // @note If the list is very long, the message may never be dropped.
         while (true) {
             // If the `_lastIndex` is from `messageQueueV2`, it will revert in `messageQueueV1.dropCrossDomainMessage`.
+            // call to messageQueueV1 is safe.
+            // slither-disable-next-line reentrancy-no-eth
             IL1MessageQueueV1(messageQueueV1).dropCrossDomainMessage(_lastIndex);
             _lastIndex = prevReplayIndex[_lastIndex];
             if (_lastIndex == 0) break;
@@ -328,6 +332,8 @@ contract L1ScrollMessenger is ScrollMessengerBase, IL1ScrollMessenger {
 
         // set execution context
         xDomainMessageSender = ScrollConstants.DROP_XDOMAIN_MESSAGE_SENDER;
+        // xDomainMessageSender serves as reentrancy guard (notInExecution modifier).
+        // slither-disable-next-line reentrancy-eth
         IMessageDropCallback(_from).onDropMessage{value: _value}(_message);
         // clear execution context
         xDomainMessageSender = ScrollConstants.DEFAULT_XDOMAIN_MESSAGE_SENDER;
diff --git a/src/L1/gateways/L1ETHGateway.sol b/src/L1/gateways/L1ETHGateway.sol
@@ -90,6 +90,8 @@ contract L1ETHGateway is ScrollGatewayBase, IL1ETHGateway, IMessageDropCallback
 
         // @note can possible trigger reentrant call to messenger,
         // but it seems not a big problem.
+        // no reentrancy risk (nonReentrant modifier).
+        // slither-disable-next-line arbitrary-send-eth
         (bool _success, ) = _to.call{value: _amount}("");
         require(_success, "ETH transfer failed");
 
@@ -108,6 +110,8 @@ contract L1ETHGateway is ScrollGatewayBase, IL1ETHGateway, IMessageDropCallback
 
         require(_amount == msg.value, "msg.value mismatch");
 
+        // no reentrancy risk (nonReentrant modifier).
+        // slither-disable-next-line arbitrary-send-eth
         (bool _success, ) = _receiver.call{value: _amount}("");
         require(_success, "ETH transfer failed");
 
diff --git a/src/L1/gateways/L1GatewayRouter.sol b/src/L1/gateways/L1GatewayRouter.sol
@@ -104,15 +104,19 @@ contract L1GatewayRouter is OwnableUpgradeable, IL1GatewayRouter {
      *****************************/
 
     /// @inheritdoc IL1GatewayRouter
-    /// @dev All the gateways should have reentrancy guard to prevent potential attack though this function.
+    /// @dev All the gateways should have reentrancy guard to prevent potential attack through this function.
     function requestERC20(
         address _sender,
         address _token,
         uint256 _amount
     ) external onlyInContext returns (uint256) {
         address _caller = _msgSender();
         uint256 _balance = IERC20Upgradeable(_token).balanceOf(_caller);
+
+        // only whitelisted caller allowed (onlyInContext modifier).
+        // slither-disable-next-line arbitrary-send-erc20
         IERC20Upgradeable(_token).safeTransferFrom(_sender, _caller, _amount);
+
         _amount = IERC20Upgradeable(_token).balanceOf(_caller) - _balance;
         return _amount;
     }
@@ -157,6 +161,8 @@ contract L1GatewayRouter is OwnableUpgradeable, IL1GatewayRouter {
         // encode msg.sender with _data
         bytes memory _routerData = abi.encode(_msgSender(), _data);
 
+        // gatewayInContext serves as reentrancy guard (onlyNotInContext modifier).
+        // slither-disable-next-line reentrancy-eth
         IL1ERC20Gateway(_gateway).depositERC20AndCall{value: msg.value}(_token, _to, _amount, _routerData, _gasLimit);
 
         // leave deposit context
diff --git a/src/L1/gateways/L1WETHGateway.sol b/src/L1/gateways/L1WETHGateway.sol
@@ -102,6 +102,8 @@ contract L1WETHGateway is L1ERC20Gateway {
         require(_l2Token == l2WETH, "l2 token not WETH");
         require(_amount == msg.value, "msg.value mismatch");
 
+        // deposit to WETH token is safe.
+        // slither-disable-next-line arbitrary-send-eth
         IWETH(_l1Token).deposit{value: _amount}();
     }
 
@@ -114,6 +116,8 @@ contract L1WETHGateway is L1ERC20Gateway {
         require(_token == WETH, "token not WETH");
         require(_amount == msg.value, "msg.value mismatch");
 
+        // deposit to WETH token is safe.
+        // slither-disable-next-line arbitrary-send-eth
         IWETH(_token).deposit{value: _amount}();
     }
 
diff --git a/src/L2/L2ScrollMessenger.sol b/src/L2/L2ScrollMessenger.sol
@@ -156,6 +156,8 @@ contract L2ScrollMessenger is ScrollMessengerBase, IL2ScrollMessenger {
 
         xDomainMessageSender = _from;
         // solhint-disable-next-line avoid-low-level-calls
+        // no reentrancy risk, only alias(l1ScrollMessenger) can call relayMessage.
+        // slither-disable-next-line reentrancy-eth
         (bool success, ) = _to.call{value: _value}(_message);
         // reset value to refund gas.
         xDomainMessageSender = ScrollConstants.DEFAULT_XDOMAIN_MESSAGE_SENDER;
diff --git a/src/L2/gateways/L2ETHGateway.sol b/src/L2/gateways/L2ETHGateway.sol
@@ -86,6 +86,8 @@ contract L2ETHGateway is ScrollGatewayBase, IL2ETHGateway {
         require(msg.value == _amount, "msg.value mismatch");
 
         // solhint-disable-next-line avoid-low-level-calls
+        // no reentrancy risk.
+        // slither-disable-next-line arbitrary-send-eth
         (bool _success, ) = _to.call{value: _amount}("");
         require(_success, "ETH transfer failed");
 
diff --git a/src/L2/gateways/L2WETHGateway.sol b/src/L2/gateways/L2WETHGateway.sol
@@ -110,6 +110,8 @@ contract L2WETHGateway is L2ERC20Gateway {
         require(_l2Token == WETH, "l2 token not WETH");
         require(_amount == msg.value, "msg.value mismatch");
 
+        // deposit to WETH token is safe.
+        // slither-disable-next-line arbitrary-send-eth
         IWETH(_l2Token).deposit{value: _amount}();
         IERC20Upgradeable(_l2Token).safeTransfer(_to, _amount);
 
diff --git a/src/L2/predeploys/L2TxFeeVault.sol b/src/L2/predeploys/L2TxFeeVault.sol
@@ -119,7 +119,9 @@ contract L2TxFeeVault is OwnableBase {
 
         emit Withdrawal(_value, recipient, msg.sender);
 
-        // no fee provided
+        // @note no fee provided
+        // transfer to messenger is safe.
+        // slither-disable-next-line arbitrary-send-eth
         IL2ScrollMessenger(messenger).sendMessage{value: _value}(
             recipient,
             _value,
diff --git a/src/L2/predeploys/WrappedEther.sol b/src/L2/predeploys/WrappedEther.sol
@@ -39,6 +39,8 @@ contract WrappedEther is ERC20Permit {
 
         _burn(_sender, wad);
 
+        // no reentrancy risk (checks-effects-interactions).
+        // slither-disable-next-line arbitrary-send-eth
         (bool success, ) = _sender.call{value: wad}("");
         require(success, "withdraw ETH failed");
 
diff --git a/src/batch-bridge/L1BatchBridgeGateway.sol b/src/batch-bridge/L1BatchBridgeGateway.sol
@@ -220,9 +220,13 @@ contract L1BatchBridgeGateway is AccessControlEnumerableUpgradeable, ReentrancyG
     function depositERC20(address token, uint96 amount) external nonReentrant {
         if (token == address(0)) revert ErrorIncorrectMethodForETHDeposit();
 
-        // common practice to handle fee on transfer token.
         uint256 beforeBalance = IERC20Upgradeable(token).balanceOf(address(this));
+
+        // no reentrancy risk (nonReentrant modifier).
+        // slither-disable-next-line reentrancy-no-eth
         IERC20Upgradeable(token).safeTransferFrom(_msgSender(), address(this), amount);
+
+        // common practice to handle fee on transfer token.
         amount = uint96(IERC20Upgradeable(token).balanceOf(address(this)) - beforeBalance);
 
         _deposit(token, _msgSender(), amount);
@@ -280,13 +284,17 @@ contract L1BatchBridgeGateway is AccessControlEnumerableUpgradeable, ReentrancyG
             accumulatedFee = IERC20Upgradeable(token).balanceOf(address(this)) - cachedTokenState.pending;
         }
         if (accumulatedFee > 0) {
+            // no reentrancy risk (onlyRole modifier).
+            // slither-disable-next-line reentrancy-eth, reentrancy-no-eth
             _transferToken(token, feeVault, accumulatedFee);
         }
 
         // deposit token to L2
         BatchState memory cachedBatchState = batches[token][cachedTokenState.pendingBatchIndex];
         address l2Token;
         if (token == address(0)) {
+            // transfer to messenger is safe.
+            // slither-disable-next-line arbitrary-send-eth
             IL1ScrollMessenger(messenger).sendMessage{value: cachedBatchState.amount + depositFee}(
                 counterpart,
                 cachedBatchState.amount,
@@ -298,6 +306,9 @@ contract L1BatchBridgeGateway is AccessControlEnumerableUpgradeable, ReentrancyG
             l2Token = IL1ERC20Gateway(gateway).getL2ERC20Address(token);
             IERC20Upgradeable(token).safeApprove(gateway, 0);
             IERC20Upgradeable(token).safeApprove(gateway, cachedBatchState.amount);
+
+            // transfer to whitelisted gateway is safe.
+            // slither-disable-next-line arbitrary-send-eth
             IL1ERC20Gateway(gateway).depositERC20{value: depositFee}(
                 token,
                 counterpart,
@@ -329,6 +340,8 @@ contract L1BatchBridgeGateway is AccessControlEnumerableUpgradeable, ReentrancyG
         // refund keeper fee
         unchecked {
             if (msg.value > depositFee + batchBridgeFee) {
+                // no reentrancy risk (onlyRole modifier).
+                // slither-disable-next-line reentrancy-eth, reentrancy-no-eth
                 _transferToken(address(0), _msgSender(), msg.value - depositFee - batchBridgeFee);
             }
         }
diff --git a/src/batch-bridge/L2BatchBridgeGateway.sol b/src/batch-bridge/L2BatchBridgeGateway.sol
@@ -232,6 +232,7 @@ contract L2BatchBridgeGateway is AccessControlEnumerableUpgradeable {
     ) private returns (bool success) {
         if (token == address(0)) {
             // We add gas limit here to avoid DDOS from malicious receiver.
+            // slither-disable-next-line arbitrary-send-eth
             (success, ) = receiver.call{value: amount, gas: SAFE_ETH_TRANSFER_GAS_LIMIT}("");
         } else {
             // We perform a low level call here, to bypass Solidity's return data size checking mechanism.
diff --git a/src/misc/ScrollOwner.sol b/src/misc/ScrollOwner.sol
@@ -136,6 +136,8 @@ contract ScrollOwner is AccessControlEnumerable {
         bytes calldata _data
     ) private {
         // solhint-disable-next-line avoid-low-level-calls
+        // no reentrancy risk.
+        // slither-disable-next-line arbitrary-send-eth
         (bool success, ) = _target.call{value: _value}(_data);
         if (!success) {
             // solhint-disable-next-line no-inline-assembly

Original file line number	Diff line number	Diff line change
`@@ -102,6 +102,8 @@ contract L1WETHGateway is L1ERC20Gateway {`
`102`	`102`	`require(_l2Token == l2WETH, "l2 token not WETH");`
`103`	`103`	`require(_amount == msg.value, "msg.value mismatch");`
`104`	`104`
	`105`	`+ // deposit to WETH token is safe.`
	`106`	`+ // slither-disable-next-line arbitrary-send-eth`
`105`	`107`	`IWETH(_l1Token).deposit{value: _amount}();`
`106`	`108`	`}`
`107`	`109`
`@@ -114,6 +116,8 @@ contract L1WETHGateway is L1ERC20Gateway {`
`114`	`116`	`require(_token == WETH, "token not WETH");`
`115`	`117`	`require(_amount == msg.value, "msg.value mismatch");`
`116`	`118`
	`119`	`+ // deposit to WETH token is safe.`
	`120`	`+ // slither-disable-next-line arbitrary-send-eth`
`117`	`121`	`IWETH(_token).deposit{value: _amount}();`
`118`	`122`	`}`
`119`	`123`