diff --git a/.github/workflows/contracts.yml b/.github/workflows/contracts.yml index b53668ed..4cfe981c 100644 --- a/.github/workflows/contracts.yml +++ b/.github/workflows/contracts.yml @@ -136,4 +136,59 @@ jobs: run: npx hardhat compile - name: Run hardhat tests - run: npx hardhat test \ No newline at end of file + 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 index c9700f3a..a54f467c 100644 --- 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 index c268a1ba..09c41ff9 100644 --- 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 index 25bf1aa7..5e9f30d4 100644 --- a/src/L1/gateways/L1GatewayRouter.sol +++ b/src/L1/gateways/L1GatewayRouter.sol @@ -104,7 +104,7 @@ 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, @@ -112,7 +112,11 @@ contract L1GatewayRouter is OwnableUpgradeable, IL1GatewayRouter { ) 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 index 9adfd7af..8c0f4b22 100644 --- 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 index 3d01b40e..3fb0541a 100644 --- 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 index ae9c42ff..695f1485 100644 --- 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 index a50a7b35..593c4cae 100644 --- 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 index 20248eb7..08cedf6d 100644 --- 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 index 64aa269f..fa5e5c72 100644 --- 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 index 8033213b..6f1dcbb3 100644 --- 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,6 +284,8 @@ 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); } @@ -287,6 +293,8 @@ contract L1BatchBridgeGateway is AccessControlEnumerableUpgradeable, ReentrancyG 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 index 84b95273..e069411a 100644 --- 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 index bb3bace7..bf7f3fbe 100644 --- 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