security: address slither findings and add slither CI (#46)

* add slither.config.json

* slither: ignore encode-packed-collision for _deployVault

* slither: ingore encode-packed-collision

* slither: ignore arbitrary-send-erc20 and add zero address check

* slither: use cached array length

* slither: ignore pragma issue

* slither: remove unused state variables

* add slither job

* slither: add comments for disabled detector

* format

* beauty comments

.github/workflows/slither.yml

@@ -0,0 +1,20 @@
+name: Slither Analysis
+
+on:
+  pull_request:
+  push:
+    branches:
+      - main
+      - release/**
+    tags:
+      - "*"
+
+jobs:
+  analyze:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v4
+
+      - uses: crytic/[email protected]
+        with:
+          fail-on: medium

slither.config.json

@@ -0,0 +1,19 @@
+{
+    "detectors_to_exclude": "pragma,assembly,solc-version,naming-convention,timestamp,low-level-calls,too-many-digits,similar-names,calls-loop,reentrancy-benign,reentrancy-events,dead-code",
+    "filter_paths": "lib/|test/|mocks/|BytesLib|script/",
+    "solc_remaps": [
+        "forge-std/=lib/forge-std/src/",
+        "ds-test/=lib/forge-std/lib/ds-test/src/",
+        "@layerzero-contracts/=lib/solidity-examples/contracts/",
+        "@openzeppelin/contracts/=lib/openzeppelin-contracts/contracts/",
+        "@openzeppelin/contracts-upgradeable/=lib/openzeppelin-contracts-upgradeable/contracts/",
+        "@layerzero-v2/=lib/LayerZero-v2/",
+        "@layerzerolabs/lz-evm-protocol-v2=lib/LayerZero-v2/protocol/",
+        "@layerzerolabs/lz-evm-oapp-v2=lib/LayerZero-v2/oapp/",
+        "@layerzerolabs/lz-evm-oapp-v2=lib/LayerZero-v2/oapp/",
+        "@layerzerolabs/lz-evm-messagelib-v2=lib/LayerZero-v2/messagelib/",
+        "@beacon-oracle=lib/eigenlayer-beacon-oracle/",
+        "solidity-bytes-utils/=lib/solidity-bytes-utils/"
+    ],
+    "compile_force_framework": "foundry"
+}

src/core/BaseRestakingController.sol

@@ -29,6 +29,7 @@ abstract contract BaseRestakingController is
         whenNotPaused
         nonReentrant
     {
+        require(recipient != address(0), "BaseRestakingController: recipient address cannot be empty or zero address");
         if (token == VIRTUAL_STAKED_ETH_ADDRESS) {
             IExoCapsule capsule = _getCapsule(msg.sender);
             capsule.withdraw(amount, payable(recipient));

src/core/Bootstrap.sol

@@ -158,6 +158,10 @@ contract Bootstrap is
         _addWhitelistTokens(tokens);
     }
 
+    // Though `_deployVault` would make external call to newly created `Vault` contract and initialize it,
+    // `Vault` contract belongs to Exocore and we could make sure its implementation does not have dangerous behavior
+    // like reentrancy.
+    // slither-disable-next-line reentrancy-no-eth
     function _addWhitelistTokens(address[] calldata tokens) internal {
         for (uint256 i; i < tokens.length; i++) {
             address token = tokens[i];
@@ -228,7 +232,8 @@ contract Bootstrap is
      */
     function consensusPublicKeyInUse(bytes32 newKey) public view returns (bool) {
         require(newKey != bytes32(0), "Consensus public key cannot be zero");
-        for (uint256 i = 0; i < registeredOperators.length; i++) {
+        uint256 arrayLength = registeredOperators.length;
+        for (uint256 i = 0; i < arrayLength; i++) {
             address ethAddress = registeredOperators[i];
             string memory exoAddress = ethToExocoreAddress[ethAddress];
             if (operators[exoAddress].consensusPublicKey == newKey) {
@@ -274,7 +279,8 @@ contract Bootstrap is
      * safely used for a new operator.
      */
     function nameInUse(string memory newName) public view returns (bool) {
-        for (uint256 i = 0; i < registeredOperators.length; i++) {
+        uint256 arrayLength = registeredOperators.length;
+        for (uint256 i = 0; i < arrayLength; i++) {
             address ethAddress = registeredOperators[i];
             string memory exoAddress = ethToExocoreAddress[ethAddress];
             if (keccak256(abi.encodePacked(operators[exoAddress].name)) == keccak256(abi.encodePacked(newName))) {
@@ -471,6 +477,11 @@ contract Bootstrap is
     }
 
     // implementation of ILSTRestakingController
+    // Though `_deposit` would make external call to `Vault` and some state variables would be written in the following
+    // `_delegateTo`,
+    // `Vault` contract belongs to Exocore and we could make sure it's implementation does not have dangerous behavior
+    // like reentrancy.
+    // slither-disable-next-line reentrancy-no-eth
     function depositThenDelegateTo(address token, uint256 amount, string calldata operator)
         external
         payable

src/core/ClientGatewayLzReceiver.sol

@@ -22,6 +22,8 @@ abstract contract ClientGatewayLzReceiver is PausableUpgradeable, OAppReceiverUp
         _;
     }
 
+    // This function would call other functions inside this contract through low-level-call
+    // slither-disable-next-line reentrancy-no-eth
     function _lzReceive(Origin calldata _origin, bytes calldata payload) internal virtual override whenNotPaused {
         if (_origin.srcEid != EXOCORE_CHAIN_ID) {
             revert UnexpectedSourceChain(_origin.srcEid);
@@ -183,6 +185,10 @@ abstract contract ClientGatewayLzReceiver is PausableUpgradeable, OAppReceiverUp
         emit DepositThenDelegateResult(delegateSuccess, delegator, operator, token, amount);
     }
 
+    // Though `_deployVault` would make external call to newly created `Vault` contract and initialize it,
+    // `Vault` contract belongs to Exocore and we could make sure its implementation does not have dangerous behavior
+    // like reentrancy.
+    // slither-disable-next-line reentrancy-no-eth
     function afterReceiveAddWhitelistTokensRequest(bytes calldata requestPayload)
         public
         onlyCalledFromThis

src/core/CustomProxyAdmin.sol

@@ -14,14 +14,17 @@ contract CustomProxyAdmin is Initializable, ProxyAdmin {
     constructor() ProxyAdmin() {}
 
     function initialize(address newBootstrapper) external initializer onlyOwner {
+        require(newBootstrapper != address(0), "CustomProxyAdmin: newBootstrapper cannot be zero or empty address");
         bootstrapper = newBootstrapper;
     }
 
     function changeImplementation(address proxy, address implementation, bytes memory data) public virtual {
         require(msg.sender == bootstrapper, "CustomProxyAdmin: sender must be bootstrapper");
         require(msg.sender == proxy, "CustomProxyAdmin: sender must be the proxy itself");
-        ITransparentUpgradeableProxy(proxy).upgradeToAndCall(implementation, data);
+
+        // we follow check-effects-interactions pattern to write state before external call
         bootstrapper = address(0);
+        ITransparentUpgradeableProxy(proxy).upgradeToAndCall(implementation, data);
     }
 
 }

src/core/ExoCapsule.sol

@@ -153,9 +153,8 @@ contract ExoCapsule is Initializable, ExoCapsuleStorage, IExoCapsule {
     }
 
     function withdraw(uint256 amount, address payable recipient) external onlyGateway {
-        require(
-            amount <= withdrawableBalance, "ExoCapsule: withdrawal amount is larger than staker's withdrawable balance"
-        );
+        require(recipient != address(0), "ExoCapsule: recipient address cannot be zero or empty");
+        require(amount > 0 && amount <= withdrawableBalance, "ExoCapsule: invalid withdrawal amount");
 
         withdrawableBalance -= amount;
         (bool sent,) = recipient.call{value: amount}("");

src/core/ExocoreGateway.sol

@@ -169,6 +169,9 @@ contract ExocoreGateway is
         super.setPeer(clientChainId, clientChainGateway);
     }
 
+    // Though this function would call precompiled contract, all precompiled contracts belong to Exocore
+    // and we could make sure its implementation does not have dangerous behavior like reentrancy.
+    // slither-disable-next-line reentrancy-no-eth
     function addWhitelistTokens(
         uint32 clientChainId,
         bytes32[] calldata tokens,

src/core/NativeRestakingController.sol

@@ -45,6 +45,9 @@ abstract contract NativeRestakingController is
         emit StakedWithCapsule(msg.sender, address(capsule));
     }
 
+    // The bytecode returned by `BEACON_PROXY_BYTECODE` and `EXO_CAPSULE_BEACON` address are actually fixed size of byte
+    // array, so it would not cause collision for encodePacked
+    // slither-disable-next-line encode-packed-collision
     function createExoCapsule() public whenNotPaused nativeRestakingEnabled returns (address) {
         require(
             address(ownerToCapsule[msg.sender]) == address(0),
@@ -58,8 +61,10 @@ abstract contract NativeRestakingController is
                 abi.encodePacked(BEACON_PROXY_BYTECODE.getBytecode(), abi.encode(address(EXO_CAPSULE_BEACON), ""))
             )
         );
-        capsule.initialize(address(this), msg.sender, BEACON_ORACLE_ADDRESS);
+
+        // we follow check-effects-interactions pattern to write state before external call
         ownerToCapsule[msg.sender] = capsule;
+        capsule.initialize(address(this), msg.sender, BEACON_ORACLE_ADDRESS);
 
         emit CapsuleCreated(msg.sender, address(capsule));
 

src/core/Vault.sol

@@ -49,6 +49,9 @@ contract Vault is Initializable, VaultStorage, IVault {
         emit WithdrawalSuccess(withdrawer, recipient, amount);
     }
 
+    // Though `safeTransferFrom` has arbitrary passed in `depositor` as sender, this function is only callable by
+    // `gateway` and `gateway` would make sure only the `msg.sender` would be the depositor.
+    // slither-disable-next-line arbitrary-send-erc20
     function deposit(address depositor, uint256 amount) external payable onlyGateway {
         underlyingToken.safeTransferFrom(depositor, address(this), amount);
         totalDepositedPrincipalAmount[depositor] += amount;

src/libraries/BeaconChainProofs.sol

@@ -13,47 +13,12 @@ library BeaconChainProofs {
 
     // constants are the number of fields and the heights of the different merkle trees used in merkleizing
     // beacon chain containers
-    uint256 internal constant NUM_BEACON_BLOCK_HEADER_FIELDS = 5;
     uint256 internal constant BEACON_BLOCK_HEADER_FIELD_TREE_HEIGHT = 3;
 
-    uint256 internal constant NUM_BEACON_BLOCK_BODY_FIELDS = 11;
     uint256 internal constant BEACON_BLOCK_BODY_FIELD_TREE_HEIGHT = 4;
 
-    uint256 internal constant NUM_BEACON_STATE_FIELDS = 21;
     uint256 internal constant BEACON_STATE_FIELD_TREE_HEIGHT = 5;
 
-    uint256 internal constant NUM_ETH1_DATA_FIELDS = 3;
-    uint256 internal constant ETH1_DATA_FIELD_TREE_HEIGHT = 2;
-
-    uint256 internal constant NUM_VALIDATOR_FIELDS = 8;
-    uint256 internal constant VALIDATOR_FIELD_TREE_HEIGHT = 3;
-
-    uint256 internal constant NUM_EXECUTION_PAYLOAD_HEADER_FIELDS = 15;
-    uint256 internal constant EXECUTION_PAYLOAD_HEADER_FIELD_TREE_HEIGHT = 4;
-
-    uint256 internal constant NUM_EXECUTION_PAYLOAD_FIELDS = 15;
-    uint256 internal constant EXECUTION_PAYLOAD_FIELD_TREE_HEIGHT = 4;
-
-    // HISTORICAL_ROOTS_LIMIT	 = 2**24, so tree height is 24
-    uint256 internal constant HISTORICAL_ROOTS_TREE_HEIGHT = 24;
-
-    // HISTORICAL_BATCH is root of state_roots and block_root, so number of leaves =  2^1
-    uint256 internal constant HISTORICAL_BATCH_TREE_HEIGHT = 1;
-
-    // SLOTS_PER_HISTORICAL_ROOT = 2**13, so tree height is 13
-    uint256 internal constant STATE_ROOTS_TREE_HEIGHT = 13;
-    uint256 internal constant BLOCK_ROOTS_TREE_HEIGHT = 13;
-
-    //HISTORICAL_ROOTS_LIMIT = 2**24, so tree height is 24
-    uint256 internal constant HISTORICAL_SUMMARIES_TREE_HEIGHT = 24;
-
-    //Index of block_summary_root in historical_summary container
-    uint256 internal constant BLOCK_SUMMARY_ROOT_INDEX = 0;
-
-    uint256 internal constant NUM_WITHDRAWAL_FIELDS = 4;
-    // tree height for hash tree of an individual withdrawal container
-    uint256 internal constant WITHDRAWAL_FIELD_TREE_HEIGHT = 2;
-
     uint256 internal constant VALIDATOR_TREE_HEIGHT = 40;
 
     // MAX_WITHDRAWALS_PER_PAYLOAD = 2**4, making tree height = 4
@@ -65,45 +30,15 @@ library BeaconChainProofs {
 
     // in beacon block header
     // https://github.com/ethereum/consensus-specs/blob/dev/specs/phase0/beacon-chain.md#beaconblockheader
-    uint256 internal constant SLOT_INDEX = 0;
-    uint256 internal constant PROPOSER_INDEX_INDEX = 1;
     uint256 internal constant STATE_ROOT_INDEX = 3;
     uint256 internal constant BODY_ROOT_INDEX = 4;
     // in beacon state
     // https://github.com/ethereum/consensus-specs/blob/dev/specs/capella/beacon-chain.md#beaconstate
-    uint256 internal constant HISTORICAL_BATCH_STATE_ROOT_INDEX = 1;
-    uint256 internal constant BEACON_STATE_SLOT_INDEX = 2;
-    uint256 internal constant LATEST_BLOCK_HEADER_ROOT_INDEX = 4;
-    uint256 internal constant BLOCK_ROOTS_INDEX = 5;
-    uint256 internal constant STATE_ROOTS_INDEX = 6;
-    uint256 internal constant HISTORICAL_ROOTS_INDEX = 7;
-    uint256 internal constant ETH_1_ROOT_INDEX = 8;
     uint256 internal constant VALIDATOR_TREE_ROOT_INDEX = 11;
-    uint256 internal constant EXECUTION_PAYLOAD_HEADER_INDEX = 24;
-    uint256 internal constant HISTORICAL_SUMMARIES_INDEX = 27;
-
-    // in validator
-    // https://github.com/ethereum/consensus-specs/blob/dev/specs/phase0/beacon-chain.md#validator
-    uint256 internal constant VALIDATOR_PUBKEY_INDEX = 0;
-    uint256 internal constant VALIDATOR_WITHDRAWAL_CREDENTIALS_INDEX = 1;
-    uint256 internal constant VALIDATOR_BALANCE_INDEX = 2;
-    uint256 internal constant VALIDATOR_SLASHED_INDEX = 3;
-    uint256 internal constant VALIDATOR_WITHDRAWABLE_EPOCH_INDEX = 7;
-
-    // in execution payload header
-    uint256 internal constant TIMESTAMP_INDEX = 9;
-    uint256 internal constant WITHDRAWALS_ROOT_INDEX = 14;
 
     //in execution payload
     uint256 internal constant WITHDRAWALS_INDEX = 14;
 
-    // in withdrawal
-    uint256 internal constant WITHDRAWAL_VALIDATOR_INDEX_INDEX = 1;
-    uint256 internal constant WITHDRAWAL_VALIDATOR_AMOUNT_INDEX = 3;
-
-    //In historicalBatch
-    uint256 internal constant HISTORICALBATCH_STATEROOTS_INDEX = 1;
-
     //Misc Constants
 
     /// @notice The number of slots each epoch in the beacon chain
@@ -113,10 +48,10 @@ library BeaconChainProofs {
     uint64 internal constant SECONDS_PER_SLOT = 12;
 
     /// @notice Number of seconds per epoch: 384 == 32 slots/epoch * 12 seconds/slot
+    /// @dev This constant would be used by other contracts that import this library
+    // slither-disable-next-line unused-state
     uint64 internal constant SECONDS_PER_EPOCH = SLOTS_PER_EPOCH * SECONDS_PER_SLOT;
 
-    bytes8 internal constant UINT64_MASK = 0xffffffffffffffff;
-
     /// @notice This struct contains the merkle proofs and leaves needed to verify a partial/full withdrawal
     struct WithdrawalProof {
         bytes withdrawalProof;

src/storage/BootstrapStorage.sol

@@ -452,6 +452,9 @@ contract BootstrapStorage is GatewayStorage {
         return true;
     }
 
+    // The bytecode returned by `BEACON_PROXY_BYTECODE` and `EXO_CAPSULE_BEACON` address are actually fixed size of byte
+    // array, so it would not cause collision for encodePacked
+    // slither-disable-next-line encode-packed-collision
     function _deployVault(address underlyingToken) internal returns (IVault) {
         Vault vault = Vault(
             Create2.deploy(

src/storage/ClientChainGatewayStorage.sol

@@ -23,9 +23,13 @@ contract ClientChainGatewayStorage is BootstrapStorage {
     IBeacon public immutable EXO_CAPSULE_BEACON;
 
     // constant state variables
+    uint256 internal constant TOKEN_ADDRESS_BYTES_LENGTH = 32;
     uint256 internal constant GWEI_TO_WEI = 1e9;
     address internal constant VIRTUAL_STAKED_ETH_ADDRESS = 0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE;
     IETHPOSDeposit internal constant ETH_POS = IETHPOSDeposit(0x00000000219ab540356cBB839Cbe05303d7705Fa);
+    // constants used for layerzero messaging
+    uint128 internal constant DESTINATION_GAS_LIMIT = 500_000;
+    uint128 internal constant DESTINATION_MSG_VALUE = 0;
 
     uint256[40] private __gap;
 

src/storage/ExocoreGatewayStorage.sol

@@ -16,8 +16,10 @@ contract ExocoreGatewayStorage is GatewayStorage {
     uint256 internal constant CLAIM_REWARD_REQUEST_LENGTH = 96;
     // bytes32 token + bytes32 delegator + bytes(42) operator + uint256 amount
     uint256 internal constant DEPOSIT_THEN_DELEGATE_REQUEST_LENGTH = DELEGATE_REQUEST_LENGTH;
-    uint256 internal constant UINT8_BYTES_LENGTH = 1;
-    uint256 internal constant UINT256_BYTES_LENGTH = 32;
+
+    // constants used for layerzero messaging
+    uint128 internal constant DESTINATION_GAS_LIMIT = 500_000;
+    uint128 internal constant DESTINATION_MSG_VALUE = 0;
 
     mapping(uint32 clienChainId => bool) public chainToBootstrapped;
     mapping(uint32 clienChainId => bool registered) public isRegisteredClientChain;

src/storage/GatewayStorage.sol

@@ -14,11 +14,6 @@ contract GatewayStorage {
         RESPOND
     }
 
-    /* ----------------- constants used for layerzero messaging ----------------- */
-    uint256 internal constant TOKEN_ADDRESS_BYTES_LENGTH = 32;
-    uint128 internal constant DESTINATION_GAS_LIMIT = 500_000;
-    uint128 internal constant DESTINATION_MSG_VALUE = 0;
-
     mapping(Action => bytes4) internal _whiteListFunctionSelectors;
     address payable public exocoreValidatorSetAddress;
 

test/mocks/EndpointV2Mock.sol

@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: UNLICENSED
-pragma solidity 0.8.22;
+pragma solidity ^0.8.20;
 
 import {WorkerOptions} from "@layerzerolabs/lz-evm-messagelib-v2/contracts/SendLibBase.sol";
 import {IExecutorFeeLib} from "@layerzerolabs/lz-evm-messagelib-v2/contracts/interfaces/IExecutorFeeLib.sol";

test/mocks/NonShortCircuitEndpointV2Mock.sol

@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: UNLICENSED
-pragma solidity 0.8.22;
+pragma solidity ^0.8.20;
 
 import {WorkerOptions} from "@layerzerolabs/lz-evm-messagelib-v2/contracts/SendLibBase.sol";
 import {IExecutorFeeLib} from "@layerzerolabs/lz-evm-messagelib-v2/contracts/interfaces/IExecutorFeeLib.sol";