KingNFT - ````VotingEscrow._moveTokenDelegates()```` would work abnormally while called more than once for same ````Rep```` in one block

[sherlock-admin3]

KingNFT

Medium

VotingEscrow._moveTokenDelegates() would work abnormally while called more than once for same Rep in one block

Summary

The current implementation of VotingEscrow._moveTokenDelegates() can not process the situation that a Rep's delegated NFT list is changed more than once in one block.

Vulnerability Detail

Please pay attention on L1373 and L1423, the nextSrcRepNum would be _nCheckPoints - 1 if a second try in one block (timestamp keeps same) to update checkpoints for same Rep, which results in srcRepOld and srcRepNew points to the same storage. Then, the for loop for coping token IDs (L1378~1383) becomes flawed. As srcRepOld.length (L1378) increases by 1 after each push(), which incurs dead loop and burn all available gas.

File: contracts\VotingEscrow.sol
1362:     function _moveTokenDelegates(
1363:         address srcRep,
1364:         address dstRep,
1365:         uint _tokenId
1366:     ) internal {
...
1369:                 uint32 srcRepNum = numCheckpoints[srcRep];
1370:                 uint[] storage srcRepOld = srcRepNum > 0
1371:                     ? checkpoints[srcRep][srcRepNum - 1].tokenIds
1372:                     : checkpoints[srcRep][0].tokenIds;
1373:                 uint32 nextSrcRepNum = _findWhatCheckpointToWrite(srcRep);
1374:                 uint[] storage srcRepNew = checkpoints[srcRep][
1375:                     nextSrcRepNum
1376:                 ].tokenIds;
1377:                 // All the same except _tokenId
1378:                 for (uint i = 0; i < srcRepOld.length; i++) {
1379:                     uint tId = srcRepOld[i];
1380:                     if (tId != _tokenId) {
1381:                         srcRepNew.push(tId);
1382:                     }
1383:                 }
...
1411:     }

File: contracts\VotingEscrow.sol
1413:     function _findWhatCheckpointToWrite(address account)
1414:         internal
1415:         view
1416:         returns (uint32)
1417:     {
...
1421:         if (
1422:             _nCheckPoints > 0 &&
1423:             checkpoints[account][_nCheckPoints - 1].timestamp == _timestamp
1424:         ) {
1425:             return _nCheckPoints - 1;
1426:         } else {
1427:             return _nCheckPoints;
1428:         }
1429:     }

Impact

Transactions from users with same Reps would fail unexpectedly and all available gas is burned.

Code Snippet

https://github.com/sherlock-audit/2024-06-velocimeter/blob/63818925987a5115a80eff4bd12578146a844cfd/v4-contracts/contracts/VotingEscrow.sol#L1362

Tool used

Manual Review

Recommendation

Adding a special process logic for this situation

Duplicate of #228