Liquid Collective Lceth

In `oracle/report.go:generateReport()`, there is a snippet of code that calculates the skimmed balance: ``` case withdrawalEpoch >= withdrawableEpoch: // All amounts below 32 ETH are considered exited validatorsExitedBalance += MinGwei( withdrawal.Amount, beaconcommon.Gwei(32_000_000_000), ) // All amounts above 32 ETH are considered skimmed validatorsSkimmedBalance += MaxGwei( 0, withdrawal.Amount - beaconcommon.Gwei(32_000_000_000), ) ``` However, the calculation of `validatorsSkimmedBalance` can potentially underflow when `withdrawal.Amount` is less than 32 ETH. Since the underlying type of `withdrawal.Amount` is simply `uint64`, underflowing will result in setting `validatorsSkimmedBalance` to a value larger than `2^64 - 1 - 32_000_000_000 = 1.8446744e+19 GWEI`, approximately 150 times the current ETH supply. When a validator is slashed, they are forced to exit the validator set, with the `withdrawableEpoch` set to 36 days after the slashing event. This means the underflow illustrated above can occur around 36 days after a slashing event since the withdrawable amount of a slashed validator is lower than 32 ETH. Consequently, the report submission to the smart contract side will likely revert at `OracleManager.1.sol::setConsensusLayerData() -> River.1.sol::_pullCLFunds()` because `collectedCLFunds` will not be equivalent to `_skimmedEthAmount + _exitedEthAmount`, given that `_skimmedEthAmount` will be a significantly large amount. ``` function _pullCLFunds(uint256 _skimmedEthAmount, uint256 _exitedEthAmount) internal override { ... IWithdrawV1(WithdrawalCredentials.getAddress()).pullEth(totalAmountToPull); uint256 collectedCLFunds = address(this).balance - currentBalance; if (collectedCLFunds != _skimmedEthAmount + _exitedEthAmount) { revert InvalidPulledClFundsAmount(_skimmedEthAmount + _exitedEthAmount, collectedCLFunds); } ... } ``` This could have an impact as it might cause a denial of service on the oracles since no one will be able to submit a consensus layer report to the smart contract; it will revert due to the underflow issue described.

Here are some potential improvements to the codebase that can enhance the readability or performance of the program: 1. In `oracle/types.go`, the `Validator` struct has a typo in the field `widthdrawals`, which should be `withdrawals`. 2. In `cmd/allowlist.go:NewCmdAllowlistDeny()`, the sentence in the "Short" description seems unfinished: "Transact to deny accounts (disabling all operations including)". 3. In `cmd/validator.go:NewCmdAddValidators()`, consider adding `defer trackTicker.Stop()` and `defer trackTimerMax.Stop()` right after the initiation of the tickers to ensure the `Stop()` function is called even if the `NewCmdAddValidators()` function exits early. 4. Consider lowering the generated folder permissions in `cmd/oracle.go:NewCmdOracleRun()#L345`. The permission `0700` might be sufficient for the database's usage. 5. In `oracle/validators.go:syncCLValidators()`, the line `validatorIDs := make([]string, 0)` can be optimized by using `validatorIDs := make([]string, 0, len(o.fundedValidatorKeys))` to avoid capacity reallocation.

The Golang security checker `govulncheck` has reported that the project relies on external libraries with known vulnerabilities, particularly in the native Golang libraries `net/http` and `crypto`. During our review, we believe that these vulnerabilities are not directly exploitable given the current project design. However, we recommend keeping all dependencies up-to-date to mitigate potential security risks in the future.

In `exit/watcher.go:computeValidatorsToExit()`, the function computes a list of exitable validators and counts the number of exited validators using the statuses `active_exiting`, `exited_unslashed`, `withdrawal_possible`, and `withdrawal_done`, but it does not consider the slashed validators as exited. However, it should be noted that after the penalty period (i.e., 36 days) for a slashed validator, it will be forced to exit, eventually moving its status to `withdrawal_possible` (See [here](https://docs.blockdaemon.com/docs/eth-validator-statuses)). This means that the slashed validators will be considered as exited in a later invocation of the function `computeValidatorToExit()` (say after 40 days) because their status will have changed to `withdrawal_possible`. This can potentially lead to an undesired state where `exited > toExitCount`, where the system considers the exited slashed validators as unsolicited exits. ``` // In watcher.go:computeValidatorsToExit() if exited > toExitCount { return nil, fmt.Errorf("expecting to exit a total of %d; currently exited: %d; unsolicited exits: %d", toExitCount, exited, exited - toExitCount) } ``` This will return an error, causing the daemon to fail and either retry or wait for the next tick. However, unless a new exit is requested, it will remain in the current status. Note that similarly, in the `oracle/report.go:generateReport()` function, the slashed validator will not be counted as a "stopped validator" and the `validatorsExitingBalance` will not take into account those validators until they are in the "withdrawal" statuses. ``` // In oracle/report.go:generateReport() ... if v.IsStatus( StatusActiveExiting, StatusExitedUnslashed, StatusExited, StatusWithdrawal, StatusWithdrawalDone, StatusWithdrawalPossible, ) { validatorsExitingBalance += v.clData.Balance stoppedValidatorCountPerOperator[0]++ // sum stoppedValidatorCountPerOperator[v.operatorIdx+1]++ // count per operator } ... ``` After the discussion with the team, they confirmed that they intended to treat slashed validators similarly to unsolicitedly exited validators. Additionally, the team confirmed that despite the gap in accounting for the "exiting balance" in specific statuses, this is also considered. Given that the smart contract side is considered safe, we value this issue's severity as Informational.

The `core.go:getLastConsensusLayerReport()` function retrieves the report data from the smart contract and returns the `types.CLReport` type. The "balance" fields: `ActiveValidatorsBalance`, `SkimmedValidatorsBalance`, `ExitedValidatorsBalance`, and `ExitingValidatorsBalance` are directly cast to `beaconcommon.Gwei` from the numbers received from the contract. However, in the contract, the values are stored in the unit of wei instead of Gwei. Thus, there is a misplacement of the field values here. We have not found any usage of these impacted "balance" fields within the codebase, and we assign a lower severity to the issue. However, if the codebase is imported and used in a different repository, there could be a larger impact.

When the system retrieves the latest oracle report through `core.go::GetLastConsensusLayerReport()`, there's an implicit assumption that the reported data has reached a majority consensus from all the oracle members. ``` See comments above Oracle.1.sol::_clearReportsAndSetQuorum(). /// @dev The quorum value Q should respect the following invariant, where O is oracle member count /// @dev (O / 2) + 1 <= Q <= O ``` However, this may not be the case as the quorum can be set to any integer irrespective of the number of oracles in the system. In particular, when adding a new oracle member to the system in `Oracle.1.sol::addMember()`, it allows the admin to set a new quorum, which internally calls `Oracle.1.sol::_clearReportsAndSetQuorum()`. However, the invariant where the quorum is at least half the number of oracle members is not actually enforced, as shown below. ``` function _clearReportsAndSetQuorum(uint256 _newQuorum, uint256 _previousQuorum) internal { uint256 memberCount = OracleMembers.get().length; if ((_newQuorum == 0 && memberCount > 0) || _newQuorum > memberCount) { revert LibErrors.InvalidArgument(); } _clearReports(); if (_newQuorum != _previousQuorum) { Quorum.set(_newQuorum); emit SetQuorum(_newQuorum); } } ```

In the `exit.go:NewCmdExitRun()` function, the `webhookCallback` will be a `nil` pointer if it does not enter the `if v.GetBool(webhookFlag.ViperKey) && !dryRun {}` code block that sets the webhook. In other words, the webhook will be `nil` if it is in dry run mode, or if the `webhookFlag.ViperKey` environment or config is not set. The `webhookCallback` will be called in the `watcher.go:runExitRequestWatcher()` function as long as it is not in dry-run mode. There is a risk of the program panicking due to triggering the function of a `nil` pointer.

`gosec` reports the following places with the risk of unintended file path can be specified: 1. `cmd/validators.go:NewCmdAddValidators()#L289`: the `depositInputFile` variable. 2. `cmd/validators.go:NewCmdAddValidators()#L273`: the `depositDataFile` variable. 3. `cmd/gen.go:GenMarkdownTreeCustom()#L99`: the `file` variable. 4. `cmd/deposit_data.go:NewCmdDepositDataValidate()#L33`: the `depositDataFile` variable.

The `oracle.go:GetCurrentReportStatus()` function is used in the `report.go:report()` function to provide the report status from the contract. However, the underlying `getCurrentReportStatus()` function makes several calls to build the `types.Status` data. These calls do not ensure reading from the same block, and there is a risk that they might read inconsistent data. For instance, the code first calls `members, err := c.getOracleMembers(opts)` and then `status, err := c.oracle.GetGlobalReportStatus(opts)`. The `s.ReportedMembers` are built by the status' bits. If the member order changes due to the removal of oracle members, then the `ReportedMembers` will be incorrectly flagged as the status no longer reflects the correct ordering. Another potential impact is that the members might not match the quorum due to the `members, err := c.getOracleMembers(opts)` and `s.Quorum, err = c.getQuorum(opts)` calls potentially reading from different blocks. If the quorum is larger than the number of members due to new members joining between the calls, it can risk causing the `computeWaitEpoch()` function to panic on the line `range opOrder[quorum:]` as it may attempt to slice the array with an index that is out of bounds.

The `report.go:refreshFrameReport()` function reuses some data from `currentEpoch` for `currentFrame` in the line: `currentFrame := currentEpoch // Copy CLSpec & OperatorsCount from currentEpoch`. The `CLSpec` is very unlikely to change as it is tied to the consensus, but the operator count can be updated and cause the `currentFrame` to use an incorrect value. Currently, the `OperatorsCount` is only used to initialize the `stoppedValidatorCountPerOperator` array in the `generateReport()` function. From our understanding, operators can only be added but not removed on the contract side, so the operator count will only increase over time. As a result, even if the `currentFrame` uses an incorrect operator count, it will only waste some memory to initialize unnecessary slots for the array. There should not be any impact on business logic based on our current analysis. However, we would like to alert the team to the risk and any future usage of the `OperatorsCount` data here.