Malicious Vouchers Avoid Penalty

[hats-bug-reporter[bot]]

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

Description: Description
In the ProofOfHumanityExtended contract, the processVouches function is responsible for penalizing malicious vouchers. It does so by checking the voucher's humanity ownership and its expiration date:

            if (applyPenalty) {
                // Situation when the vouching address is in the middle of a renewal process.
@>                if (voucherHumanity.owner != address(0x0) && block.timestamp < voucherHumanity.expirationTime) {
                    uint256 voucherRequestId = voucherHumanity.requestCount[voucherHumanity.owner] - 1;
@>                    if (voucherRequestId != 0) voucherHumanity.requests[voucherRequestId].punishedVouch = true;

                    delete voucherHumanity.owner;

                    // If not claimed in this contract, directly remove it in the fork module.
                } else forkModule.remove(address(voucherHumanityId));

                emit HumanityDischargedDirectly(voucherHumanityId);
            }

According to this logic, if the current timestamp is beyond the humanity's expiration time, there is no need to delete the owner since the humanity has already expired. While this is logically correct, the issue arises because the punishedVouch flag is not set to true in this scenario.

If the punishedVouch variable is set to true, it prevents a penalized voucher from renewing their humanity:

Function rule:

@>                if (!request.punishedVouch) {
                    // If all reasons are used, the request can't be challenged again, so 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;
                    }
                }

Function executeRequest:

@>        } else if (!request.punishedVouch) {
            humanity.owner = request.requester;
            humanity.expirationTime = uint40(block.timestamp).addCap40(humanityLifespan);

            emit HumanityClaimed(_humanityId, _requestId);
        }

Malicious vouchers can exploit this issue by vouching right before their humanity's expiration.

The entire process of registering humanity takes considerable time to complete, especially in cases involving challenges and appeals. During this period, a malicious user can submit a renewal request via renewHumanity.

If malicious users vouch just before their humanity expires, and by the time the processVouches function is called their humanity has already expired, the punishedVouch flag will not be set to true. As a result, their renewal request can still be processed.

Attack Scenario

1. Alice, a malicious user, her humanity status is close to expiring. Bob is another malicious user, who wants to join the registry, and Alice decides to vouch for Bob's humanity.
2. Knowing that her humanity is about to expire, Alice decides to vouch for Bob's humanity request. At this point, the vouch is recorded, but Alice's own humanity is still valid.
3. Bob’s humanity request is challenged by a legitimate user, initiating a dispute process. While the whole process for Bob's humanity is ongoing, Alice sents a request to renew her humanity.
4. As Alice is considered a legitimate user, her request proceeds without any challenges.
5. After Bob's request is Resolved and challenger wins, the legitimate user calls processVouches function to punish malicious vouchers. But by this time, Alice’s own humanity status reaches its expiration time.
6. The processVouches function checks the voucher’s humanity status. Since Alice’s humanity has already expired, the condition voucherHumanity.owner != address(0x0) && block.timestamp < voucherHumanity.expirationTime evaluates to false and punishedVouch variable is not set to true.
7. Because the punishedVouch flag is not set to true, Alice avoids any penalty associated with her malicious vouch. Alice then calls executeRequest function to renew her humanity.

Mitigation
Add the following modification to the processVouches function in the ProofOfHumanityExtended contract:

            if (applyPenalty) {
++      uint256 voucherRequestId = voucherHumanity.requestCount[voucherHumanity.owner] - 1;
++                    if (voucherRequestId != 0) voucherHumanity.requests[voucherRequestId].punishedVouch = true;
                // Situation when the vouching address is in the middle of a renewal process.
                if (voucherHumanity.owner != address(0x0) && block.timestamp < voucherHumanity.expirationTime) {
--                    uint256 voucherRequestId = voucherHumanity.requestCount[voucherHumanity.owner] - 1;
--                    if (voucherRequestId != 0) voucherHumanity.requests[voucherRequestId].punishedVouch = true;

                    delete voucherHumanity.owner;

                    // If not claimed in this contract, directly remove it in the fork module.
                } else forkModule.remove(address(voucherHumanityId));

                emit HumanityDischargedDirectly(voucherHumanityId);
            }

[clesaege]

If malicious users vouch just before their humanity expires, and by the time the processVouches function is called their humanity has already expired, the punishedVouch flag will not be set to true. As a result, their renewal request can still be processed.

This is indeed the expected behaviour, you can't remove someone from a registry if he is not in the registry. The penalty for vouching for malicious users is to be removed from the registry, currently, a malicious voucher could reregister (there has been talks about asking to pay fines before being allowed in but not for now). The main point of removing vouchers of malicious submissions is that they are themselves likely to be malicious (an attacker would make profiles vouching other fake profils, vouching again other fake profiles).

[0xshivay]

Hello @clesaege ,

This is indeed the expected behaviour, you can't remove someone from a registry if he is not in the registry. The penalty for vouching for malicious users is to be removed from the registry, currently, a malicious voucher could reregister (there has been talks about asking to pay fines before being allowed in but not for now).

I agree that if a user’s humanity has already expired, there’s no need to remove them from the registry—this is expected behavior. I also agree that a malicious voucher could re-register, but that should only occur after they’ve been penalized, not while they’re still engaging in malicious vouching.

In the ProofOfHumanity contract, there isn’t a similar if condition in the processVouches function. As a result, this vulnerability can’t be exploited in that contract to avoid penalties. In that contract, malicious vouchers can only re-register after they’ve been penalized, meaning they’ve been removed from the registry. To claim their humanity again, they have to go through the entire registration process, including the vouching, challenge, and appeal phases.

The main point of removing vouchers of malicious submissions is that they are themselves likely to be malicious (an attacker would make profiles vouching other fake profiles, vouching again other fake profiles).

This isn’t always the case. A user can create a legitimate profile with valid evidence and then decide to vouch maliciously after being registered in POHv2. The user exploiting this vulnerability doesn’t need to be malicious from the start. A legitimate user whose humanity is nearing expiration could vouch maliciously and then create a reclaim request. Because the user is legitimate, they could easily pass the vouching and challenge phases. By the time the malicious vouch is penalized, the user’s humanity will be expired, preventing them from being penalized. They can then re-register in POHv2 by calling the executeRequest function, effectively avoiding the penalty.

This vulnerability allows any user who is close to their expiration time to vouch maliciously, avoid penalties, and renew their humanity. It enables the user to bypass the vouching and challenge phases while allowing the person who received the vouches to skip the vouching phase entirely.

Therefore, the contract should not permit a reclaim request until the malicious vouchers have been penalized, and the punishedVouch variable should always be set to true, even if the user’s humanity has expired.

(there has been talks about asking to pay fines before being allowed in but not for now)

If the punishedVouch variable is used as a metric to impose fines, users could exploit this vulnerability to evade those fines as well.

[clesaege]

By the time the malicious vouch is penalized, the user’s humanity will be expired, preventing them from being penalized. They can then re-register in POHv2 by calling the executeRequest function, effectively avoiding the penalty.

This is the expected behaviour. And if they are a valid submission, they could reregister even after being removed. There is no expectation that the removal of a malicious voucher would create a minimal period of non registration. Actually if they do as you state in your example, they will incur a period were they are not registered.