V1 Profile Exploit: Bypassing Vouching Phase to Register on Multiple Chains

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

Github username: @0xmahdirostami Twitter username: 0xmahdirostami Submission hash (on-chain): 0x7a8220354ae1e9ef4446a2566e9e58ae4e9581e133b1b5e45d23ea72877d3670 Severity: medium

Description:

Summary

The ProofOfHumanityExtended::ccDischargeHumanity function currently only checks for the vouching phase in v2 humanity profiles, allowing v1 profile users to exploit the system. By combining the transferHumanity and renewHumanity functions, a malicious user could bypass the vouching phase and register the same humanity ID on two different chains.

Vulnerability Detail

The issue arises due to inconsistencies in how the system handles v1 and v2 profiles during the transfer and renewal processes:

transferHumanity Function: This function transfers humanity IDs between chains, deleting the humanity ID from the source chain and registering it on the target chain. It calls ccDischargeHumanity to ensure that the user is not in the vouching phase before transferring.
ProofOfHumanityExtended::ccDischargeHumanity Function: This function checks whether the user is currently in the vouching phase, but only for v2 profiles. V1 profiles can pass the check without verifying the vouching status.
ProofOfHumanityExtended::renewHumanity Function: This function allows users to renew their humanity ID.

Exploit Scenario:

A user with a v1 humanity ID (X) calls ProofOfHumanityExtended::renewHumanity. (goes to vouching phase)
As the user is trusted, he gets vouches, but does not call advanceState
The user then calls transferHumanity, which invokes ccDischargeHumanity. As the function does not check vouching status for v1 profiles, the user successfully transfers humanity ID X to another chain.
The user can then call advanceState.

Impact

This vulnerability allows users to bypass the critical vouching phase, undermining the integrity of the Proof of Humanity system by registering the same humanity ID across multiple chains without proper verification.

Recommendation

Ensure that the vouching phase is consistently checked for both v1 and v2 users in all ProofOfHumanityExtended::ccDischargeHumanity.

clesaege commented 2 weeks ago

This isn't a problem as when a user renews, he would have a new submission time assigned to it. This either in the executeRequest or the executeRuling function.

So if a user renews, on V1, he will have a submission time post fork and his profile will not be valid for v2.

0xmahdirostami commented 1 week ago

@clesaege

Ensure that the vouching phase is consistently checked for both v1 and v2 users in all ProofOfHumanityExtended::ccDischargeHumanity.

If v1 profiles are not checked for vouching, they could be transferred with vouching set to true.

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

0xmahdirostami commented 1 week ago

POC:

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import {ProofOfHumanityExtended} from "./../contracts/extending-old/ProofOfHumanityExtended.sol";
import {CrossChainProofOfHumanity} from "./../contracts/CrossChainProofOfHumanity.sol";

import {ForkModule} from "./../contracts/extending-old/ForkModule.sol";
import {Test, console} from "forge-std/Test.sol";

contract testest is Test {
    ProofOfHumanityExtended dappExtened;
    ForkModule dappFork;
    CrossChainProofOfHumanity dappCC;
    bytes20 humanity = bytes20(keccak256("0x"));
    address PROOF_EXTENDED = 0x87c5c294C9d0ACa6b9b2835A99FE0c9A444Aacc1;
    address CC = 0xD8D462ac9F3FAD77Af2ae2640fE7F591F1651A2C;
    address BGA = 0x290e997D7c46BDFf666Ad38506fcFB3082180DF9;
    address FORK_ADDRESS = 0x116cB4077afbb9B5c7E0dCd5fc4Ce943Ab624dbF;
    address V1_USER = 0x9359E6A126d25D75F73669B29ca666df12D834D7;
    address V2_USER_ALICE = address(0x1231321);
    bytes20 V1_HUMANITY = bytes20(V1_USER);
    bytes20 V2_HUMANITY = bytes20(V2_USER_ALICE);
    address[] vouches;

    function setUp() public {
        vm.selectFork(vm.createFork("https://eth-mainnet.g.alchemy.com/v2/URL", 20620123));
        dappExtened = ProofOfHumanityExtended(PROOF_EXTENDED);
        dappFork = ForkModule(FORK_ADDRESS);
        dappCC = CrossChainProofOfHumanity(CC);
        vm.deal(V1_USER, 10 ether);
        vm.deal(V2_USER_ALICE, 10 ether);
        assert(dappFork.isRegistered(V1_USER) == true);
    }

    function testF() public {
        vm.prank(V2_USER_ALICE);
        dappExtened.claimHumanity{value: 10 ether}(V2_HUMANITY, "first", "first");

        vm.startPrank(V1_USER);
        dappExtened.addVouch(V2_USER_ALICE, V2_HUMANITY);
        vouches.push(V1_USER);
        ProofOfHumanityExtended.SignatureVouch[] memory signs;
        dappExtened.advanceState(V2_USER_ALICE, vouches, signs);

        dappCC.transferHumanity(BGA);
    }
}

As you can see V1 user transfers to chains while in the vouching phase.

0xshivay commented 1 week ago

Hi, Issue #129 is not a duplicate of Issue #65.

Issue #65 is incorrect.

In summary, Issue #65 claims that malicious users can bypass the vouching phase, which is not accurate.

The `ProofOfHumanityExtended::ccDischargeHumanity` function currently only checks for the vouching phase in v2 humanity profiles, allowing v1 profile users to exploit the system. By combining the `transferHumanity` and `renewHumanity` functions, a malicious user could bypass the vouching phase and register the same humanity ID on two different chains.

The check is specifically for vouchers, not for users currently in the vouching stage. Malicious users cannot bypass the vouching phase due to a missing check; only malicious vouchers can bypass the penalty.

As the function does not check vouching status for v1 profiles, the user successfully transfers humanity ID X to another chain.

The exploit scenario described is also incorrect. The vouching status applies to the vouchers vouching for the user, not the user in the vouching stage. A user in the vouching stage will not have the vouching variable set to true; rather, it is the vouchers' vouching variable that is set to true. You can verify this here: ProofOfHumanity.sol#L968.

There is no need for the ccDischargeHumanity function to check the vouching status of a user in the vouching stage.

The impact is entirely independent of the vulnerability mentioned in Issue #65.

The recommended fix will not resolve the issue, as the problem is not a missing vouching phase check for a user in the vouching stage, but rather a missing check for the vouching variable for vouchers.

Additionally, the POC you mentioned in the comments of Issue #65 is related to a completely different vulnerability with a different root cause, which is not outlined in your issue.

0xmahdirostami commented 1 week ago

@0xshivay

The ProofOfHumanityExtended::ccDischargeHumanity function currently only checks for the vouching phase in v2 humanity profiles, allowing v1 profile users to exploit the system.

For this report, i used ChatGPT and unfortunately, i didn't check the rephrased sentences. By the term vouching phase i mean the following line: humanity.vouching, it's the only check for v2 users that not check for v1 users.

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

if you check my report:

Summary: The ProofOfHumanityExtended::ccDischargeHumanity function currently only checks for the vouching phase in v2 humanity profiles, allowing v1 profile users to exploit the system.

So the only check that done here for v2 users and not v1 users is require(!humanity.vouching);, that is the root of the issue.

Again in Vulnerability Detail i explain it: This function transfers humanity IDs between chains, deleting the humanity ID from the source chain and registering it on the target chain. It calls ccDischargeHumanity to ensure that the user is not in the vouching phase before transferring. ProofOfHumanityExtended::ccDischargeHumanity Function: This function checks whether the user is currently in the vouching phase, but only for v2 profiles. V1 profiles can pass the check without verifying the vouching status.
In scenario: Yes, I wanted to make it complicated, but the rephrased sentences make it a little less unordered.
Again in Impact section: This vulnerability allows users to bypass the critical vouching phase, undermining the integrity of the Proof of Humanity system by registering the same humanity ID across multiple chains without proper verification.
Recommendation: Ensure that the vouching phase is consistently checked for both v1 and v2 users in all ProofOfHumanityExtended::ccDischargeHumanity. the recommendation is the same as for you.

0xshivay commented 1 week ago

@0xmahdirostami The terms "vouching phase" and "humanity.vouching" have significantly different meanings. The term "vouching phase" refers to a user who is in the process of claiming or reclaiming their humanity and is currently in the vouching stage, where they collect vouches to proceed further.

On the other hand, humanity.vouching refers to a variable that is specifically set for vouchers.

In Exploit Scenario of your report, it states:

A user with a v1 humanity ID (X) calls ProofOfHumanityExtended::renewHumanity. (goes to vouching phase)

This sentence clearly demonstrates how the term "vouching phase" is used in your report, so the term "vouching phase" cannot be interpreted to mean humanity.vouching when they have vastly different meanings.

You cannot attribute this to ChatGPT and then claim you meant something else entirely afterward.

clesaege commented 1 week ago

Yes, it looks like #65 submitter found something which looked wrong but drew the wrong conclusions about it while https://github.com/hats-finance/Proof-Of-Humanity-V2-0xef0709445d394a22704850c772a28a863bb780b0/issues/129 managed to find out what was the issue.

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