hats-finance / Proof-Of-Humanity-V2-0xef0709445d394a22704850c772a28a863bb780b0

Proof of Humanity Protocol v2
2 stars 1 forks source link

User is able to transfer humanityId while having a request open #110

Open hats-bug-reporter[bot] opened 2 months ago

hats-bug-reporter[bot] commented 2 months ago

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

Description: A user can create a request to renew his humanity by calling renewHumanity:

    humanity.requestCount[msg.sender] = requestId + 1;

The requestCount will be incremented by 1

Now a user is still able to call transferHumanity because of the following reason:

transferHumanity will invoke ccDischargeHumanity for additional checks, one of which, is checking if there are any pending requests open by checking

    function ccDischargeHumanity(
        address _account
    ) external onlyCrossChain returns (bytes20 humanityId, uint40 expirationTime) {

     =>   require(humanity.nbPendingRequests == 0);

    }

This will not fail since renewHumanity does not increment the nbPendingRequests rather it uses a different system which is the requestCount which is not checked here.

This in itself breaks a core component of the contract as per: * - Humanity must have no pending requests.

As a result, the following scenarios can occur:

Additionally, others might deposit on his behalf and risk losing their funds if Bob decides to transfer. Either way, this situation will cause issues.

Fix

make sure to also check for the requestCount whenever transferring OR include the nbPendingRequests incrementation inside renewHumanity

whoismxuse commented 2 months ago

I forgot to add that this problem gets even worse since the humanity.owner gets deleted upon bridging, this is especially problematic if users participated in the request by contributing for or against it.

delete humanity.owner;
clesaege commented 2 months ago

The request would only be valid once it has been moved to the next phase. While it's still in a vouching phase, there is no problem to transfer the humanity. Note that if the renewal request is advanced while you are registered on the other chain, it should be challenged. See the registry rules. image

clesaege commented 2 months ago

About your second comment, it's too short to understand exactly what you mean, you may want to create a separate issue.