Be Chain - ValidatorSmartContract

Within the `ValidatorSmartContract`, the `_requireValidSignatures` function contains a logic bug involving the line: ```solidity uint16 validatorBit = uint16(1 << info.index); ``` This line attempts to track which root validators have signed a particular action using a 16-bit bitmap. The contract stores a list of validators in the `validators` array. Each validator has an associated `ValidatorInfo` struct, which includes a `uint16` index. This index is simply the validator’s position in the `validators` array. When a new root validator is added (via `addRootValidator`), `info.index` is set to the new `validators.length - 1`. This value can be between 0 and 254. Because a validator’s `info.index` can exceed 15, shifting `1 << info.index` and then casting to `uint16` truncates values above 15 bits to 0. Consequently, potential double-signature from two different root validators both above index 15 could map to 0 in the mask, so the code might not detect that they are distinct signers. (Though in practice it sees 0 for both and just checks `signersBitmap & 0 == 0`, so the second signature is also “accepted” but increments approvals incorrectly—leading to under- or over-counting.) This issue directly compromises the core security guarantee of multi-signature authorization among root validators. An action may fail to be authorized, or the contract may count incorrect signers.

The `queueAction` function allows the root validator to initiate `TimedAction` data for subsequent `executeAction`. The `actionType` in `queueAction` can be set any value, which is different from other functions, such as `APPROVE_VALIDATOR` in `approveValidator`. ```solidity=373 function queueAction( string memory actionId, string memory actionType, uint256 timeoutBlock, address targetAddress, ContractState newState, bytes[] memory signatures, bytes calldata _data ) external nonReentrant onlyRootValidator { if (_usedActionIds[actionId]) revert DuplicateActionError(); _usedActionIds[actionId] = true; _requireValidSignatures( actionId, actionType, timeoutBlock, targetAddress, newState, signatures ); ... } ``` If a one root validator disagrees with an `actionId` for functions such as `approveValidator` or `removeRootValidator`, that validator can front-run the function and consume the `actionId` in the `queueAction`. Consequently, functions like `approveValidator` or `removeRootValidator` will be reverted. This allows the root validator to veto an `actionId` easily.

In the `ValidatorSmartContract`, the functions that require multiple signatures (e.g., `approveValidator`, `disapproveValidator`, `addRootValidator`, `removeRootValidator`, `revokeValidator`, and `changeState`) only include `actionId` and `actionType` in the signed message. They do not include critical parameters such as the target `validator` address or the new contract `state`. This allows anyone who holds valid signatures for a certain (`actionId`, `actionType`) pair to reuse those signatures to carry out the same action type but on any arbitrary parameter.

In the `queueAction` function, the `_data` parameter can be set to any value without validation, which may threaten the system in the subsequent `executeAction`. For example, the `data` for `changeState` can be replaced by the `executeEmergencyWithdraw`.

The following codes in the function `executeAction()` do not meet the Checks-Effects-Interactions pattern. And there also exits a limited reentrancy issue. ```solidity=476 _timelockActionExecuting = true; (bool success, bytes memory returnData) = address(this).call( action.data ); _timelockActionExecuting = false; ... action.executed = true; ``` One edge case could be when the `EMERGENCY_WALLET` is a contract and `address(this).call` is used in the `executeEmergencyWithdraw` function. During the execution of the `EMERGENCY_WALLET.receive` function, it can call functions that do not have the `nonReentrant` modifier, for example, the `changeState` function. ```solidity function _safeTransfer(address to, uint256 amount) private { if (to == address(0) || to == address(this)) revert InvalidRecipientError(); if (address(this).balance < amount) revert InsufficientBalanceError(); (bool success, ) = payable(to).call{value: amount}(""); if (!success) { emit TransferFailed(to, amount); revert TransferFailedError(); } } ```

If an event is not indexed in a smart contract, it means that the event's parameters are not tagged with the `indexed` keyword. This has implications for how the event data can be searched and filtered when looking through blockchain logs. Without indexing, the event will still emit the data as part of the transaction log, but users won't be able to query for these events using the parameters. They'll have to retrieve the entire set of logs and manually sift through them to find events with the specific data. This can be less efficient and more time-consuming, especially on a blockchain with a high volume of transactions and events.

There should always be events emitted in the sensitive functions that are controlled by centralization roles.

If a validator is revoked by the admin, there is no enforced waiting period at all; the user can claim the pending stake immediately. By contrast, if the validator initiates their own exit, they must wait for the full `WITHDRAW_COOLDOWN` before claiming. It is inconsistent and can be unfair from the validator’s perspective. If the admin can instantly revoke you (maybe maliciously or by mistake) and you can immediately withdraw, it might sound good for you. On the other side, if an honest validator wants to leave, they must wait for the cooldown.

The signature has no timeout detection or expiration check. Attackers can hoard signatures of some operations rather than execute them, which may threaten the system in the future.

The `TimedAction` lacks timeout detection or expiration checks. Attackers can hold `TimedAction` for certain operations instead of executing them immediately once the `action.executeAfter` condition is met, which could pose a risk to the system in the future. ```solidity function executeAction( string memory actionId ) external nonReentrant onlyRootValidator { ... if (!success) { // Extract revert reason from return data if (returnData.length > 0) { // Bubble up the revert reason assembly { let returndata_size := mload(returnData) revert(add(32, returnData), returndata_size) } } else { revert ActionExecutionFailed(); } } action.executed = true; emit ActionExecuted(actionId); } ``` Furthermore, if the `executeAction` initially fails due to a revert, it can be re-executed later without any expiration checks. The `timeoutBlock` is used solely for `queueAction` and has no impact on `executeAction`.