Velodrome Finance contest - kenzo's results

Lines of code

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/VotingEscrow.sol#L517:#L528

Vulnerability details

VotingEscrew _burn function does not remove the token from the token's delegate token list.

Impact

Wrong voting results. Delegate's votes will be inflated.

Proof of Concept

When minting a token, it is added to the owner's delegate using _moveTokenDelegates:

    function _mint(address _to, uint _tokenId) internal returns (bool) {
        assert(_to != address(0));
        _moveTokenDelegates(address(0), delegates(_to), _tokenId);
        _addTokenTo(_to, _tokenId);
        emit Transfer(address(0), _to, _tokenId);
        return true;
    }

_moveTokenDelegates is also called when transferring tokens. But when burning a token, the token is not removed from the delegate's list:

    function _burn(uint _tokenId) internal {
        require(_isApprovedOrOwner(msg.sender, _tokenId), "caller is not owner nor approved");
        address owner = ownerOf(_tokenId);
        approve(address(0), _tokenId);
        // TODO add delegates
        _removeTokenFrom(msg.sender, _tokenId);
        emit Transfer(owner, address(0), _tokenId);
    }

Therefore, the token would stay live at the delegate's token list, and will be counted towards the vote count, althought it is burned.

Recommended Mitigation Steps

Add to the burn function:

        _moveTokenDelegates(delegates(owner), address(0), _tokenId);