Inflated voting balance due to duplicated veNFTs within a checkpoint

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0x38af4af798c80712ca428f4d6bd7ad29d16c29c34a102ceb271080a582951667 Severity: high

Description: Description

Note: This issue affects VotingEscrow._moveTokenDelegates and VotingEscrow._moveAllDelegates functions

Attack Scenario\ A checkpoint can contain duplicated veNFTs (tokenIDs) under certain circumstances leading to double counting of voting balance. Malicious users could exploit this vulnerability to inflate the voting balance of their accounts and participate in governance and gauge weight voting, potentially causing loss of assets or rewards for other users if the inflated voting balance is used in a malicious manner (e.g. redirect rewards to gauges where attackers have a vested interest). Following is the high-level pseudo-code of the existing _moveTokenDelegates function, which is crucial for understanding the issue.

1. Assuming moving tokenID=888 from Alice to Bob.
2. Source Code Logic (Moving tokenID=888 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=888
3. Destination Code Logic (Moving tokenID=888 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=888 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(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;

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.

    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 1000

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 1000 (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:

1. The numCheckpoints is incorrect. Should be two (2) instead to three (3)
2. TokenID=1 has been added to Bob's Checkpoint, but it has not been removed from Alice's Checkpoint
3. Bob's Checkpoint contains duplicated tokenIDs (e.g. there are two TokenID=2 and TokenID=3)
4. 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

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

Additional Comment about nextSrcRepNum variable and _findWhatCheckpointToWrite function

the code wrongly assumes that the _findWhatCheckpointToWrite function will always return the index of the next new checkpoint. The _findWhatCheckpointToWrite function will return the index of the latest checkpoint instead of a new one if block.number == checkpoint.fromBlock.

    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;

Additional Comment about numCheckpoints

the function computes the new number of checkpoints by incrementing the srcRepNum by one. However, this is incorrect because if block.number == checkpoint.fromBlock, then the number of checkpoints remains the same and does not increment.

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

Attachments

1. Proof of Concept (PoC) File

2. 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

[BohdanHrytsak]

Thank you for the submission.

After the tests described in the view, there is indeed a certain logic for displaying tokens in both user1 and user2 if the transfer took place within the same block

But does it affect the votes?

We can see that the vote power is counted correctly.

and do not take into account these issues of duplicates, etc.

The principle of operation looks like with vote counting, when you take votes for a certain period, yes, there are indeed votes, but if there was a change of token owner or something else in a certain block, when you take votes using methods that are designed to take actual votes, it returns the correct value

I would like to request additional clarification from the submitter, namely, a specific case that will lead to a critical case within the protocol.

At the moment, this logic was inherited from Thena, as it is Medium, so OOS

Regarding "transfer but will return the last/previous checkpoint in the second transfer. This will cause the move token delegate" - It will not return the previous checkpoint, since the timestamp is never initialized (other bug)

[sonny2k]

Thank you for reviewing my submission!

Due to the bug of "timestamp variable is never initialized", the _findWhatCheckpointToWrite function will always create and return a new checkpoint, which make this issue not happening as described. Please view this issue supposing the timestamp variable is correctly intitialized. In the mean time, kindly refer to the mitigation which velodrome use to fix this issue. And to mention about a critical case within the protocol, as the title says, it definitely about manipulating voting power within the protocol.

[BohdanHrytsak]

In the severity of this submission, I am more inclined to medium. Taking into account the way VotingEscrow is used and what methods are taken into account to obtain votes, I came to the conclusion (tests) that although this issue is valid, it does not pose risks to the methods used to obtain the value of the strength of the vote (balanceOfNFT) in the Voter.

It does not jeopardize the funds or manipulation of votes that will allow them to be used in the state in which the protocol is now.

Since the actual calculations and state variables of some methods that are present are broken, which makes their work incorrect, these methods are not called anywhere, which does not pose a threat to funds in the state of the protocol as it is (Medium).

Since there is a lack of criticality (direct impact on funds), OOS remains due to inheritance from Thena/Chronos from the beginning

Would like to see how this problem is used to extract/receive additional funds in the system in specific case

[sonny2k]

Thank you for replying!

After looking carefully at the code, it seems like this inflating issue may only affect the DAO Governance voting. As I observed from Fenix Finance docs, You guys state:

And from Thena:

Since the code will be inherited from Thena, there is possibly a chance that you will set up a governance for the protocol. If that is the case, then you may have to re-consider this issue if you don't want the governance to be manipulated by some malicious users with inflated voting power from their $veFNX.

When implementing the DAO, these functions such as VotingEscrowUpgradeable.getVotes() and VotingEscrowUpgradeable.getPastVotes() will be used to calculated the votes count of a proposal. This is where the issue happens.

With the ability to inflate voting balance, I want to change the severity of this issue to Medium instead of ~High~:

• Impact: High, as it breaks the current votes of a particular account

• Likelihood: Low, as the protocol may or may not implement a DAO governance within the protocol although malicious users can cast extra votes at any time

[BohdanHrytsak]

It remains OOS due to the lack of sufficient impact.

As we can see, at the moment there are only theoretical problems / in the future or in connection with the integration of these methods.

And the impact is the incorrect return voting power/list of the above methods, but since it is not used anywhere in contracts at the moment, it does not cause an impact on real funds and sufficient criticality, so OOS