cryptic - `VotingEscrow::_findWhatCheckpointToWrite` returns an incorrect index in some cases since `Checkpoint timestamp` is never set

[sherlock-admin2]

cryptic

Medium

VotingEscrow::_findWhatCheckpointToWrite returns an incorrect index in some cases since Checkpoint timestamp is never set

Summary

Users can delegate their veNFT to anyone, which transfers the balance of their locked veNFT amount to them. This is done via a call to _moveTokenDelegates.

_moveTokenDelegates utilizes another function _findWhatCheckpointToWrite, which determines the index of the checkpoint to update. Each checkpoint stores the state of the user's veNFTs at that point in time.

_findWhatCheckpointToWrite is supposed to return the index of the previous checkpoint index when the checkpoint timestamp is equal to block.timestamp.

However, the checkpoint timestamp is never initialized, causing _findWhatCheckpointToWrite to incorrectly return a new index each time. This will impact all transfer/mint/burn/delegate calls.

Vulnerability Detail

The following function is executed when NFTs are delegated:

VotingEscrow.sol#L1362-L1411

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

            if (dstRep != address(0)) {
                uint32 dstRepNum = numCheckpoints[dstRep];
                uint[] storage dstRepOld = dstRepNum > 0
                    ? checkpoints[dstRep][dstRepNum - 1].tokenIds
                    : checkpoints[dstRep][0].tokenIds;
@>              uint32 nextDstRepNum = _findWhatCheckpointToWrite(dstRep);
                uint[] storage dstRepNew = checkpoints[dstRep][
                    nextDstRepNum
                ].tokenIds;
                // All the same plus _tokenId
                require(
                    dstRepOld.length + 1 <= MAX_DELEGATES,
                    "dstRep would have too many tokenIds"
                );
                for (uint i = 0; i < dstRepOld.length; i++) {
                    uint tId = dstRepOld[i];
                    dstRepNew.push(tId);
                }
                dstRepNew.push(_tokenId);

                numCheckpoints[dstRep] = dstRepNum + 1;
            }
        }
    }

VotingEscrow.sol#L1413-L1429

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

We can see if the latest checkpoint's timestamp is the same as the current block.timestamp, then the old checkpoint index is returned. However, the timestamp for checkpoints are never updated, therefore this will always return a new checkpoint index instead of the old one.

Proof of Concept

Coded PoC

Accessing the `checkpoints` mapping caused some issues, so to avoid them I added the following to `VotingEscrow`: ```javascript function getCheckpoint(address owner, uint32 index) public view returns (Checkpoint memory) { return checkpoints[owner][index]; } ``` Add the following to `test/VotingEscrow.t.sol` and run `forge test --mt testTimestampNotSet -vv` ```javascript function testTimestampNotSet() public { uint256 maxtime = 52 * 7 * 24 * 3600; // 52 weeks FLOW.mint(address(owner), TOKEN_1M); DAI.mint(address(owner), TOKEN_1M); FLOW.approve(address(router), TOKEN_1M); DAI.approve(address(router), TOKEN_1M); router.addLiquidity(address(FLOW), address(DAI), false, TOKEN_1M, TOKEN_1M, 0, 0, address(owner), block.timestamp); address alice = vm.addr(1); vm.startPrank(address(owner)); flowDaiPair.transfer(alice, 2 ether); flowDaiPair.approve(address(escrow), type(uint256).max); uint tokenId = escrow.create_lock(TOKEN_1, maxtime); vm.roll(block.number + 1); vm.warp(block.timestamp + 2); vm.stopPrank(); vm.startPrank(address(alice)); flowDaiPair.approve(address(escrow), type(uint256).max); uint tokenId2 = escrow.create_lock(1 ether, maxtime); vm.roll(block.number + 1); vm.warp(block.timestamp + 2); uint tokenId3 = escrow.create_lock(1 ether, maxtime); vm.roll(block.number + 1); vm.warp(block.timestamp + 2); escrow.approve(address(owner), tokenId2); escrow.approve(address(owner), tokenId3); vm.stopPrank(); escrow.transferFrom(address(alice), address(owner), tokenId2); vm.warp(block.timestamp + 2); uint32 nCheckpoints = escrow.numCheckpoints(address(owner)); VotingEscrow.Checkpoint memory cp = escrow.getCheckpoint(address(owner), nCheckpoints - 1); console.log("Current block.timestamp: ", uint(block.timestamp)); console.log("Timestamp of checkpoint: ", cp.timestamp); } ```

Console Output

Running 1 test for test/VotingEscrow.t.sol:VotingEscrowTest
[PASS] testTimestampNotSet() (gas: 1596642)
Logs:
  Current block.timestamp:  9
  Timestamp of checkpoint:  0

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 5.25ms

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Impact

Incorrect checkpoint when previous checkpoint == block.timestamp, impacting transfer/mint/burn/delegate functions and possibly storing incorrect voting power (i.e., lower power) for specific checkpoints.

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

Tool used

Manual Review

Recommendation

Update the timestamp of the checkpoint when it is updated.

Duplicate of #288