Flawed ruling logic in function `rule`

[hats-bug-reporter[bot]]

Github username: @Vancelott Twitter username: vancelotx Submission hash (on-chain): 0xbc875b1c59f7eba5220e04126d548b73877390f8d15903bb935b5c274af29c37 Severity: medium

Description: Description

One of the protocol's main concepts is that users can dispute the validity of a humanity. Whenever the validity of a profile is disputed, an arbitrator examines the evidence of both parties - the challenger and the owner of a humanity - to determine who is correct.

Using the rule function, the arbitrator can confirm their decision in regards to a dispute, which handles the internal logic for both the winning and losing side. The end decision of the arbitrator is stored in the challenge struct:

    struct Challenge {
        uint16 lastRoundId; // Id of the last round.
@>   Party ruling; // Ruling given by the arbitrator of the dispute.
        address payable challenger; // Address that challenged the request.
        uint256 disputeId; // Id of the dispute related to the challenge.
        mapping(uint256 => Round) rounds; // Tracks the info of each funding round of the challenge.
    }

Which then gets set to the variable ruling. All of this pre-context will demonstrate that the current implementation is flawed, as the user loses ownership of their humanity even after winning.

...
        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);
...

For the second if statement to be executed, the Party.Requester (the owner of the humanity) has to be ruled the winner of the dispute. At the end, even though they've won and defended their humanity, their ownership gets revoked.

Attack Scenario

This issue can lead to users losing their humanity unfairly, or also keeping their humanity unfairly, when they should have lost it.

Recommendation

Swap the logic in the if/else statement as follows:

...
if (resultRuling == Party.Requester) {
+              humanity.lastFailedRevocationTime = uint40(block.timestamp);
+             } else {
+               delete humanity.owner;
+
+               emit HumanityRevoked(disputeData.humanityId, disputeData.requestId);
+             }
...

[Vancelott]

Small typo - the correct variable name is resultRuling and not ruling in the sentence: Which then gets set to the variable ruling.

[clesaege]

What you are looking at is what happens if it is a revocation request (if (request.revocation) { ... }). So in a revocation request, the requester asks to remove a humanity. If the requester wins, this humanity will be removed. I think you didn't notice that this part of the code is for revocation requests.