Loss of Humanity Due to Incomplete Cross-Chain Transfer During Vouching Phase

[hats-bug-reporter]

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

Description:
Description
A user who transfers their humanity while in the vouching phase cannot transfer it back to the previous chain.

The ccGrantHumanity function checks if the user’s humanity is in the process of being claimed on the receiving chain:

        // Must not be in the process of claiming a humanity.
        require(humanityData[accountHumanity[_account]].requestCount[_account] == 0);

Similarly, the ccDischargeHumanity function does not allow the transfer of humanity if the user has any pending requests:

        require(humanity.nbPendingRequests == 0);

The nbPendingRequests variable is updated when a request reaches the Resolving or Resolved state.

However, when a user claims or reclaims their humanity, the request initially enters the vouching state, where the requestCount variable is updated.

As a result, a user can only transfer their humanity if they have no ongoing requests or if their claim request has not reached the Resolving state. This means a user is allowed to transfer their humanity while the request is in the vouching state.

Problems occur when a user makes a reclaim request on Chain A and then decides to reclaim their humanity on Chain B without first withdrawing the request on Chain A. Initially, this causes no issues. However, when the user attempts to transfer their humanity back from Chain B to Chain A, their humanity will be lost on both chains due to the ongoing reclaim request on Chain A, which keeps the requestCount variable updated.

Vulnerability Scenario

1. Bob initiates a reclaim request on Chain A using renewHumanity function. His humanity enters the vouching state, where other users can vouch for his humanity. The requestCount variable for Bob’s humanity is updated to reflect the ongoing reclaim request.
2. While his reclaim request on Chain A is still in the vouching phase, Bob decides to transfer his humanity to Chain B using the transferHumanity function.
3. The ccDischargeHumanity function on Chain A checks if Bob has any pending requests. Since his request is in the vouching state, and not yet in the Resolving or Resolved state, the transfer is allowed.
4. Bob initiates a reclaim request on Chain B. The process on Chain B proceeds normally. Bob later decides to transfer his humanity back to Chain A. The ccDischargeHumanity function on Chain B checks for any pending requests.
5. However, because Bob's humanity on Chain A is still in the vouching state, with an active requestCount, the transfer is blocked.
6. Since the transfer is blocked, Bob's humanity is deleted on Chain B but not registered on Chain A due to an active request in the vouching state.
7. As a result, Bob loses his humanity on both Chain A and Chain B.

Mitigation:
Add the following lines to the ccDischargeHumanity function:

        require(humanity.nbPendingRequests == 0);
        require(!humanity.vouching);
++	require(humanityData[accountHumanity[_account]].requestCount[_account] == 0);

[aktech297]

humanity is not allowed to transfer during vouching state.
otherwise the vouched humanity can not be penalized when challenged.

[0xETHH]

duplicate #84

[0xshivay]

Hi, Issue #84 is not a duplicate of Issue #165.

Issue #84 is invalid.

A user can bypass the vouching phase and acquire a Humanity ID on two different chains by exploiting the `renewHumanity` and `transferHumanity` functions. This allows the user to transfer their Humanity ID from one chain to another without proper vouching, effectively registering the same ID on multiple chains.

According to Issue #84, once a user transfers their humanity to a new chain, they can have humanity on multiple chains simultaneously. This is incorrect. When a user transfers their humanity to a different chain, their humanity on the original chain is deleted due to the ccDischargeHumanity function. Therefore, a user cannot have more than one humanity at the same time by using the transferHumanity function.

The vulnerability, impact, and exploit described in Issue #84 are entirely different.

[clesaege]

Yeah, those are different.

Indeed, you shouldn't have a request while having a humanity on another chain. If you do this means that when this submission advance you would have an invalid submission.

Note that there is no point in preventing at the contract level the transfer of a humanity if there is a current request (made through a renewal) as the user could just claim the humanity just after the transfer anyways (and there is no way to prevent that as we can't know in real time if a user is registered on another platform).

But the frontend should not allow the user to do that (and if someone does anyways, they can only harm themselves).
I'll double check that.

[0xshivay]

@clesaege
Yes, the user could claim humanity immediately after the transfer. However, the vulnerability arises when the user transfers back to the previous chain.

Because the renewHumanity function does not require users to pay the full deposit when creating a claim or reclaim request, it's crucial to implement the fix directly at the contract level.

A user can simply call the renewHumanity function without funding the request and then choose to transfer to a different chain. Since they haven't paid any funds, there's no reason to call the withdrawRequest function to reclaim their deposit.

As a result, when the user transfers back to the previous chain, they will lose their humanity.

The fix proposed in the report is straightforward. We don't need to know if a user is registered on another platform in real time. We just need to prevent users from transferring while they have an open request.

[clesaege]

Preventing transfer while there is an open request doesn't provide any security. Because they can just make a claim just after and end up in the same situation (have a request in one chain and a humanity on the other).
As per the Kleros Coop development guidelines:

Don't protect the user from himself. Client execution is almost free, but smart contract execution isn't, so limit smart contracts to blocking malicious behavior and let clients prevent the stupid ones.

So here this is a frontend job.

[clesaege]

Answer from the frontend dev:

Does the frontend check that? (Don't allow to transfer if there is a renewal request // Don't allow to transfer if you have a request on the receiving chain)

The first case is not allowed, so the front end will not show you the button for transferring if your latter request is not a resolved claim. So if your request is in another state like vouching you won't be able to transfer
I believe that the frontend wouldn't allow to have requests on both chains simultaneously. However I'm not sure which would be its behavior if someone manages to open a claiming request in the foreign chain skipping the frontend while having a resolved claim in the homechain...

So from my understanding the frontend does prevent that. Of course, users can hurt themselves by interacting with the contract directly, but that is at their own risk.