cergyk - BlockSpecimenProofChain::finalizeSpecimenSession strict inequality yields wrong quorum ratio

[sherlock-admin2]

cergyk

medium

BlockSpecimenProofChain::finalizeSpecimenSession strict inequality yields wrong quorum ratio

Summary

A quorum factor is used to determine the valid entry for a block specimen, however in some cases a valid quorum may be rejected because of a strict inequality condition upon check in finalizeSpecimenSession

Vulnerability Detail

The following check in BlockSpecimenProofChain::finalizeSpecimenSession checks that participants is strictly greater than the applied quorum ratio: https://github.com/sherlock-audit/2023-11-covalent/blob/main/cqt-staking/contracts/BlockSpecimenProofChain.sol#L431

However we can see that with the default parameters this check is incorrect:

• default quorum threshold = 50% https://github.com/sherlock-audit/2023-11-covalent/blob/main/cqt-staking/contracts/BlockSpecimenProofChain.sol#L134

• minimum submissions required = 2 https://github.com/sherlock-audit/2023-11-covalent/blob/main/cqt-staking/contracts/BlockSpecimenProofChain.sol#L137

This means that in the case 2 of 4 validators agree on a block specimen, the quorum is rejected, because: (2*_DIVIDER/4) > _DIVIDER/2 is not verified

This means that legitimate block specimen end up audited

Impact

Some quorums are marked as invalid during finalization step, and end up DoSing the process of producing block specimens

Code Snippet

Tool used

Manual Review

Recommendation

Please consider replacing with a gte check:

-if (_minSubmissionsRequired <= max && (max * _DIVIDER) / contributorsN > _blockSpecimenQuorum) {
+if (_minSubmissionsRequired <= max && (max * _DIVIDER) / contributorsN >= _blockSpecimenQuorum) {

[sherlock-admin2]

1 comment(s) were left on this issue during the judging contest.

takarez commented:

valid: there is inconsistency in the calculations of the quorum; medium(7)

[noslav]

fixed by fix quorum calc in case of 4 auditors and default values - sa55/sa115

[midori-fuse]

Escalate

This issue is valid but low severity. No funds are lost, no contract functions are broken, admin can just set the quorum down by a single unit (that is, $10^{-16}$ percent) to mitigate, and no sessions are disrupted.

The only impact is that, for sessions which quorum yields exactly the quorum ratio, _finalizeWithParticipants() is not called, but the function only handles read-only storage values and emits an event anyway. Regardless of the quorum outcome, the session is still ended, and nothing is DOS-ed. The impact is thus that of low severity.

[sherlock-admin]

Escalate

This issue is valid but low severity. No funds are lost, no contract functions are broken, admin can just set the quorum down by a single unit (that is, $10^{-16}$ percent) to mitigate, and no sessions are disrupted.

The only impact is that, for sessions which quorum yields exactly the quorum ratio, _finalizeWithParticipants() is not called, but the function only handles read-only storage values and emits an event anyway. Regardless of the quorum outcome, the session is still ended, and nothing is DOS-ed. The impact is thus that of low severity.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[Oot2k]

Agree with escalation here. Furthermore currently an audit is required in any case (no matter if quorum is reached or not). Also in my opinion the usage of ">=" creates more issues, in case 2 out of 4 agree on one proof, and the quorum is set to 50%, it is correct to require an Audit. If this is not the case and one of the two is used, there is no way to guarantee which one is correct.

[whoismxuse]

Disagree with escalation.

It's evident that services are being disrupted, particularly since the team established a 50% base quorum with a clear rationale. 
setQuorumThreshold(_DIVIDER / 2); // 50% setMinSubmissionsRequired(2);

The missing” =“ sign implies that in a scenario that occurs frequently, the logic would fail and incorrect executions will be made. Also it makes 0 sense to apply something like "admin can set quorum down by a single unit" this is illogical and not how quorums should work.

[midori-fuse]

I'm not saying it's invalid, I'm saying it's valid but low severity.

I have shown that it can be easily mitigated without a code change, and that other sessions may still start and operate normally, without any DoS occuring. The only impact is incorrect session info and event emission, which is not medium severity.

If a session fails to reach quorum, then it is the job of the session auditor to re-validate the info, and the admin will be able to distribute rewards correctly for that session anyway.

Quoting the rules:

Incorrect values in View functions are by default considered low. Exception: In case any of these incorrect values returned by the view functions are used as a part of a larger function which would result in loss of funds then it would be a valid medium/high depending on the impact.

I have shown that the eventual incorrect state values do not lead to loss of funds, as any sessions that do not reach quorum has to be audited. Therefore no funds is lost even with the given state handling, further showing this is not medium severity.

[whoismxuse]

Once again, I have to disagree:

"I have shown that it can be easily mitigated without a code change.”

Incorrect. Your previous comment suggested that the admin can set the quorum down by a single unit. This step is illogical and does not make any sense.

Furthermore,

"and that other sessions may still start and operate normally without any DoS occurring.”

That is not true; there is an occurrence of DoS; users that should’ve met the quorum threshold will not be able to because of the design. This does not happen rarely but very frequently due to the nature of the design of the quorum.

setQuorumThreshold(_DIVIDER / 2); // 50% setMinSubmissionsRequired(2);

Furthermore, because of this, the users that should have passed the quorum will not and will fail to enter FInalizeWithParticipants and instead will enter a QuorumNotReached event.

"If a session fails to reach quorum, then it is the job of the session auditor to re-validate the info, and the admin will be able to distribute rewards correctly for that session anyway."

This means you want the auditor to re-validate the information and afterward want the admin to manually distribute the rewards correctly every time this happens. This solution seems odd to me if you put it against the simplified way of just changing > to >=

[midori-fuse]

Incorrect. Your previous comment suggested that the admin can set the quorum down by a single unit. This step is illogical and does not make any sense.

What part of this is incorrect? Here is the function to set the quorum. Setting a storage variable is not code change.

That is not true; there is an occurrence of DoS; users that should’ve met the quorum threshold will not be able to because of the design. This does not happen rarely but very frequently due to the nature of the design of the quorum.

Please provide evidence of DoS and/or supporting the claim that this happens very frequently. The current Covalent network has 15 active validators. Let us be generous and say it's 14, how is a situation in which exactly 7 out of those 14 submit the same answer "very frequently"?

• I have already provided evidence that no other sessions are disrupted by the code I already linked a few comments prior.

Furthermore, here is a counter-evidence to your "very frequently" claim. Here is the contract for the current version of the ProofChain contract. It has the same comparison issue, as well as a current quorum setting at 50%. The keccak256 hash of the QuorumNotReached(uint64,uint64) event is 0x398fd8f638a7242217f011fd0720a06747f7a85b7d28d7276684b841baea4021.

• The last 25 emissions of QuorumNotReached spanned over the course of 43 days. I'll also note that there was a burst of non-quorum today, specifically 4 such events in the last 24 hours. Note that this includes all of non-reached quorums (and not just those just about the quorum). How is a quorum ratio of exactly 50% "very frequently"?

Furthermore, because of this, the users that should have passed the quorum will not and will fail to enter FInalizeWithParticipants and instead will enter a QuorumNotReached event.

Correct. However I have already made my point that this impact is that of low severity by Sherlock rules. No data from a (un)finalized session is ever used in other sessions, and only the wrong event is emitted. Even the contest docs shows that rewards will be distributed correctly, and case 2 even implies that > was the actual intended comparison.

• Suppose we fix it to >=, then in case 2, one of the submissions will be accepted as valid, while it could've been that the other was the correct answer, and the rewards will be wrongly distributed without the admin noticing. Then doesn't it mean this would actually create more issues, even H/M level?

This means you want the auditor to re-validate the information and afterward want the admin to manually distribute the rewards correctly every time this happens. This solution seems odd to me if you put it against the simplified way of just changing > to >=

I agree it's simplier, but it does not equate to compulsory or medium/high impact. I have provided evidence that this happens very infrequently in practice (average of less than one a day).

Furthermore, it is the job of the staking manager to distribute rewards, the contract does not do it automatically. What extra job is being added here besides the (already proven to be infrequent) job of the auditor?

[whoismxuse]

I rest my case that this is a medium and should remain valid. And I have provided evidence for it. We will leave the lead judge to decide.

[nevillehuang]

I think I agree with @midori-fuse, given case 2 explicitly mentioned this is expected behavior, I think this should be low severity unless I am missing something.

@noslav why was a fix implemented, is it expected that the session should still be finalized instead of just ended, i.e. documentation error?

[Czar102]

Agree with the escalation and comments supporting it. @nevillehuang thank you for an example from the docs.

Planning to accept the escalation and invalidate the issue.

[Czar102]

Result: Invalid Has duplicates

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• midori-fuse: accepted

[sherlock-admin]

The protocol team fixed this issue in PR/commit https://github.com/covalenthq/cqt-staking/pull/125/commits/7405f5a456096337f69f7f9af89c7dc945b9fd7e.

[sherlock-admin]

The Lead Senior Watson signed off on the fix.