The timestamp variable of a checkpoint is not initialized

[hats-bug-reporter[bot]]

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

Description: Context: VotingEscrowUpgradeable.sol#L1251, VotingEscrowUpgradeable.sol#L1163, VotingEscrowUpgradeable.sol#L56

Description\ A checkpoint contains a timestamp variable which stores the block number the checkpoint is created.

    /// @notice A checkpoint for marking delegated tokenIds from a given timestamp
    struct Checkpoint {
        uint timestamp;
        uint[] tokenIds;
    }

Attack Scenario\ However, it was found that the timestamp variable of a checkpoint was not initialized anywhere in the codebase. Therefore, any function that relies on the fromBlock of a checkpoint will break.

The VotingEscrowUpgradeable._findWhatCheckpointToWrite and VotingEscrowUpgradeable.getPastVotesIndex functions were found to rely on the timestamp variable of a checkpoint for computation. The following is a list of functions that calls these two affected functions.

_findWhatCheckpointToWrite -> _moveTokenDelegates -> _transferFrom
_findWhatCheckpointToWrite -> _moveTokenDelegates -> _mint
_findWhatCheckpointToWrite -> _moveTokenDelegates -> _burn
_findWhatCheckpointToWrite -> _moveAllDelegates -> _delegate -> delegate/delegateBySig

getPastVotesIndex -> getPastVotes

Instance 1 - VotingEscrowUpgradeable._findWhatCheckpointToWrite function

The VotingEscrowUpgradeable._findWhatCheckpointToWrite function verifies if the timestamp of the latest checkpoint of an account is equal to the current block number. If true, the function will return the index number of the last checkpoint.

    function _findWhatCheckpointToWrite(address account) internal view returns (uint32) {
        uint _timestamp = block.timestamp;
        uint32 _nCheckPoints = numCheckpoints[account];

        if (_nCheckPoints > 0 && checkpoints[account][_nCheckPoints - 1].timestamp == _timestamp) {
            return _nCheckPoints - 1;
        } else {
            return _nCheckPoints;
        }
    }

As such, this function does not work as intended and will always return the index of a new checkpoint.

Instance 2 - VotingEscrowUpgradeable.getPastVotesIndex function

The VotingEscrowUpgradeable.getPastVotesIndex function relies on the timestamp of the latest checkpoint for optimization purposes. If the request block number is the most recently updated checkpoint, it will return the latest index immediately and skip the binary search. Since the timestamp variable is not populated, the optimization will not work.

    function getPastVotesIndex(address account, uint timestamp) public view returns (uint32) {
        uint32 nCheckpoints = numCheckpoints[account];
        if (nCheckpoints == 0) {
            return 0;
        }
        // First check most recent balance
        if (checkpoints[account][nCheckpoints - 1].timestamp <= timestamp) {
            return (nCheckpoints - 1);
        }

        // Next check implicit zero balance
        if (checkpoints[account][0].timestamp > timestamp) {
            return 0;
        }

        uint32 lower = 0;
        uint32 upper = nCheckpoints - 1;
        while (upper > lower) {
            uint32 center = upper - (upper - lower) / 2; // ceil, avoiding overflow
            Checkpoint storage cp = checkpoints[account][center];
            if (cp.timestamp == timestamp) {
                return center;
            } else if (cp.timestamp < timestamp) {
                lower = center;
            } else {
                upper = center - 1;
            }
        }
        return lower;
    }

Recommendation: Initialize the timestamp variable of the checkpoint in the codebase.

[BohdanHrytsak]

Thank you for the submission.

The submission is valid, the timestamp is not initialized anywhere, which leads to some methods not working as expected (incorrectly)

There is not enough criticality and critical cases to recognize it as valid.

Since this code is inherited and is original from Thena, OOS

[sonny2k]

According to Hats rules in Fenix Finance competition for Medium severity issue, it says: "Attacks that make essential functionality of the contracts temporarily unusable or inaccessible."

And, the fact that the timestamp variable of a checkpoint is not initialized will render these main functions (_moveTokenDelegates, _moveAllDelegates) not working as expected (you said above)

_findWhatCheckpointToWrite -> _moveTokenDelegates -> _mint
_findWhatCheckpointToWrite -> _moveTokenDelegates -> _burn
_findWhatCheckpointToWrite -> _moveAllDelegates -> _delegate -> delegate/delegateBySig

getPastVotesIndex -> getPastVotes

[BohdanHrytsak]

The question of whether the issue is valid is not raised, as there is indeed a problem with the timestamp.

It is located in the OOS section as it is inherited from the Thena/Chronos code and is not included in the scope.

The issue itself does not lead to critical consequences that are described in the scopes to include it in the scopes, so it remains OOS