Off-by-One Error in Time-Based Condition Checks

[hats-bug-reporter[bot]]

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

Description: Description

The contract inconsistently uses time-based condition checks, leading to potential off-by-one errors. Specifically, the stateOpen and stateOpenOrPendingArbitration modifiers use finalize_ts > block.timestamp to check if the finalization deadline has not passed, indicating an exclusive check. Similarly, the submitAnswerReveal function also uses the same exclusive check with commitments[commitment_id].reveal_ts > block.timestamp to ensure the reveal deadline has not passed. In contrast, the isFinalized function uses finalize_ts <= block.timestamp to check if the finalization deadline has passed, making it an inclusive check. The assignWinnerAndSubmitAnswerByArbitrator function also uses < in commitments[last_answer_or_commitment_id].reveal_ts < block.timestamp to check the reveal deadline, which could cause off-by-one errors when the reveal time is exactly equal to the current timestamp. This inconsistency can lead to unexpected behavior and logical errors in time-sensitive operations, such as preventing legitimate actions or incorrectly determining if a condition has been met.

Attachments

1. Proof of Concept (PoC)

In the RealityETH_v3_0 contract:

function assignWinnerAndSubmitAnswerByArbitrator(bytes32 question_id, bytes32 answer, address payee_if_wrong, bytes32 last_history_hash, bytes32 last_answer_or_commitment_id, address last_answerer) 
    external {
        bool is_commitment = _verifyHistoryInputOrRevert(questions[question_id].history_hash, last_history_hash, last_answer_or_commitment_id, questions[question_id].bond, last_answerer);

        address payee;
        // If the last answer is an unrevealed commit, it's always wrong.
        // For anything else, the last answer was set as the "best answer" in submitAnswer or submitAnswerReveal.
        if (is_commitment && !commitments[last_answer_or_commitment_id].is_revealed) {
            require(commitments[last_answer_or_commitment_id].reveal_ts < uint32(block.timestamp), "You must wait for the reveal deadline before finalizing");      
            payee = payee_if_wrong;
        } else {
            payee = (questions[question_id].best_answer == answer) ? last_answerer : payee_if_wrong;
        }
        submitAnswerByArbitrator(question_id, answer, payee);
    }

2. Revised Code

Align the time-based condition checks across all relevant parts of the contract to use a consistent logic for inclusivity or exclusivity. Review and modify the conditions to ensure they match the intended behavior of the contract, either using inclusive (<=) or exclusive (<) checks consistently, based on the specific use case. This will help avoid off-by-one errors and ensure reliable execution of time-dependent functionalities.

[clesaege]

About finalize_ts there is no inconsistency. It fullfil the finalization criterion if finalize_ts <= uint32(block.timestamp). So it is not fullfilled if finalize_ts > uint32(block.timestamp). So the use is consistent.

For reveal_ts, since both are strict, there is one second where we can't assign the winner nor reveal an answer, but this doesn't lead to any issue (at worse we "waste" 1s which is negligeable).

Per the rules, are excluded:

• Issues in third party contracts (those in the interaction folder + Kleros + Curate + DAI) which do not lead to issues with Seer (if you find those, we'll help you report them to their respective projects).
• Issues about code/project quality which do not lead to exploitable vulnerabilities.