Wrong voting balance due to duplicated veNFTs within a checkpoint

Summary

A checkpoint might include duplicate veNFTs (tokenIDs) in certain situations, resulting in the double counting of voting balances. Malicious users could exploit this vulnerability to artificially increase the voting balance of their accounts. This inflated voting balance could then be used in governance and gauge weight voting, potentially leading to the loss of assets or rewards for other users if manipulated maliciously (e.g., redirecting rewards to gauges where attackers have a vested interest).

Vulnerability Detail

Attack path

Assuming moving tokenID=50 from Alice to Bob.
Source Code Logic (Moving tokenID=50 out of Alice) • Fetch the existing Alice's token IDs and assign them to srcRepOld • Create a new empty array = srcRepNew • Copy all the token IDs in srcRepOld to srcRepNew except for tokenID=50
Destination Code Logic (Moving tokenID=50 into Bob) • Fetch the existing Bobs' token IDs and assign them to dstRepOld • Create a new empty array = dstRepNew • Copy all the token IDs in dstRepOld to dstRepNew • Copy tokenID=50 to dstRepNew

The existing logic works fine as long as a new empty array (srcRepNew OR dstRepNew) is created every single time. The code relies on the _findWhatCheckpointToWrite function to return the index of a new checkpoint.

function _moveTokenDelegates(
..SNIP..
uint32 nextSrcRepNum = _findWhatCheckpointToWrite(srcRep);
uint256[] storage srcRepNew = _checkpoints[srcRep][nextSrcRepNum].tokenIds;

However, the problem is that the _findWhatCheckpointToWrite function does not always return the index of a new checkpoint. It will return the last checkpoint if it has already been written once within the same block timestamp.

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;
        }
    }

If someone triggers the _moveTokenDelegates more than once within the same block (e.g. perform NFT transfer twice to the same person), the _findWhatCheckpointToWrite function will return a new checkpoint in the first transfer but will return the last/previous checkpoint in the second transfer. This will cause the move token delegate logic to be off during the second transfer.

First Transfer at Block 100 Assume the following states: numCheckpoints[Alice] = 1 _checkpoints[Alice][0].tokenIds = [n1, n2] <== Most recent checkpoint numCheckpoints[Bob] = 1 _checkpoints[Bob][0].tokenIds = [n3] <== Most recent checkpoint To move tokenID=2 from Alice to Bob, the _moveTokenDelegates(Alice, Bob, n2) function will be triggered. The _findWhatCheckpointToWrite will return the index of 1 which points to a new array. The end states of the first transfer will be as follows: numCheckpoints[Alice] = 2 _checkpoints[Alice][0].tokenIds = [n1, n2] _checkpoints[Alice][1].tokenIds = [n1] <== Most recent checkpoint numCheckpoints[Bob] = 2 _checkpoints[Bob][0].tokenIds = [n3] _checkpoints[Bob][1].tokenIds = [n2, n3] <== Most recent checkpoint

Everything is working fine at this point in time. Second Transfer at Block 100(same block) To move tokenID=1 from Alice to Bob, the _moveTokenDelegates(Alice, Bob, n1) function will be triggered. This time round since the last checkpoint block is the same as the current block, the _findWhatCheckpointToWrite function will return the last checkpoint instead of a new checkpoint. The srcRepNew and dstRepNew will end up referencing the old checkpoint instead of a new checkpoint. As such, the srcRepNew and dstRepNew array will reference back to the old checkpoint _checkpoints[Alice][1].tokenIds and _checkpoints[Bob][1].tokenIds respectively. The end state of the second transfer will be as follows: numCheckpoints[Alice] = 3 _checkpoints[Alice][0].tokenIds = [n1, n2] _checkpoints[Alice][1].tokenIds = [n1] <== Most recent checkpoint numCheckpoints[Bob] = 3 _checkpoints[Bob][0].tokenIds = [n3] _checkpoints[Bob][1].tokenIds = [n2, n3, n2, n3, n1] <== Most recent checkpoint

Four (4) problems could be observed from the end state:

The numCheckpoints is incorrect. Should be two (2) instead to three (3)
TokenID=1 has been added to Bob's Checkpoint, but it has not been removed from Alice's Checkpoint
Bob's Checkpoint contains duplicated tokenIDs (e.g. there are two TokenID=2 and TokenID=3)
TokenID is not unique (e.g. TokenID appears more than once)

Since the token IDs within the checkpoint will be used to determine the voting power, the voting power will be inflated in this case as there will be a double count of certain NFTs.

Impact

Inflation of voting balance.

Code Snippet

https://github.com/sherlock-audit/2024-06-velocimeter/blob/main/v4-contracts/contracts/VotingEscrow.sol#L1423 https://github.com/velodrome-finance/contracts/blob/80dd33824a51fd2b2162311ac3c44512e151bcff/contracts/VotingEscrow.sol#L1376

Tool used

Manual Review

Recommendation

Update the move token delegate logic within the affected functions (VotingEscrow._moveTokenDelegates and VotingEscrow._moveAllDelegates) to ensure that the latest checkpoint is overwritten correctly when the functions are triggered more than once within a single block. Further, ensure that the following invariants hold in the new code: • No duplicated veNFTs (tokenIDs) within a checkpoint • When moving a tokenID, it must be deleted from the source tokenIds list and added to the destination tokenIds list • No more than one checkpoint within the same block for an account. Otherwise, the binary search within the VotingEscrow.getPastVotesIndex will return an incorrect number of votes

Duplicate of #228

sherlock-audit / 2024-06-velocimeter-judging

Varun_19 - Wrong voting balance due to duplicated veNFTs within a checkpoint #661