Liquid Collective security record

The `AddToAllowlistEntry` and `RemoveFromAllowlistEntry` instructions have the following constraint for the `allower` signer value: ``` address = state.allower @ ErrorCode::OnlyAllower ``` However, the `allow_list_entry` account passed to both instructions only has the following constraint for its seeds: ``` seeds = [b"allow_list_entry", args.stake_pool.as_ref(), args.user_account.as_ref()] ``` This effectively means that a valid `state` has to be passed to the instruction and that the `allower` that signs the transaction has to be that state's allower. However, this also means that, as long as the allower passes a valid `stake_pool` account and a valid `user_account`, they can add/remove said user account to/from the allow list for the referenced stake pool. As a result, any account that has `allower` access to any state can add and remove users from any arbitrary stake pool they desire.

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.

The following functions do not validate public key values before setting them, resulting in setting/resetting account identifiers to the default public key if it is sent in the transaction. This is similar to the lack of zero address checks issue in Solidity code bases. - The `new_pauser` key in `processors::set_pauser::exec()` - The `new_allower` key in `processors::set_allower::exec()` - The `admin`, `allower`, `pauser`, and `initial_manager` keys in `processors::initialize::exec()`

The `FeeArgs` struct consists of two parts, a `numerator` and a `denominator`, representing the two parts of a fraction. There is currently no validation that the `denominator` part of the struct isn't a zero, which may lead to unwanted consequences and potential reverts if an invalid `FeeArgs` is sent in transactions.

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.