V1 Profiles Can Avoid Penalties Using `transferHumanity`

[hats-bug-reporter[bot]]

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

Description: Description
When a V1 profile vouches on the ProofOfHumanityExtended contract, their humanity status is updated by setting the voucherHumanity.vouching variable to true. This change prevents the V1 voucher from vouching again:

                bytes20 voucherHumanityId = humanityOf(voucherAccount);
                voucherHumanity = humanityData[voucherHumanityId];
                if (
                    ((voucherHumanity.owner == voucherAccount && block.timestamp < voucherHumanity.expirationTime) ||
                        forkModule.isRegistered(voucherAccount)) &&
                    !voucherHumanity.vouching &&
                    voucherAccount != _claimer
                ) {
                    request.vouches.push(voucherHumanityId);
@>                    voucherHumanity.vouching = true;

                    // Emit event to signal the processing of the signature vouch (and the on-chain as well)
                    emit VouchRegistered(voucherHumanityId, humanityId, requestId);
                }

If a V1 profile transfers their humanity to a new chain via the transferHumanity function of the CrossChainProofOfHumanity contract, without first renewing their humanity on POHv2, the ProofOfHumanityExtended::ccDischargeHumanity function is called. This function performs the following check:

        if (humanity.owner == _account && block.timestamp < humanity.expirationTime) {
@>            require(!humanity.vouching);

            expirationTime = humanity.expirationTime;

            delete humanity.owner;
        } else {
            // V1 profiles have default humanity.
            humanityId = bytes20(_account);

            // Should revert in case the account is not registered.
@>            expirationTime = forkModule.tryRemove(_account);
        }

Here, for V2 profiles, the function reverts if the humanity is still vouching. However, for V1 profiles that haven't renewed their humanity on POHv2, the humanity variable on V2 is mostly empty with default values, except for humanity.vouching being set to true due to vouching. The function only checks if the V1 profile is registered using forkModule.tryRemove(_account); and does not verify the vouching status on V2.

A malicious V1 voucher can exploit this issue by transferring their humanity to a new chain while they are vouching, thereby avoiding the penalty of not being able to renew their humanity on POHv2.

Mitigation
Add the following check to the ProofOfHumanityExtended::ccDischargeHumanity function:

        } else {
            // V1 profiles have default humanity.
            humanityId = bytes20(_account);

++            require(!humanityData[humanityId].vouching);

        // Should revert in case the account is not registered.
            expirationTime = forkModule.tryRemove(_account);
        }

[0xmahdirostami]

Duplicate #65

[0xshivay]

Hi, Issue #129 is not a duplicate of Issue #65.

Issue #65 is incorrect.

In summary, Issue #65 claims that malicious users can bypass the vouching phase, which is not accurate.

The `ProofOfHumanityExtended::ccDischargeHumanity` function currently only checks for the vouching phase in v2 humanity profiles, allowing v1 profile users to exploit the system. By combining the `transferHumanity` and `renewHumanity` functions, a malicious user could bypass the vouching phase and register the same humanity ID on two different chains.

The check is specifically for vouchers, not for users currently in the vouching stage. Malicious users cannot bypass the vouching phase due to a missing check; only malicious vouchers can bypass the penalty.

As the function does not check vouching status for v1 profiles, the user successfully transfers humanity ID X to another chain.

The exploit scenario described is also incorrect. The vouching status applies to the vouchers vouching for the user, not the user in the vouching stage. A user in the vouching stage will not have the vouching variable set to true; rather, it is the vouchers' vouching variable that is set to true. You can verify this here: ProofOfHumanity.sol#L968.

There is no need for the ccDischargeHumanity function to check the vouching status of a user in the vouching stage.

The impact is entirely independent of the vulnerability mentioned in Issue #65.

The recommended fix will not resolve the issue, as the problem is not a missing vouching phase check for a user in the vouching stage, but rather a missing check for the vouching variable for vouchers.

Additionally, the POC you mentioned in the comments of Issue #65 is related to a completely different vulnerability with a different root cause, which is not outlined in your issue.

[clesaege]

I'll need to double check, but this does look valid.

This is not medium (and even less high). Medium: Issues that can lead to improper profile creation/removal but only at small scale or to steal funds. High: Issues that allows a takeover of the registry

Here it would allow to prevent ones removal from the registry while vouching for a malicious submission, which shouldn't happen, but does not lead to malicious profiles getting in or good profiles being removed (it does however manage to vouch on two chains at the same time).

[clesaege]

The fix should be to but the require outside of the if. Note that we already changed the humanityId to humanityOf(_account); due to another issue.

    function ccDischargeHumanity(
        address _account
    ) external onlyCrossChain returns (bytes20 humanityId, uint40 expirationTime) {
        humanityId = humanityOf(_account);
        Humanity storage humanity = humanityData[humanityId];
        require(humanity.nbPendingRequests == 0);
        require(!humanity.vouching);

        if (humanity.owner == _account && block.timestamp < humanity.expirationTime) {
            expirationTime = humanity.expirationTime;

            delete humanity.owner;
        } else {
            // Should revert in case account is not registered.
            expirationTime = forkModule.tryRemove(_account);
        }

        emit HumanityDischargedDirectly(humanityId);
    }