hats-finance / Fenix--0x9d7765a7ebd5b6322a30797a44a5428531970d3d

0 stars 1 forks source link

`getCheckpointIndex()` will lead to misinformation #25

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

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

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

Description: Description

The getCheckpointIndex() function retrieves the index of the checkpoint that is nearest to and less than or equal to the given timestamp. It uses binary search to find the closest timestamp. However, the function currently lacks a check to revert if timestamp_ >= block.timestamp, which can lead to misinformation when getCheckpointIndex() is called. Issue is when it doesn't revert if timestamp_ >= block.timestamp

  1. Proof of Concept (PoC) File

    function getCheckpointIndex(
        mapping(uint256 index => Checkpoint checkpoint) storage self_,
        uint256 lastIndex_,
        uint256 timestamp_
    ) internal view returns (uint256) {
        if (lastIndex_ == 0) {
            return 0;
        }
    
        if (self_[lastIndex_].timestamp <= timestamp_) {
            return lastIndex_;
        }
    
        if (self_[0].timestamp > timestamp_) {
            return 0;
        }
    
        uint256 start;
        uint256 end = lastIndex_;
        while (end > start) {
            uint256 middle = end - (end - start) / 2;
            Checkpoint memory checkpoint = self_[middle];
            if (checkpoint.timestamp == timestamp_) {
                return middle;
            } else if (checkpoint.timestamp < timestamp_) {
                start = middle;
            } else {
                end = middle - 1;
            }
        }
    
        return start;
    }

    https://github.com/hats-finance/Fenix--0x9d7765a7ebd5b6322a30797a44a5428531970d3d/blob/353c8e8e24454336e805e5c0e11e4e9ae1491d03/contracts/nest/libraries/VirtualRewarderCheckpoints.sol#L74C1-L106C6

The function lacks a necessary check to revert if timestamp_ >= block.timestamp. This omission can lead to incorrect results when fetching information through binary search.

A similar concept from Compound's Comp contract includes a check for valid block numbers to prevent misinformation:

Block number must be a finalized block or else this function will revert to prevent misinformation https://github.com/compound-finance/compound-protocol/blob/master/contracts/Governance/Comp.sol#L190

  1. Revised Code File (Optional)
    function getCheckpointIndex(
        mapping(uint256 index => Checkpoint checkpoint) storage self_,
        uint256 lastIndex_,
        uint256 timestamp_
    ) internal view returns (uint256) {
+       require(timestamp_ < block.timestamp, "Invalid timestamp");
        if (lastIndex_ == 0) {
            return 0;
        }

        if (self_[lastIndex_].timestamp <= timestamp_) {
            return lastIndex_;
        }

        if (self_[0].timestamp > timestamp_) {
            return 0;
        }

        uint256 start;
        uint256 end = lastIndex_;
        while (end > start) {
            uint256 middle = end - (end - start) / 2;
            Checkpoint memory checkpoint = self_[middle];
            if (checkpoint.timestamp == timestamp_) {
                return middle;
            } else if (checkpoint.timestamp < timestamp_) {
                start = middle;
            } else {
                end = middle - 1;
            }
        }

        return start;
    }
0xmahdirostami commented 1 month ago

The future checks is a best practice technique and does not lead to a critical effect / lack of POC