Inadequate Validation in updateQuorum and removeOracleMember

[hats-bug-reporter[bot]]

Github username: @0xmahdirostami Submission hash (on-chain): 0x5629c79aa1725c2a1490dabece300f911f04089b64c2a0a48b2ecba6799df2b7 Severity: medium

Description: Description\ In the system, there's a value known as quorum, which represents the number of reports required to consolidate a new rewards root (N/M, where N is the number of reports and M is the number of oracles). Two functions, updateQuorum and removeOracleMember, are used to manage these N and M values. However, there's a vulnerability in both of these functions.

The issue with updateQuorum is that it doesn't check the total number of oracles. This could lead to a situation where M (number of oracles) becomes less than N (quorum), which should be avoided.

Similarly, removeOracleMember doesn't take the quorum value into consideration. This can potentially result in the state where M is less than N.

Impact\ In this system, the quorum value (N) should always be less than or equal to the number of oracles (M). If this condition is not met, a new state root cannot be updated, which would cause delays in users receiving their rewards.

Attachments\ Revised Code File (Optional)

To address this vulnerability, a check needs to be added in both the updateQuorum and removeOracleMember functions. Here's the proposed fix for updateQuorum:

    function updateQuorum(uint64 newQuorum) external onlyGovernance {
        require(
            newQuorum != 0,
            "DappnodeSmoothingPool::updateQuorum: Quorum cannot be 0"
        );
+       require(
+            newQuorum <= oracleMembers.length,
+            "DappnodeSmoothingPool::updateQuorum: Quorum cannot be more than orcales"
        );
        quorum = newQuorum;
        emit UpdateQuorum(newQuorum);
    }

And here's the suggested fix for removeOracleMember:

function removeOracleMember(
        address oracleMemberAddress,
        uint256 oracleMemberIndex
    ) external onlyGovernance {
+          require(
+            oracleMembers.length -1 >= quorum,
+            "DappnodeSmoothingPool::removeOracleMember: oracles couldn't be less than quorum"

[invocamanman]

The governance in this system will be a multisig, and it's supposed to act correctly. The governance actually can stop the system, for example, setting the quorum to high, or deleting all the participants of the oracle. Or even add malicious oracle participants and steal the funds. My point is that this checks does not reduce any risk if we take in account the the governance is compromised.

That being said, is possible that in a future the governance might be another smart contract that could only perform certain functions ( which could be more secure than a multisig).

Also think makes sense to first set the quorum high and then the participants of the oracle. I understand that you propose first to add the participants and then update the quorum. But this leads to a window where the system is less secure, since there are more oracles with the same quorum cause could not be setted before.

Since the proposed checks does not reduce any trust assumptions, and constraint more the operations of the governance, making more difficult increment the quorum and oracle participants without taking any additional risk i think it's not worth to add them.