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

Proof of Humanity Protocol v2
2 stars 1 forks source link

Exploit in claimHumanity and withdrawRequest Functions #170

Open hats-bug-reporter[bot] opened 1 month ago

hats-bug-reporter[bot] commented 1 month ago

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

Description: Description\ The current implementation of the claimHumanity and withdrawRequest functions in the ProofOfHumanityExtended.sol contract allows a user to exploit the system by creating a request with zero msg.value and then withdrawing the request after it has been fully funded by other contributors. This can lead to frustration among genuine contributors who have funded the request in good faith with other impacts

Attack Scenario\

User Calls claimHumanity with Zero msg.value:

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
    );
}

_requestHumanity Function:

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);

    // 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);
}
function _contribute(
    bytes20 _humanityId,
    uint256 _requestId,
    uint256 _challengeId,
    uint256 _roundId,
    Party _side,
    uint256 _totalRequired
) internal returns (bool paidInFull) {
    Round storage round = humanityData[_humanityId].requests[_requestId].challenges[_challengeId].rounds[_roundId];

    uint256 remainingETH;
    uint256 contribution = msg.value;
    uint256 requiredAmount = _totalRequired.subCap(
        _side == Party.Requester ? round.paidFees.forRequester : round.paidFees.forChallenger
    );
    if (requiredAmount <= msg.value) {
        contribution = requiredAmount;
        remainingETH = msg.value - requiredAmount;

        paidInFull = true;
        round.sideFunded = round.sideFunded == Party.None ? _side : Party.None;
    }

    if (_side == Party.Requester) {
        round.contributions[msg.sender].forRequester += contribution;
        round.paidFees.forRequester += contribution;
    } else {
        round.contributions[msg.sender].forChallenger += contribution;
        round.paidFees.forChallenger += contribution;
    }
    round.feeRewards += contribution;

    emit Contribution(_humanityId, _requestId, _challengeId, _roundId, msg.sender, contribution, _side);

    if (remainingETH != 0) payable(msg.sender).safeSend(remainingETH, wNative);
}

Crowdfunding by Other Contributors:

User Calls withdrawRequest:

Attachments

  1. Proof of Concept (PoC) File

  2. Revised Code File (Optional)

    • Introduce a minimum deposit requirement for the requester when calling claimHumanity
clesaege commented 1 month ago

If the user withdraws the request, funders can get they money back. Users necessarily need to trust people they fund requests for (as those could be malicious leading to a loss of their deposit). If the user withdraws the request, contributors can be refunded. This is the expected behaviour.

batmanBinary commented 1 month ago

@clesaege ,Thank you for your response. While I understand that funders can get their money back if the user withdraws the request, I would like to elaborate on why this issue should still be considered valid.

Summery

Yes, I agree that if the users withdraw the request, the funders can withdraw their money back. However, as there is no authorization, or we can say that anyone can create a request free of cost, many users can create numerous requests. This can end up with the funders' money being trapped (for a period of time, like until All the requests are fully funded and users call the withdrawRequest). In this scenario, some legitimate users may face long waiting periods as the crowdfund is invested in these trap profiles.

Creating a Request Without Cost:

Multiple Requests by Malicious Users:

Funders Contribute:

Money Gets Trapped:

Withdrawal by Malicious Users:

Impact on Legitimate Users:

clesaege commented 1 month ago

as the crowdfund is invested in these trap profile

If you fund a "trap" (malicious) profile, your money could be lost (as they can lose their disputes). You should only fund legitimate people you can trust. This is the expected behaviour.

Other users, who are funders, see these requests and start contributing money to them.

While would they do that? Users should not crowdfund random profiles.

batmanBinary commented 1 month ago

Hey @clesaege,

As there is no authorization, or we can say that anyone can create a request free of cost, many users can create numerous requests.

Here I am assuming that users who have the intention to DoS (the legit users), I mean here users are alive and as expected but they can simply DoS while creating requests with 0 msg.value.

If you fund a "trap" (malicious) profile, your money could be lost (as they can lose their disputes). You should only fund legitimate people you can trust.

As I mentioned, here users will not be bots or AI as it will be easily detected. I am assuming the users who are alive, they just need to create a request and withdraw when fully funded.

Why would they do that? Users should not crowdfund random profiles.

Again, here as the users are alive and as expected, and the users have only the intention to create a request and withdraw when fully funded. Here, as the users follow the guidelines, the users are expected to fund the profile, right?

@clesaege does this make sense to you now? Please let me know.

clesaege commented 1 month ago

Here, as the users follow the guidelines, the users are expected to fund the profile, right?

No, you should only fund people you know and trust, not random profiles you see on the front.