Unsafe type casting of `arbitratorDataId` in `ProofOfHumanity.sol::_requestHumanity`.

[hats-bug-reporter[bot]]

Github username: @erictee2802 Twitter username: 0xEricTee Submission hash (on-chain): 0x7fda2a1003378e68127eb5da6048241a045522fb250474f4c25916ba2a3e9d9c Severity: low

Description: Description

In ProofOfHumanity.sol::_requestHumanity:

 function _requestHumanity(bytes20 _humanityId) internal returns (uint256 requestId) {
        // Human must not be in the process of claiming a humanity.
        require(humanityData[accountHumanity[msg.sender]].requestCount[msg.sender] == 0);

        Humanity storage humanity = humanityData[_humanityId];

        requestId = humanity.requests.length;

        Request storage request = humanity.requests.push();
        request.requester = payable(msg.sender);

        uint256 arbitratorDataId = arbitratorDataHistory.length - 1;
        request.arbitratorDataId = uint16(arbitratorDataId); //erictee-issue: type(uint16).max = 65535 Unsafe casting.

        // Use requestCount like this in order to avoid having referencing the claimer's pending request as 0 at any point.
        humanity.requestCount[msg.sender] = requestId + 1;
        accountHumanity[msg.sender] = _humanityId;

        ArbitratorData memory arbitratorData = arbitratorDataHistory[arbitratorDataId];
        uint256 totalCost = arbitratorData.arbitrator.arbitrationCost(arbitratorData.arbitratorExtraData).addCap(
            requestBaseDeposit
        );
        _contribute(_humanityId, requestId, 0, 0, Party.Requester, totalCost);
    }

Type casting of uint256 to uint16 in line 1418 request.arbitratorDataId = uint16(arbitratorDataId); is unsafe as it will silently overflow if the value of arbitratorDataId >= 65536.

Attack Scenario

Everytime the functions ProofOfHumanity.sol::changeMetaEvidence, ProofOfHumanity.sol::changeArbitrator are called, arbitratorDataHistory.length will be increased by 1. When this value reaches 65536, the mentioned line will silently overflow and arbitratorDataId = 0. As a result, stale data of arbitratorData will be used to calculate the arbitrationCost. The impact of this issue is medium and likelihood is low, therefore im rating this issue as low severity.

Attachments

NA

1. Proof of Concept (PoC) File

Manual Analysis

2. Revised Code File (Optional)

Consider changing the type of arbitratorDataId in struct Request to uint256 to avoid unsafe type casting.

[clesaege]

As per your report, this would require 65536 arbitrator changes (and the associated governance processes) to cause a problem. This cannot happen in practice.

As per contest rules are excluded:

• Any issue that is only theoretical but can't happen in practice.
• Issues about the ability for a governor/owner to set parameters in a way breaking the contract (they are trusted to be both non-malicious and non-stupid).