Liquid Collective security record

During the review, we noticed that the smart contract does not prevent the addition of duplicate validators by an operator. From discussions with the team, some mitigations are in place, such as an off-chain process that validates the "valid" validator count and sets operator limits to the verified amount, likely through the `cmd/node_operators.go:NewCmdSetOperatorLimit()` command. Additionally, the provided client embeds logic to prevent duplicate additions in the `cmd/validators.go:NewCmdAddValidators()`. However, since operators can directly add validators to the smart contract and operator limit protections can be bypassed by front-running the transaction, a malicious operator could still add duplicate or already exited validators to the contract. We have attempted to assess the impact, which appears to primarily lower the APY of the protocol. Depositing to a duplicate validator, whether still active or already withdrawn, results in the deposited ETH becoming non-productive. This requires a validator sweep to recycle these inactive balances back to the protocol, where they can then be redeposited to an effective validator. However, as the smart contract is outside the scope of this audit, we can only review the impact with limited knowledge of the contract codebase. There is a risk of other unexpected results if the attack occurs.

In the current system design, exited validators are determined by the `computeValidatorsToExit()` function in `watcher.go`. If these validators choose not to exit, they will not be prioritized for selection as the next active validator, as described in the `pickNextValidatorsToDepositFromActiveOperators()` function in the `OperatorRegistry.1.sol` smart contract implementation. The issue is that the current incentive is insufficient as the operator can still earn shared rewards from those supposed-to-exit validators. The motivation to exit (not prioritized for the next validator activation), is lower than the incentive to not exit (a promised shared staking reward). Consequently, validators may opt not to exit and continue earning rewards without any risk of being exited.

In `oracle.go::getCurrentReportStatus()`, we have the following snippet which uses the `status` variable to keep track of which oracle member has reported consensus layer data to the oracle contract: ``` for i, a := range members { if status.Bit(i) == 1 { s.ReportedMembers = append(s.ReportedMembers, a) } } ``` Note that the `status` variable is essentially a `uint256` since it corresponds to the returned value from this function in `ReportPositions.sol`: ``` function getRaw() internal view returns (uint256) { return LibUnstructuredStorage.getStorageUint256(REPORTS_POSITIONS_SLOT); } ``` This means the maximum number of oracle members supported is 256. Particularly, when there are more than 256 members, there will be indices with a value greater than 255 (e.g., `uint256(memberIndex) > 255` in `Oracle.1.sol::reportConsensusLayerData()`). In `ReportPositions.sol::register()`, the code tracks which oracle member submitted a report by flipping the bit corresponding to its index in a uint256 variable, as follows: ``` function register(uint256 _idx) internal { uint256 mask = 1 << _idx; LibUnstructuredStorage.setStorageUint256( REPORTS_POSITIONS_SLOT, LibUnstructuredStorage.getStorageUint256(REPORTS_POSITIONS_SLOT) | mask ); } ``` However, this implies any oracle with an index over 255 will simply be ignored because the mask will simply be `uint256(0)` due to the bit shift. Currently, there are no enforced limits on how many oracle members can be registered (e.g., through `Oracle.1.sol::addMember()`), so it is possible to have more than 256 oracle members. When an oracle with an index greater than 255 reports consensus layer data (e.g., by calling `Oracle.1.sol::reportConsensusLayerData()`), it will fail because of the following check: ``` if (ReportsPositions.get(uint256(memberIndex))) { revert AlreadyReported(_report.epoch, msg.sender); } ``` Specifically, `ReportsPositions.get()` fails because `mask = 1 << _idx;` will simply be zero, so the condition `LibUnstructuredStorage.getStorageUint256(REPORTS_POSITIONS_SLOT) & mask == mask;` will always be true. ``` function get(uint256 _idx) internal view returns (bool) { uint256 mask = 1 << _idx; return LibUnstructuredStorage.getStorageUint256(REPORTS_POSITIONS_SLOT) & mask == mask; } ```

The `report.go:shouldReport()` function checks if `o.lastReportedEpoch >= epochSpec.FrameFirstEpoch()` and will return `false` if the daemon considers that it has already submitted a report for this frame. However, there are a few scenarios where resubmission might be preferred: 1. The admin makes changes to the operator member set or the quorum by calling `addMember()`, `removeMember()`, or `setQuorum()`, which will clear the reports on the contract side. 2. The daemon has attempted to submit the report through the `trySubmitReport()` function, but the transaction was eventually dropped or reverted. It might be better for the oracles to submit the report again in the above cases, or the system will need to wait for another frame (~1 day) before a new report can be submitted, risking the inability to reach the quorum.