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

Proof of Humanity Protocol v2
2 stars 1 forks source link

Lack of Validation for `_evidence` and `_name` in claimHumanity Function #36

Open hats-bug-reporter[bot] opened 2 weeks ago

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

Github username: -- Twitter username: -- Submission hash (on-chain): 0x1c668ee76895cd52c90dbda9dc7840b804c7e8ad1e7e1a03720e3658cb13e9cd Severity: medium

Description: Description\

    function claimHumanity(bytes20 _humanityId, string calldata _evidence, string calldata _name) external payable {
        Humanity storage humanity = humanityData[_humanityId];

        require(_humanityId != 0);
        require(!isHuman(msg.sender));
        require(humanity.owner == address(0x0) || humanity.expirationTime < block.timestamp);

        uint256 requestId = _requestHumanity(_humanityId);

        emit ClaimRequest(msg.sender, _humanityId, requestId, _name);
        emit Evidence(
            arbitratorDataHistory[arbitratorDataHistory.length - 1].arbitrator,
            uint256(keccak256(abi.encodePacked(_humanityId, requestId))),
            msg.sender,
            _evidence
        );
    }

The claimHumanity function does not perform any validation on the _evidence and _name parameters. This lack of validation can lead to several potential issues:

Empty Strings: Users might submit empty strings for both _evidence and _name, which could be meaningless and not useful for the purpose of the function. Malicious Input: Without validation, users could submit malicious or inappropriate content, which could be stored and later displayed or processed by the system. Data Integrity: The absence of validation could lead to inconsistencies and unreliable data within the system, making it difficult to trust the information stored.

Attack Scenario\ Describe how the vulnerability can be exploited.

Attachments

  1. Proof of Concept (PoC) File

  2. Revised Code File (Optional)

    To address this issue, it is recommended to add validation checks for _evidence and _name to ensure they are not empty and meet certain criteria.

clesaege commented 2 weeks ago

An empty name is valid from the code perspective but can be challenged. Incorrect evidence are handled by the frontend (and this will lead to the profile being challenged).

PoH is a TCR, that is it's goal to only accept valid information (by challenging invalid submissions).

batmanBinary commented 1 week ago

@clesaege ,thank you for the reply.i have rechecked the issue

An empty name is valid from the code perspective but can be challenged.

image

as mentioned in the POH registry policy the name is required and is expected to be valid(can be anything like any name) and as mentioned Name under which the submitter is known (any UTF8 characters) - Required here by allowing empty name submitter maynot be identified like unknown .

Incorrect evidence are handled by the frontend (and this will lead to the profile being challenged).

as the contracts are to be deployed on mainnet like(i mean publicly accessible).some user may prefer directly interacting with the contract rather than the frontEnd.

PoH is a TCR, that is it's goal to only accept valid information (by challenging invalid submissions).

yes,but this code level issues should be handled by smart contract itself by verification/authorization,instead of depending on the challengers and the resources.maybe some legitimate users lose their deposit(for the empty name as allowed) due to losing in challenge

@clesaege let me know your Thoughts on this..

clesaege commented 1 week ago

the name is required and is expected to be valid

And if it is not provided, the submission can be challenged.

as the contracts are to be deployed on mainnet like(i mean publicly accessible).some user may prefer directly interacting with the contract rather than the frontEnd.

Yes there is not problem to that, the frontend should check the validity of evidence submitted.

but this code level issues should be handled by smart contract itself by verification/authorization

Not if they don't affect the smart contract logic. Actually Kleros Coop development guidelines explicitly warn against doing so. image