Incorrect Increment In `challenge.lastRoundId` Results In `rule()` Always Having Empty Round

[hats-bug-reporter[bot]]

Github username: -- Twitter username: johny37GR Submission hash (on-chain): 0x4a8bf545098f51115e614fa80feaa91cb27b2cb54c63839cb7727e3e65adbc11 Severity: low

Description: Description\ In challengeRequest() the challenge.lastRoundId is incremented by one after calling _contribute() with _roundId equals zero. Which leads to the rule() always retrieving an empty round Round storage round = challenge.rounds[challenge.lastRoundId];, because the lastRoundId will be 1, while the _roundId that was used in _contribute() was 0.

Attachments

1. Proof of Concept

ProofOfHumanity.sol@L1005-1071

function challengeRequest(
    bytes20 _humanityId,
    uint256 _requestId,
    Reason _reason,
    string calldata _evidence
) external payable {
    Request storage request = humanityData[_humanityId].requests[_requestId];
    // If request is for revocation request reason must be None, otherwise must be not None
    require(request.revocation == (_reason == Reason.None));
    require(request.status == Status.Resolving);
    require(block.timestamp < request.challengePeriodStart + challengePeriodDuration);

    // Only check used reasons on claim requests
    if (!request.revocation) {
        // Get the bit that corresponds with reason's index.
        uint8 reasonBit;
        unchecked {
            reasonBit = uint8(1 << (uint256(_reason) - 1));
        }

        require((reasonBit & ~request.usedReasons) == reasonBit);

        // Mark the bit corresponding with reason's index as 'true', to indicate that the reason was used.
        request.usedReasons ^= reasonBit;

        request.currentReason = _reason;
    }

    uint256 challengeId = request.lastChallengeId++;
    Challenge storage challenge = request.challenges[challengeId];
    Round storage round = challenge.rounds[0];

    ArbitratorData memory arbitratorData = arbitratorDataHistory[request.arbitratorDataId];
    uint256 arbitrationCost = arbitratorData.arbitrator.arbitrationCost(arbitratorData.arbitratorExtraData);

@>  require(_contribute(_humanityId, _requestId, challengeId, 0, Party.Challenger, arbitrationCost));

    // Subtract the costs from the total of staked contributions.
    round.feeRewards = round.feeRewards.subCap(arbitrationCost);

    uint256 disputeId = arbitratorData.arbitrator.createDispute{value: arbitrationCost}(
        RULING_OPTIONS,
        arbitratorData.arbitratorExtraData
    );
    challenge.disputeId = disputeId;
    challenge.challenger = payable(msg.sender);
@>  challenge.lastRoundId++;

    DisputeData storage disputeData = disputeIdToData[address(arbitratorData.arbitrator)][disputeId];
    disputeData.humanityId = _humanityId;
    disputeData.requestId = uint96(_requestId);
    disputeData.challengeId = uint96(challengeId);

    request.status = Status.Disputed;

    // Hash evidenceGroupId to make sure it's unique.
    uint256 evidenceGroupId = uint256(keccak256(abi.encodePacked(_humanityId, _requestId)));

    emit RequestChallenged(_humanityId, _requestId, challengeId, _reason, disputeId);
    emit Dispute(
        arbitratorData.arbitrator,
        disputeId,
        2 * arbitratorData.metaEvidenceUpdates + (request.revocation ? 1 : 0),
        evidenceGroupId
    );
    emit Evidence(arbitratorData.arbitrator, evidenceGroupId, msg.sender, _evidence);
}

ProofOfHumanity.sol@L1315-1377

function rule(uint256 _disputeId, uint256 _ruling) external override {
    DisputeData memory disputeData = disputeIdToData[msg.sender][_disputeId];
    Humanity storage humanity = humanityData[disputeData.humanityId];
    Request storage request = humanity.requests[disputeData.requestId];
    Challenge storage challenge = request.challenges[disputeData.challengeId];
@>  Round storage round = challenge.rounds[challenge.lastRoundId];
    ...
}

2. Revised Code Instead of using the challenge.lastRoundId, use challenge.lastRoundId - 1

[clesaege]

The last round will be empty if no one funds appeals. If one does, it will not be be.

[Giannis443]

hey @clesaege, thank you for the reply.

In case you are referring to last round, meaning this: Round storage round = challenge.rounds[challenge.lastRoundId];, it will be empty even if an appeal gets funded, because in the line Round storage round = challenge.rounds[challenge.lastRoundId]; the appeal edits the round with the lastRoundId and increments the lastRoundId challenge.lastRoundId++; after it has started editing this round. Which will lead to the rule() function reading an empty round because the lastRoundId is incremented only after the round has been edited, so it ends up pointing to an empty round.

[clesaege]

What you say seems correct, but it is just a description of the contract behaviour, I don't see any issue. If there is an appeal, the last round will be empty, unless someone funds an appeal (and then it won't be empty). The last round is only non empty if a side was funded while the other was not.