User is able to bypass the requirements inside revokeHumanity

[hats-bug-reporter[bot]]

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

Description: A user is able to revoke a humanity by calling revokeHumanity:

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

        require(
            (humanity.owner != address(0x0) && block.timestamp < humanity.expirationTime) ||
                // If not claimed on this contract check on V1.
                forkModule.isRegistered(address(_humanityId))
        );
        require(!humanity.pendingRevocation);
        require(humanity.lastFailedRevocationTime.addCap40(failedRevocationCooldown) < block.timestamp);

Inside a check is performed to see if the _humanityId exists on the V1 if not on V2, if so it will perform some additional checks in the forkModule:

    function isRegistered(address _submissionID) external view override returns (bool) {
        if (removed[_submissionID]) return false;

        (, uint64 submissionTime, , bool registered, , ) = proofOfHumanityV1.getSubmissionInfo(_submissionID);

        uint40 expirationTime = uint40(submissionTime).addCap40(submissionDuration);

        return registered && block.timestamp < expirationTime && submissionTime < forkTime;
    }

It will retrieve the submisstionTime and registered from the V1 version and will do some time checks and eventually return either true or false.

After that the following 2 requirements will be made inside revokeHumanity:

require(!humanity.pendingRevocation);       require(humanity.lastFailedRevocationTime.addCap40(failedRevocationCooldown) < block.timestamp);

The problem is however that if we are dealing with a V1 profile, these checks will not perform the check towards V1 storage but towards V2 storage. Unlike the aforementioned checks such as registered / submissionTime which were checked upon proofOfHumanityV1.

This means the following:

• Bob is able to perform revokeHumanity upon a V1 humanityId while that humanityId might still have processing revocations AND/OR the lastfailedRevocation has not been passed on V1, but Bob is able to circumvent this since it will check the storage from V2. This can cause issues depending on the processing revocations and simply allows for circumventing these requirements

Recommendation

Introduce a function inside ForkModule that checks the pendingRevocation && lastFailedRevocationTime from proofOfHumanityV1. Just as isRegistered retrieves information from V1

[whoismxuse]

Quick side note; this should be a valid medium as per the rule set:

Issues allowing to remove a profile in an illegitimate manner in a particular situation (ex: prevent the renewal of a profile).

this issue allows to revoke a humanity in an illegitimate manner since that humanity might still have pending revocations or the lastFailedRevocation on V1 has not passed yet

[clesaege]

The cooldown between revocations is there on V2 to prevent malicious actors from making continuous revocation requests in order to prevent transfer, there is no reason for it on V1. Moreover, as long as the fork in not fully finalized, the rules of V1 could potentially be changed (and this in a way conflicting with V2 devs as even if the fork was voted, there has been some climate of conflict and malicious V1 proposals are not to be excluded), so we want to always allow revocation on V2 irrespective of the rules of V1.

So for a humanity made on V1 to be valid it should be:

• Valid on V1.
• Not revoked on V2.

The behaviour you described is the expected behaviour.

[whoismxuse]

hi @clesaege

I agree with u regarding the revocation cooldown check, however:

where does it state that even if there are any pending requests on V1 revokeHumanity should be allowed to be called on a V1 profile ?

I think you are conflicting forking from V1 > V2 whereas I am talking about revoking a humanity inside V1 while having pendingrequests open that are not checked in the revokeHumanity function.

It does not state anywhere and in fact the contrary, that revokeHumanity should never be called whenever a pending request is open, I showed in my finding that it is possible to revoke a humanity on V1 with pending requests open

[whoismxuse]

Also, I want to add that according to your logic every bug that includes V1 should be invalid since

the rules of V1 could be changed

However, you have validated issues that include V1 logic. It is not fair to measure with 2 standards, therefore I would like to propose you to validate this issue

[0xshivay]

Hello @whoismxuse ,

I would like to clarify the misunderstanding regarding POHv1 and POHv2.

Currently, there are two validated issues concerning v1 profiles: Issue #129 and Issue #134.

In both issues, v1 profiles interact with POHv2 to perform malicious activities on POHv2, not on POHv1.

In Issue #129, v1 profiles interact with the POHv2 ProofOfHumanityExtended contract to avoid penalties on v2.

In Issue #134, there are two vulnerabilities: "Avoiding Revocation Requests on POHv1" and "Avoiding Vouch Penalties on POHv1," where v1 profiles can perform malicious actions on POHv1. However, these vulnerabilities are not considered by @clesaege .

Issue #134 was validated due to the vulnerability "Avoiding Revocation Requests on POHv2," where v1 profiles interact with POHv2 to avoid Revocation Requests that were created on v2, not on v1.

In both issues, the final impact of the vulnerabilities is on POHv2.

[whoismxuse]

Hi @0xshivay Thank you for commenting,

This issue has not been invalidated because of the fact that the impact lies on V1, instead, the sponsor invalidated it since rules on V1 can still change. This is unfair since any issue that includes V1 logic to circumvent X could then hypothetically be incorrect since the rules might still change.

I believe those issues are still valid though, as should this one be.

[0xshivay]

@whoismxuse As I understand, when the sponsor mentioned that "the rules of v1 can change," he was referring to the criteria that define what constitutes a valid profile, not the actual v1 code logic. The v1 code logic itself cannot be changed.

What can change is the definition of a valid profile and the type of evidence required to support it. In other words, the types of profiles that are allowed to pass the challenge phase and register on POHv1 may change before the fork.

As far as I understand, the v1 logic has no role in the invalidation of your issue.

[whoismxuse]

@0xshivay I understand what you mean, the issue still persists that a user is able to revoke a humanity inside V1 while it may have pending requests open, this should not be allowed as per revokeHumanity the check is made:

require(!humanity.pendingRevocation);

this fails to check the user inside V1 whenever dealing with a V1 humanityId

Thank you for giving your point of view though, much appreciated.

[whoismxuse]

@clesaege this issue has been rewritten in #154, feel free to read my comments in here for additional context

[clesaege]

@0xshivay I understand what you mean, the issue still persists that a user is able to revoke a humanity inside V1 while it may have pending requests open, this should not be allowed as per revokeHumanity the check is made:

require(!humanity.pendingRevocation);

this fails to check the user inside V1 whenever dealing with a V1 humanityId

We prevent to have multiple revocation requests at the same time to:

• Prevent a constant flow of revocation requests which would block the transfer.
• Simplify the contract logic by being able to assume that there is only one revocation request at a time (in this contract).

Here a revocation request on both V1 ans V2 doesn't cause any of those problems.

[whoismxuse]

Hi @clesaege please read issue #154, it clearly shows that the logic of both V1 and V2 reflect that revoking a humanity/submission should not be possible if there are any pending requests or the status is anything other than None.

This has nothing to do with having multiple revocation requests, it has to do with V1 users getting revoked while they are in the midst of another process, may it be a renewal, removal, etc.

Nowhere does it state that this should be allowed under any circumstances