HumanityId Can Be Overwritten By Another Account

[hats-bug-reporter[bot]]

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

Description: Description\ It is possible to overwrite a humanity in specific circumstances, more specifically, a user has to request to claim humanity by calling claimHumanity(). His request has to go through the vouching system, and not be challenged, which makes him eligible to executeRequest() and claim humanity.

Another user that has the same humanity id in another chain (this isn't checked at code level as mentioned in issue #51), transfers his humanity to the same chain. The user that transferred his humanity, claims the humanity without issues.

The user that is eligible to call executeRequest(), calls it and claims (by overwritting) the humanity id.

Attack Scenario\ It is not an attack, it's a scenario which holds true under specific circumstances as mentioned in the Severity Guidelines.

Although here is the scenario that has to take place:

Let's say Alice is on chain B, and wants to claim humanity with id X. Bob is on chain A, and has the humanity with id X.

1. Alice calls claimHumanity() with id X.

2. Alice's request goes through the vouching system and challenging system.

3. Bob calls transferHumanity() to transfer it to chain B.

4. receiveTransfer() gets called, and Bob claims the humanity with id X on chain B.

5. Alice calls executeRequest() and claims humanity with id X by overwritting Bob as owner.

Attachments

1. Proof of Concept

CrossChainProofOfHumanity.sol@L332-363

function receiveTransfer(
    address _owner,
    bytes20 _humanityId,
    uint40 _expirationTime,
    bytes32 _transferHash
) external override allowedGateway(msg.sender) {
    // Once transfer hash is flagged as received it is not possible to receive the transfer again
    require(!receivedTransferHashes[_transferHash]);

    // If humanity is claimed on the main contract it will return false and not override the state
    // Otherwise requires _owner to not be in process of claiming a humanity
@>  bool success = proofOfHumanity.ccGrantHumanity(_humanityId, _owner, _expirationTime);

    CrossChainHumanity storage humanity = humanityData[_humanityId];

    // Clear human humanityId for past owner
    delete accountHumanity[humanity.owner];

    // Overriding this data in case it is outdated
    if (success) {
        accountHumanity[_owner] = _humanityId;

        humanity.owner = _owner;
        humanity.expirationTime = _expirationTime;
        humanity.isHomeChain = true;
        humanity.lastTransferTime = uint40(block.timestamp);
    }

    receivedTransferHashes[_transferHash] = true;

    emit TransferReceived(_humanityId, _owner, _expirationTime, _transferHash);
}

ProofOfHumanityExtended.sol@L503-527

function ccGrantHumanity(
    bytes20 _humanityId,
    address _account,
    uint40 _expirationTime
) external onlyCrossChain returns (bool success) {
    Humanity storage humanity = humanityData[_humanityId];

    // If humanity is claimed, don't overwrite.
    if (
@>      (humanity.owner != address(0x0) && block.timestamp < humanity.expirationTime) ||
        // If not claimed in this contract, check in fork module too.
        forkModule.isRegistered(_account)
    ) return false;

    // Must not be in the process of claiming a humanity.
    // @audit _account in this case is Bob's address, so the requestCount is 0
@>  require(humanityData[accountHumanity[_account]].requestCount[_account] == 0);

@>  humanity.owner = _account;
@>  humanity.expirationTime = _expirationTime;
@>  accountHumanity[_account] = _humanityId;

    emit HumanityGrantedDirectly(_humanityId, _account, _expirationTime);

    return true;
}

ProofOfHumanityExtended.sol@L1186-1215

function executeRequest(bytes20 _humanityId, uint256 _requestId) external {
    Humanity storage humanity = humanityData[_humanityId];
    Request storage request = humanity.requests[_requestId];
    require(request.status == Status.Resolving);
    require(request.challengePeriodStart + challengePeriodDuration < block.timestamp);

    if (request.revocation) {
        if (humanity.owner != address(0x0) && block.timestamp < humanity.expirationTime) {
            delete humanity.owner;
            humanity.pendingRevocation = false;

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

        emit HumanityRevoked(_humanityId, _requestId);
    // @audit Alice's request does not have punished vouch
@>  } else if (!request.punishedVouch) {
        //@audit here, humanity.owner gets overwritten by Alice's account
@>      humanity.owner = request.requester;
        humanity.expirationTime = uint40(block.timestamp).addCap40(humanityLifespan);

        emit HumanityClaimed(_humanityId, _requestId);
    }

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

    if (request.vouches.length != 0) processVouches(_humanityId, _requestId, VOUCHES_TO_AUTOPROCESS);

    withdrawFeesAndRewards(request.requester, _humanityId, _requestId, 0, 0); // Automatically withdraw for the requester.
}

2. Revised Code

The recommendation would be to add a way to check if a claimHumanity request of the humanity id that will be transferred, is at Status.Resolving.

[clesaege]

"Another user that has the same humanity id in another chain" that would mean one of them had done an illegal registration "Let's say Alice is on chain B, and wants to claim humanity with id X. Bob is on chain A, and has the humanity with id X." the humanity can either belong to Alice, or to Bob, but can't belong to both at the same time.

As per Proof of Humanity Registry Policy: The default humanity being the bytes20 of the address used.

It is indeed technically possible to register with a wrong Humanity ID, but challengers should challenge any of such submissions.

Per the contest rules, are excluded: Issues about challengers missing some invalid profile submissions/removals (for the purpose of this review, we will assume challengers are perfect and omniscient and that they always challenge invalid actions before the deadline).

[Giannis443]

Hey @clesaege, regarding the PoH Registry Policy, as per your comment in #51:

"it appears that the registration rules themselves do not define what is the default Humanity ID."

So, as a result, there is no default humanity ID for an address. Regarding the rules, this is not an invalid submission, since the user is legitimate and tries to claim an unclaimed humanity ID.

[clesaege]

The rules themselves do not define what is the default Humanity ID, but the code does. So Kleros would rule against those submissions (if in a contract you use terms that you do not define, jurors will have to find out what they mean, here that would be by looking at the code to understand what "default Humanity ID" means).

If your point is that the policy isn't very good as it doesn't define "default Humanity ID" which could confuse non technical users, this would be a duplicate of https://github.com/hats-finance/Proof-Of-Humanity-V2-0xef0709445d394a22704850c772a28a863bb780b0/issues/51.

Per the contest rules, are excluded:

• Issues about challengers missing some invalid profile submissions/removals (for the purpose of this review, we will assume challengers are perfect and omniscient and that they always challenge invalid actions before the deadline).
• Issues mentioned in any previous audit reports.