Timestamp not updated

Summary

The checkpoint system is totally broken because the timestamp is never updated.

Vulnerability Detail

The checkpoints mapping has the following struct:

    struct Checkpoint {
        uint timestamp;
        uint[] tokenIds;
    }

However, the timestamp value for each is never set in VotingEscrow.sol contract, even though it is used at multiple points with the contract such as:

VotingEscrow::getPastVotesIndex()
        // 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;
        }

 if (cp.timestamp == timestamp) {
                return center;
            } else if (cp.timestamp < timestamp) {
                lower = center;
            } else {
                upper = center - 1;
            }

and

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

getPastVotesIndex() is used in getPastVotes()
_findWhatCheckpointToWrite is used internally in merge(), withdraw(), transferFrom(), safeTransferFrom(), split(), create_lock_for(), create_lock(), delegateBySig(), delegate()

Impact

All conditions involving timestamp checks especially in _findWhatCheckpointToWrite() which is used to determine the next point to return wrong figures. Which is then used to update values. In the case where checkpoints[account][_nCheckPoints - 1].timestamp == _timestamp is true, the new values are written to the wrong checkpoint. Using VotingEscrow.sol with this issue is suicide.

Code Snippet


 function _moveTokenDelegates(
        address srcRep,
        address dstRep,
        uint _tokenId
    ) internal {
        if (srcRep != dstRep && _tokenId > 0) {
            if (srcRep != address(0)) {
                uint32 srcRepNum = numCheckpoints[srcRep];
                uint[] storage srcRepOld = srcRepNum > 0
                    ? checkpoints[srcRep][srcRepNum - 1].tokenIds
                    : checkpoints[srcRep][0].tokenIds;
                uint32 nextSrcRepNum = _findWhatCheckpointToWrite(srcRep);
                uint[] storage srcRepNew = checkpoints[srcRep][nextSrcRepNum]
                    .tokenIds;
                // All the same except _tokenId
                for (uint i = 0; i < srcRepOld.length; i++) {
                    uint tId = srcRepOld[i];
                    if (tId != _tokenId) {
                        srcRepNew.push(tId);
                    }
                }

                numCheckpoints[srcRep] = srcRepNum + 1;
            }

Tool used

Manual Review

Recommendation

Update the timestamps whenever updating the values of the tokenId

Duplicate of #288

sherlock-audit / 2024-06-velocimeter-judging

Ch_301 - Timestamp not updated #541