DoS due to array out-of-bound access error

[hats-bug-reporter[bot]]

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

Description: Description\ The ProofOfHumanity::advanceState() is expected to work properly without 2 arguments - 1. address[] calldata _vouches & 2. SignatureVouch[] calldata _sigantureVouches. But if the function is called like this i.e without these 2 arguments then the call will revert by 'array out-of-bound access'. The reason behind this is at first request.vouches.length is 0, if we consider the requiredNumberOfVouches 1 so it will iterate 1 time, so in first itereation as we did not pass the _signatureVouches the nbSignatureVouches will be 0, so as 0 is not less than 0 so the if block will not execute, the else part will execute, in else part voucherAccount = _vouches[i - nbSignatureVouches] = _vouches[0], so as there is no element in 0th index so it will revert by out of bound access error.

Attack Scenario\

None attack is required, it will DoS automatically. Attachments

1. 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));
    registrationMetaEvidence = "/ipfs/QmadJhyPxhk5AYrdE6JMwhC7TpsA47YZwFP28VKkr1ffJF";
    clearingMetaEvidence = "/ipfs/QmRqKmjVk1FcCRcTnuZmMG6SZEBB9LkUJb7Z4SVhJGHEfw";
    requestBaseDeposit = 110000000000000000000; // 110 ether
    humanityLifespan = 31557600;
    renewalPeriodDuration = 28512000; // 330 days
    challengePeriodDuration = 302400;
    failedRevocationCooldown = 302400;
    requiredNumberOfVouches = 1;
    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_claimingHumanityWithAnyId() public {
    address user = makeAddr("user");
    address user2 = makeAddr("user2");
    vm.deal(user, 1000 ether);
    bytes20 id = bytes20(uint160(100));
    vm.prank(user2);
    poh.addVouch(user, id);
    vm.prank(user);
    // 1st claim
    poh.claimHumanity{value: 200 ether}(id, "", ""); // success
    (, , ,uint16 lastChallengeId , , , , ProofOfHumanity.Status status, ) = poh.getRequestInfo(id, 0);
    console.log("status: ", uint(status)); // 0 means status is still in Vouching state
    assert(lastChallengeId == 0); // We know challenge Id is 0

    // 2nd then call advanceState to advance to request, before that an so called
    // registered user add voucher for him
    address[] memory vouches = new address[](1);
    vouches[0] = user2;
    assertEq(poh.requiredNumberOfVouches(),1);
    (bool vouching,,,,,) = poh.getHumanityInfo(id);
    assert(vouching == false);
    poh.advanceState(user, new address[](0), new ProofOfHumanity.SignatureVouch[](0));

}

}

To assure the point of revert is [this](https://github.com/hats-finance/Proof-Of-Humanity-V2-0xef0709445d394a22704850c772a28a863bb780b0/blob/07f6529954291f79b3c690881004f306da97759f/contracts/ProofOfHumanity.sol#L952) line, add assertion before and after that line & test it by commenting out each at once, like this:

// assert(1 == 2); voucherAccount = _vouches[i - nbSignatureVouches]; assert(1 == 2);

 In this case the test will revert by out of bound access error, but if we uncomment the first assertion and comment out the second assertion then it will revert by assertion error, try it.

2. **Revised Code File (Optional)**
- https://github.com/hats-finance/Proof-Of-Humanity-V2-0xef0709445d394a22704850c772a28a863bb780b0/blob/07f6529954291f79b3c690881004f306da97759f/contracts/ProofOfHumanity.sol#L952

[clesaege]

poh.advanceState(user, new address[](0), new ProofOfHumanity.SignatureVouch[](0)); In your example, you try to advance without providing vouches. It should indeed fail. So this is the desired behaviour.

[itsabinashb]

@clesaege calling the advanceState() without vouches is intended because it is expected as per the natspec:

• @param _claimer The address of the human whose request status to advance.
• @param _vouches Array of users whose vouches to count (optional).
• @param _signatureVouches Array of EIP-712 signatures of struct IsHumanVoucher (optional). here

you can see _vouches & _signatureVouches are optional.

[clesaege]

Each are optional, but providing neither will not exhibit enough vouches so would fail. This is the desired behaviour.