Transferring of Humanity Can Be Griefed

[hats-bug-reporter[bot]]

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

Description: Description\ When a user wants to transfer his humanity to another chain, he has to call the transferHumanity() function of CrossChainProofOfHumanity.sol. transferHumanity() calls ccDischargeHumanity(msg.sender) in order to discharge the humanity directly from the user. However, a malicious actor can front-run the transferHumanity() transaction, and call revokeHumanity() with the user's _humanityId in order to prevent him from transferring, by causing his transaction to revert due to nbPendingRequests != 0. The user won't be able to call transferHumanity() until the pendingRevocation gets resolved. After it gets resolved, the user has a time window equal to failedRevocationCooldown to transfer his humanity, if he fails to do so, his next transferHumanity() will also be front-runnable.

Attack Scenario\ Alice is a legitimate user with a humanity that wants to transfer to another chain. Bob is a malicious actor that wants to stop Alice from doing so.

1. Alice calls transferHumanity()

2. Bob calls revokeHumanity() with Alice's _humanityId and front-runs Alice.

3. Alice's call gets reverted.

4. Alice has to wait until the pendingRevocation gets resolved.

5. After it gets resolved, Alice has a window of failedRevocationCooldown to transfer her humanity.

6. If he fails to do so, Bob can start over and deny her transferring her humanity to another chain.

Attachments

1. Proof of Concept

CrossChainProofOfHumanity.sol@L250-289

function transferHumanity(address _bridgeGateway) external allowedGateway(_bridgeGateway) {
    // Function will require humanity to be claimed by sender, have no pending requests and human not vouching for others at the time
@>  (bytes20 humanityId, uint40 expirationTime) = proofOfHumanity.ccDischargeHumanity(msg.sender);
    ...
}

ProofOfHumanityExtended.sol@L544-566

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

    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 account is not registered.
        expirationTime = forkModule.tryRemove(_account);
    }

    emit HumanityDischargedDirectly(humanityId);
}

ProofOfHumanityExtended.sol@L801-840

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);

    uint256 requestId = humanity.requests.length;

    Request storage request = humanity.requests.push();
    request.status = Status.Resolving;
    request.revocation = true;
    request.requester = payable(msg.sender);
    request.challengePeriodStart = uint40(block.timestamp);

    uint256 arbitratorDataId = arbitratorDataHistory.length - 1;
    request.arbitratorDataId = uint16(arbitratorDataId);

    humanity.pendingRevocation = true;
@>  humanity.nbPendingRequests++;

    ArbitratorData memory arbitratorData = arbitratorDataHistory[arbitratorDataId];
    uint256 totalCost = arbitratorData.arbitrator.arbitrationCost(arbitratorData.arbitratorExtraData).addCap(
        requestBaseDeposit
    );

    require(_contribute(_humanityId, requestId, 0, 0, Party.Requester, totalCost));

    emit RevocationRequest(msg.sender, _humanityId, requestId);
    emit Evidence(
        arbitratorData.arbitrator,
        uint256(keccak256(abi.encodePacked(_humanityId, requestId))),
        msg.sender,
        _evidence
    );
}

2. Revised Code

The change requires multiple changes to the code, an idea would be to change where the nbPendingRequests are incremented, for example, they could be incremented in challengeRequest instead.

[clesaege]

It is indeed possible to call revokeHumanity to prevent its transfer. Note that if the humanity was valid, the actor preventing the transfer will lose its deposit. As you pointed it out in the report, in order to prevent a malicious actor from continuously preventing the transfer of a humanity (even if it is at the cost of continuously losing deposits), we have a failed revocation cooldown making sure that the owner of the humanity can transfer it after a unique invalid revocation request. Therefore the issue you are reporting has already been mitigated.

[Giannis443]

Hey @clesaege, I understand the attacker loses their deposit in a failed revocation attempt, but that’s precisely why this is a griefing attack. The attacker doesn’t care about losing the deposit. They just want to disrupt the user. The failedRevocationCooldown doesn’t prevent the griefing attack because the user can still be blocked when trying to transfer their humanity. Furthermore, a griefing attack is a scenario that can occur at least once. If this were continuously replicable, it would be closer to a DoS attack that could break the protocol entirely.

Transferring humanity is in many scenarios urgent, like when a user needs to participate in DeFi. If they fall victim to a griefing attack, they’re blocked from participating until the revocation is resolved, potentially leading to indirect financial loss.

As a result, this issue still stands and represents a valid griefing attack scenario.

[clesaege]

Transferring humanity is in many scenarios urgent, like when a user needs to participate in DeFi.

Users can never have an expectation of a fast transfer from one platform to another as humanities can be targeted by revocation requests. They can still bridge using updateHumanity.

Users can be the target of revocation requests at any time, preventing immediate transfer, but after a request there is a cooldown to allow them to transfer. This is the expected behaviour (similar to how you can delay a registration by challenging a request, this is OK as it costs the delayer money and the duration you are able to delay has an upper bound).