In case of a tie the ruling function is inconsistent in their decision making

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0xc4a377db9a4387072674ca38cf8bedece7ea2743f90f2a1d4b5d1f353a2a51c8 Severity: medium

Description:

Description

Function rule is called by the arbitrator to give a ruling for the dispute:

        if (request.revocation) {
            humanity.pendingRevocation = false;

            if (resultRuling == Party.Requester) {
                delete humanity.owner;

                emit HumanityRevoked(disputeData.humanityId, disputeData.requestId);
            } else humanity.lastFailedRevocationTime = uint40(block.timestamp);

In the case of a recovation request, if the resultRuling =! the Party.Requester (which means it can be either the Challenger or Tie) it will fail and update the lastFailedRevocationTime. Ultimately meaning that the outcome for a Challenger or a Tie is treated the same.

However if we look at any other requests other than revocation that are != resultRuling Party.Requester we will enter the following else statement:

  // Challenger won or it’s a tie.
else if (resultRuling == Party.Challenger) request.ultimateChallenger = challenge.challenger;

This code only checks for a Challenger ruling and, if found, updates request.ultimateChallenger to challenge.challenger.

Here's where the inconsistency lies: when a challenge involves a revocation, a tie is treated as a win for the challenger. However, for other types of requests, a tie isn't even considered.

The comments mention that the challenger won or it's a tie, but in the event of a tie the else if statement is not executed. The comments suggest that in the case of a tie, the else if statement should also be executed. This would be logical since, in a revocation request, a tie is treated the same as the challenger

I also want to highlight that the Party enum includes None which is used for ties:

    enum Party {
        None, // Party per default when there is no challenger or requester. Also used for unconclusive ruling.
        Requester, // Party that made the request to change a status.
        Challenger // Party that challenged the request to change a status.
    }

This is not used anywhere in the function

Ultimately, this inconsistency can result in unfair outcomes. For example, in the case of a tie, a challenger handling a non-revocation request won't win, while a challenger in a revocation request gets the same outcome in a tie.

Recommendation

I am unsure of what the logic is here but the inconsistency between requests is not intended nor mentioned anywhere, I would assume as per the commentary and the way request.revocation is treated, the following check could be implemented:

- else if (resultRuling == Party.Challenger) request.ultimateChallenger = challenge.challenger;
+ else if (resultRuling == Party.Challenger || resultRuling == Party.None) request.ultimateChallenger = challenge.challenger;

[whoismxuse]

I also want to point out that when voting ends in a tie, it should move to the removed phase as shown in the SS. However, this doesn’t actually happen with a revocation request. In cases where resultRuling != Party.Requester, which includes both a Challenger win or a Tie, The revocation request fails and is treated the same as if the Challenger had won, as shown in the code snippet:

        if (request.revocation) {
            humanity.pendingRevocation = false;

            if (resultRuling == Party.Requester) {
                delete humanity.owner;

                emit HumanityRevoked(disputeData.humanityId, disputeData.requestId);
 =>           } else humanity.lastFailedRevocationTime = uint40(block.timestamp);

[clesaege]

This is not inconsistent, in case of tie (something which shouldn't happen, as those should be appealed), we keep the registry as is (don't add nor remove). This is a conservative approach to ties which are not desired but still technically possible.

[whoismxuse]

Hey @clesaege I am confused.

in case of tie (something which shouldn't happen, as those should be appealed), we keep the registry as is (don't add nor remove).

But the following else statement is executed for both challenger and tie results:

} else humanity.lastFailedRevocationTime = uint40(block.timestamp);

so the registry does make changes in this case and treates a challenger result the same as a tie.

Also

in case of tie (something which shouldn't happen, as those should be appealed), we keep the registry as is (don't add nor remove).

This goes against the following statement:

Which says that a tie result should move the request to the Removed Phase

[clesaege]

There is a change of the humanity.lastFailedRevocationTime because the revocation did fail in case of tie. Submission Tie: Don't add it. Removal Tie: Don't remove it.

This is consistant in that the validity stays the same.

I think your screenshot is a screenshot of the registration process: https://docs.kleros.io/products/proof-of-humanity/proof-of-humanity-tutorial

[whoismxuse]

ah my bad I see what you mean now, thanks for clarifying !