Deny All Humanity Claim At Challenging System/Period

[hats-bug-reporter[bot]]

Github username: -- Twitter username: johny37GR Submission hash (on-chain): 0xcdaddb03b584a667a1358a2ac865e62eb6b010f1adb3390fac8e2df5ef8f3789 Severity: high

Description: Description\ Any humanity claim request can be denied at the challenging period if a malicious actor challenges the request, and fully funds it. Also, if the reason is IdentityTheft or SybilAttack, the malicious actor can also delete the voucher's huminity ids.

The reason is, the _contribute at challengeRequest. i.e. require(_contribute(_humanityId, _requestId, challengeId, 0, Party.Challenger, arbitrationCost));

Which is called with _humanityId, _requestId, challengeId and Party.Challenger.

The _contribute mutate a round that is new, due to challengeId being unique.

If the msg.value is at least the required.amount it will write in round.sideFunded the Party.Challenger.

When the arbitrator calls rule(), no matter his _ruling the resultRuling will get overwritten by Party.Challenger because the round.sideFunded will always be Party.Challenger.

See Round storage round = challenge.rounds[challenge.lastRoundId];

Attack Scenario\

1. Legitimate user calls claimHumanity to start the process of claiming a humanity.

2. He passes the vouching system since it's a legitimate request.

3. A malicious actor calls challengeRequest on his claim request, with reason IdentityTheft or SybilAttack and fully funded, that will result in round.sideFunded = Party.Challenger

4. The arbitrator calls rule() in favor of the requester, but the Round storage round = challenge.rounds[challenge.lastRoundId]; is the round that was mutated at challengeRequest() -> _contribute() and as a result, round.sideFunder equal Party.Challenger. The ultimateChallenger will be set.

5. Malicious actor calls processVouches to delete all voucher's humanity ids.

Attachments

1. Proof of Concept

ProofOfHumanity.sol@L709-725

function claimHumanity(bytes20 _humanityId, string calldata _evidence, string calldata _name) external payable {
    Humanity storage humanity = humanityData[_humanityId];

    require(_humanityId != 0);
    require(!isHuman(msg.sender));
    require(humanity.owner == address(0x0) || humanity.expirationTime < block.timestamp);

@>  uint256 requestId = _requestHumanity(_humanityId);

    emit ClaimRequest(msg.sender, _humanityId, requestId, _name);
    emit Evidence(
        arbitratorDataHistory[arbitratorDataHistory.length - 1].arbitrator,
        uint256(keccak256(abi.encodePacked(_humanityId, requestId))),
        msg.sender,
        _evidence
    );
}

ProofOfHumanity.sol@L1406-1429

function _requestHumanity(bytes20 _humanityId) internal returns (uint256 requestId) {
    // Human must not be in the process of claiming a humanity.
    require(humanityData[accountHumanity[msg.sender]].requestCount[msg.sender] == 0);

    Humanity storage humanity = humanityData[_humanityId];

    requestId = humanity.requests.length;

    Request storage request = humanity.requests.push();
    request.requester = payable(msg.sender);

    uint256 arbitratorDataId = arbitratorDataHistory.length - 1;
    request.arbitratorDataId = uint16(arbitratorDataId);

    // Use requestCount like this in order to avoid having referencing the claimer's pending request as 0 at any point.
    humanity.requestCount[msg.sender] = requestId + 1;
    accountHumanity[msg.sender] = _humanityId;

    ArbitratorData memory arbitratorData = arbitratorDataHistory[arbitratorDataId];
    uint256 totalCost = arbitratorData.arbitrator.arbitrationCost(arbitratorData.arbitratorExtraData).addCap(
        requestBaseDeposit
    );
@>  _contribute(_humanityId, requestId, 0, 0, Party.Requester, totalCost);
}

ProofOfHumanity.sol@L1440-1475

function _contribute(
    bytes20 _humanityId,
    uint256 _requestId,
    uint256 _challengeId,
    uint256 _roundId,
    Party _side,
    uint256 _totalRequired
) internal returns (bool paidInFull) {
@>  Round storage round = humanityData[_humanityId].requests[_requestId].challenges[_challengeId].rounds[_roundId];

    uint256 remainingETH;
    uint256 contribution = msg.value;
    uint256 requiredAmount = _totalRequired.subCap(
        _side == Party.Requester ? round.paidFees.forRequester : round.paidFees.forChallenger
    );
@>  if (requiredAmount <= msg.value) {
        contribution = requiredAmount;
        remainingETH = msg.value - requiredAmount;

        paidInFull = true;
@>      round.sideFunded = round.sideFunded == Party.None ? _side : Party.None;
    }

    if (_side == Party.Requester) {
        round.contributions[msg.sender].forRequester += contribution;
        round.paidFees.forRequester += contribution;
    } else {
        round.contributions[msg.sender].forChallenger += contribution;
        round.paidFees.forChallenger += contribution;
    }
    round.feeRewards += contribution;

    emit Contribution(_humanityId, _requestId, _challengeId, _roundId, msg.sender, contribution, _side);

    if (remainingETH != 0) payable(msg.sender).safeSend(remainingETH, wNative);
}

ProofOfHumanity.sol@L1005-1071

function challengeRequest(
    bytes20 _humanityId,
    uint256 _requestId,
    Reason _reason,
    string calldata _evidence
) external payable {
    Request storage request = humanityData[_humanityId].requests[_requestId];
    // If request is for revocation request reason must be None, otherwise must be not None
    require(request.revocation == (_reason == Reason.None));
    require(request.status == Status.Resolving);
    require(block.timestamp < request.challengePeriodStart + challengePeriodDuration);

    // Only check used reasons on claim requests
    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;
    }

@>  uint256 challengeId = request.lastChallengeId++;
    Challenge storage challenge = request.challenges[challengeId];
    Round storage round = challenge.rounds[0];

    ArbitratorData memory arbitratorData = arbitratorDataHistory[request.arbitratorDataId];
    uint256 arbitrationCost = arbitratorData.arbitrator.arbitrationCost(arbitratorData.arbitratorExtraData);

@>  require(_contribute(_humanityId, _requestId, challengeId, 0, Party.Challenger, arbitrationCost));

    // Subtract the costs from the total of staked contributions.
    round.feeRewards = round.feeRewards.subCap(arbitrationCost);

    uint256 disputeId = arbitratorData.arbitrator.createDispute{value: arbitrationCost}(
        RULING_OPTIONS,
        arbitratorData.arbitratorExtraData
    );
    challenge.disputeId = disputeId;
    challenge.challenger = payable(msg.sender);
@>  challenge.lastRoundId++;

    DisputeData storage disputeData = disputeIdToData[address(arbitratorData.arbitrator)][disputeId];
    disputeData.humanityId = _humanityId;
    disputeData.requestId = uint96(_requestId);
    disputeData.challengeId = uint96(challengeId);

    request.status = Status.Disputed;

    // Hash evidenceGroupId to make sure it's unique.
    uint256 evidenceGroupId = uint256(keccak256(abi.encodePacked(_humanityId, _requestId)));

    emit RequestChallenged(_humanityId, _requestId, challengeId, _reason, disputeId);
    emit Dispute(
        arbitratorData.arbitrator,
        disputeId,
        2 * arbitratorData.metaEvidenceUpdates + (request.revocation ? 1 : 0),
        evidenceGroupId
    );
    emit Evidence(arbitratorData.arbitrator, evidenceGroupId, msg.sender, _evidence);
}

ProofOfHumanity.sol@L1315-1377

function rule(uint256 _disputeId, uint256 _ruling) external override {
    DisputeData memory disputeData = disputeIdToData[msg.sender][_disputeId];
    Humanity storage humanity = humanityData[disputeData.humanityId];
    Request storage request = humanity.requests[disputeData.requestId];
    Challenge storage challenge = request.challenges[disputeData.challengeId];
    Round storage round = challenge.rounds[challenge.lastRoundId];

    require(address(arbitratorDataHistory[request.arbitratorDataId].arbitrator) == msg.sender);
    require(request.status == Status.Disputed);

    Party resultRuling = Party(_ruling);
    // The ruling is inverted if the loser paid its fees.
    if (round.sideFunded == Party.Requester)
        // If one side paid its fees, the ruling is in its favor. Note that 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;

    // Store the rulings of each dispute for correct distribution of rewards.
    challenge.ruling = resultRuling;

    emit Ruling(IArbitrator(msg.sender), _disputeId, uint256(resultRuling));

    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);
    } else {
        // For a claim request there can be more than one dispute.
        if (resultRuling == Party.Requester) {
            if (!request.punishedVouch) {
                // All reasons being used means the request can't be challenged again, so we can update its status.
                if (request.usedReasons == FULL_REASONS_SET) {
                    humanity.owner = request.requester;
                    humanity.expirationTime = uint40(block.timestamp).addCap40(humanityLifespan);

                    emit HumanityClaimed(disputeData.humanityId, disputeData.requestId);
                } else {
                    // Refresh the state of the request so it can be challenged again.
                    request.status = Status.Resolving;
                    request.challengePeriodStart = uint40(block.timestamp);
                    request.currentReason = Reason.None;

                    emit ChallengePeriodRestart(
                        disputeData.humanityId,
                        disputeData.requestId,
                        disputeData.challengeId
                    );

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

    humanity.nbPendingRequests--;
    request.status = Status.Resolved;
    delete humanity.requestCount[request.requester];
}

2. Revised Code Make the necessary changes in order to not take into account only the challenger's round.

[clesaege]

In order to challenge a request, this request should be resolving. In order to be resolving, this request should have the requester side funded. Therefore, when a challenge is created, the funded side prior to the call will be the requester (and not "none") and due to this line after the call, the side funded of "round 0" will become None.

Moreover, a new round will be created. And the rule function will check the last round (so round 1 in your example, not round 0).

The rule 0 serves to contain the deposits of the requester and challenger. The other rounds serves to contain the appeal deposits. At no point the side funded of round 0 can impact a rule. The rule function just checks that we are not in a case of having only one side provide the appeal deposit.

[Giannis443]

Hey @clesaege, thank you for your reply.

In order to challenge a request, this request should be resolving.

I agree with this.

In order to be resolving, this request should have the requester side funded.

I agree with this, as well.

However, your third point is wrong. In advanceState() function, the following require(request.challenges[0].rounds[0].sideFunded == Party.Requester); is using challengeId which equals to 0. In challengeRequest before calling _contribute() the challengeId is incremented by 1. This will result in _contribute using the round of the same humanity ID, request ID, round ID, but different challenge ID. Which means that indeed round.sideFunded is equal Party.None and will become Party.Challenger.

[clesaege]

In challengeRequest before calling _contribute() the challengeId is incremented by 1

This isn't true, it is a postfix ++ so the value assigned to challengeId is the value before the incrementation.