hats-finance / Proof-Of-Humanity-V2-0xef0709445d394a22704850c772a28a863bb780b0

Proof of Humanity Protocol v2
2 stars 1 forks source link

Malicious Vouchers Can Dodge Penalties by Manipulating Challenge Reasons #103

Open hats-bug-reporter[bot] opened 2 months ago

hats-bug-reporter[bot] commented 2 months ago

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

Description: Description:
When a user calls the challengeRequest function to dispute a claim, they must provide a reason for their challenge, which can be one of four specified reasons. This function sets the provided reason as the current reason for the claim request:

request.currentReason = _reason;

Later, when the processVouches function is invoked to penalize malicious vouchers, it checks the current reason of the challenge:

bool applyPenalty = request.ultimateChallenger != address(0x0) &&
    (currentReason == Reason.SybilAttack || currentReason == Reason.IdentityTheft);

According to this logic, penalties are only applied if the current reason is either SybilAttack or IdentityTheft.

However, any user can call the challengeRequest function to initiate a new challenge on the same claim request before the challenge period ends, using any of the four available reasons.

This creates an opportunity for a malicious voucher to avoid penalties by front-running the rule call with a new challengeRequest using either of the two non-penalizable reasons—IncorrectSubmission or Deceased. By doing this, the malicious voucher changes the currentReason to one of these non-penalizable reasons.

Consequently, when the rule function is executed and followed by the processVouches function, no penalties are applied since the current reason does not meet the criteria of SybilAttack or IdentityTheft.

Attack Scenario:

  1. Bob, a legitimate user, challenges a humanity claim by Alice (a malicious actor) using the challengeRequest function with a reason that triggers penalties for malicious vouchers, such as SybilAttack or IdentityTheft. This sets the currentReason of the claim request to SybilAttack or IdentityTheft.
  2. Other malicious vouchers recognizes the threat of penalties if Bob’s challenge succeeds and decides to manipulate the challenge process to avoid these penalties.
  3. Before the challenge period ends, any other malicious actor submits a new challenge using the challengeRequest function but with a different reason: either IncorrectSubmission or Deceased. This changes the currentReason for the humanity claim to the new reason.
  4. The rule function is eventually called to finalize the dispute. When processVouches function is called, which is responsible for penalizing malicious vouchers, it references the currentReason of the claim request.
  5. Since malicious actor has manipulated the currentReason to be IncorrectSubmission or Deceased, the condition to apply penalties is not met, and no penalties are enforced against the malicious vouchers.

Mitigation:
Only 1 challenge should be allowed to process at one time.

0xshivay commented 2 months ago

I’d like to highlight another point regarding this issue. Even if all four challenge reasons are already present for a request, the challengeRequest function allows a user to input a reason of 0 or None:

if (!request.revocation) {
    // Get the bit that corresponds with reason's index.
    uint8 reasonBit;
    unchecked {
        reasonBit = uint8(1 << (uint256(_reason) - 1));
    }

    require((reasonBit & ~request.usedReasons) == reasonBit);

    // Mark the bit corresponding with reason's index as 'true', to indicate that the reason was used.
    request.usedReasons ^= reasonBit;

    request.currentReason = _reason;
}

This means that even if a request has already been challenged with all four possible reasons, a malicious user can still call the challengeRequest function to set the currentReason variable to 0 or None. This makes it always possible for malicious users to exploit this issue and vouch maliciously. In other words, a malicious user can bypass the voucher system.

clesaege commented 2 months ago

A challenge puts a request in the Status.Disputed state. And when challenging, it checks Status.Resolving. So it is already mandated there can't be more than 1 challenge at once.

Note that if there are multiple reasons for a challenge, an aggravated one (i.e. one leading to removal of challengers) should be chosen per the registry rules. image

clesaege commented 2 months ago

I’d like to highlight another point regarding this issue. Even if all four challenge reasons are already present for a request, the challengeRequest function allows a user to input a reason of 0 or None:

if (!request.revocation) {
    // Get the bit that corresponds with reason's index.
    uint8 reasonBit;
    unchecked {
        reasonBit = uint8(1 << (uint256(_reason) - 1));
    }

    require((reasonBit & ~request.usedReasons) == reasonBit);

    // Mark the bit corresponding with reason's index as 'true', to indicate that the reason was used.
    request.usedReasons ^= reasonBit;

    request.currentReason = _reason;
}

This means that even if a request has already been challenged with all four possible reasons, a malicious user can still call the challengeRequest function to set the currentReason variable to 0 or None. This makes it always possible for malicious users to exploit this issue and vouch maliciously. In other words, a malicious user can bypass the voucher system.

This line prevents it.