StraawHaat - A malicious user can create duplicated `veNFTs` within a checkpoint when the `VotingEscrow._moveTokenDelegates` function is called multiple times within the same block

[sherlock-admin4]

StraawHaat

High

A malicious user can create duplicated veNFTs within a checkpoint when the VotingEscrow._moveTokenDelegates function is called multiple times within the same block

Summary

A malicious user can create duplicated veNFTs within a checkpoint when the VotingEscrow._moveTokenDelegates function is called multiple times within the same block. This duplication can lead to inflated voting balances, which can be exploited for malicious purposes.

Note: the same vulnerability exists in VotingEscrow._moveAllDelegates.

Vulnerability Detail

Note: This vulnerability is inspired by this issue from Spearbit: https://solodit.xyz/issues/inflated-voting-balance-due-to-duplicated-venfts-within-a-checkpoint-spearbit-none-velodrome-finance-pdf

The _moveTokenDelegates() function handles the delegation of voting power associated with NFT. When tokens (veNFTs) are transferred from one account to another, the function updates the voting power of the involved accounts.

The function finds the correct checkpoint for the current block using _findWhatCheckpointToWrite():

uint32 nextSrcRepNum = _findWhatCheckpointToWrite(srcRep);

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

The problem is that the function can called multiple times within the same block. This allows users to create duplicated veNFTs (token IDs) within a checkpoint. This duplication can lead to inflated voting balances, which can be exploited for malicious purposes.

Consider the following situation:

Initial State:

• Assume Alice owns token IDs [n1, n2].
• Bob owns token ID [n3]

First Transfer within Block:

• Alice transfers n2 to Bob.
• The _moveTokenDelegates(Alice, Bob, n2) function is called:
  • It creates a new checkpoint for Alice, removing n2.
  • It creates a new checkpoint for Bob, adding n2.
• The state after the first transfer:
  • Checkpoints for Alice: [n1].
  • Checkpoints for Bob: [n3, n2].

Second Transfer within the Same Block:

• Alice transfers n1 to Bob.
• The _moveTokenDelegates(Alice, Bob, n1) function is called again within the same block:
  • The _findWhatCheckpointToWrite function returns the index of the last checkpoint because it was created within the same block.
  • This leads to both Alice's and Bob's latest checkpoints being updated incorrectly.
• The state after the second transfer:
  • Checkpoints for Alice: [n1] (incorrectly retains n1 because the new checkpoint wasn't created properly).
  • Checkpoints for Bob: [n3, n2, n2, n3, n1] (contains duplicates and incorrect data).

    Impact

Malicious users can manipulate their voting power by duplicating veNFTs within the same block. This can affect governance decisions and reward distributions.

Code Snippet

https://github.com/sherlock-audit/2024-06-velocimeter/blob/main/v4-contracts/contracts/VotingEscrow.sol#L1362-L1411 https://github.com/sherlock-audit/2024-06-velocimeter/blob/main/v4-contracts/contracts/VotingEscrow.sol#L1413-L1429 https://github.com/sherlock-audit/2024-06-velocimeter/blob/main/v4-contracts/contracts/VotingEscrow.sol#L1431-L1486

Tool used

Manual Review

Recommendation

The _moveTokenDelegates() and _moveAllDelegates() function need to ensure that checkpoints are managed correctly, and tokens are not duplicated within the same block.

Duplicate of #228