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

Proof of Humanity Protocol v2
2 stars 1 forks source link

A malicious user can manipulate the outcome in the event of a tie. #113

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): 0x006c1f839f986e592c808c7cca4369786d23d2b436c3550177b7bf00710fdc79 Severity: high

Description: Description:
A malicious user can manipulate the outcome in the event of a tie.

According to the protocol's rules, in the case of a tie, the registry should remain unchanged. However, a malicious user can exploit this by calling the fundAppeal function just before the appeal period ends, selecting their preferred winner. This action prevents any legitimate user from funding an appeal during the appeal period, effectively blocking them from submitting an appeal even if they wish to do so.

There are two scenarios where this can occur:

  1. A challenge is made against a request. If the voting results in a tie, a malicious user calls the fundAppeal function right before the appeal period ends.

  2. An appeal is filed against a challenge. If the voting results in a tie, a malicious user calls the fundAppeal function right before the appeal period ends.

In both scenarios, the malicious user calls the fundAppeal function during a new round.

This happens because a new round begins after a challenge or appeal is initiated:

    challenge.lastRoundId++;

As a result, the round.sideFunded variable for the new round is set to Party.None by default.

Since the malicious user only funds one side, the appeal is not created, and the round.sideFunded variable is set to reflect the side chosen by the malicious user:

    round.sideFunded = round.sideFunded == Party.None ? _side : Party.None;

After the malicious user's action, no further appeals can be submitted, and no user can call fundAppeal once the appeal period has expired. This means that the outcome of the last round is determined by the malicious user's action.

When the arbitrator calls the rule function, the winner of the dispute will always be the side chosen by the last user who called fundAppeal when appeal is not filed:

    Party resultRuling = Party(_ruling);
    // The ruling is reversed if the losing side paid its fees.
    if (round.sideFunded == Party.Requester)
        // If one side paid its fees, the ruling is in its favor. If the other side had also paid, an appeal would have been created.
        resultRuling = Party.Requester;
    else if (round.sideFunded == Party.Challenger) resultRuling = Party.Challenger;

Attack Scenario

  1. Alice challenges a request or files an appeal against a challenge. A new round begins, and the round.sideFunded variable is set to Party.None.
  2. The voting results in a tie, where the registry should remain unchanged according to the protocol's rules.
  3. Bob, a malicious user, waits until just before the appeal period ends and calls the fundAppeal function, funding only his preferred side (e.g., Party.Requester or Party.Challenger).
  4. Since no further appeals can be submitted after the appeal period ends, and no legitimate user can call fundAppeal, the outcome of the dispute is effectively determined by the malicious user's action.
  5. When the arbitrator calls the rule function, the winner is determined by the round.sideFunded value set by Bob, irrespective of the tie in voting.

Solution: Adjust the fundAppeal function to reduce the time allowed for users to call it in the event of a tie to half the original period. This will prevent malicious users from exploiting the timing to manipulate the outcome. Add the following code in the fundAppeal Function:

    Party winner = Party(IArbitrator(_arbitrator).currentRuling(_disputeId));
    if (winner == _side) multiplier = winnerStakeMultiplier;
++  else if (block.timestamp - appealPeriodStart < (appealPeriodEnd - appealPeriodStart) / 2) {
++      if (winner == Party.None) multiplier = sharedStakeMultiplier;
++      else multiplier = loserStakeMultiplier; }
--  else if (winner == Party.None) multiplier = sharedStakeMultiplier;
--  else if (block.timestamp - appealPeriodStart < (appealPeriodEnd - appealPeriodStart) / 2)
--      multiplier = loserStakeMultiplier;
        // If half of the appeal period has passed and the funded side is the winner, it will revert
    else revert();
0xshivay commented 2 months ago

I’d like to highlight another point regarding this issue. Since the last round is not fully funded, a malicious user can retrieve their appealCost by using the withdrawFeesAndRewards function.

clesaege commented 2 months ago

"None" (the ruling given in case of a tie) is never a correct ruling on PoH. So people do need to appeal and if a side doesn't it will lose. They need to do so before the deadline. As per the context rules are excluded: Issues about missing appeals (for the purpose of this review, we will assume that there is always a user providing appeal deposits in the case of a wrong provisional ruling).