Scope

Detailed Findings

[High] Inflated voting balance due to duplicated veNFTs within a checkpoint

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

Recommendation:

Follow https://github.com/velodrome-finance/contracts/commit/a670bfb62c69fe38ac918b734a03b68a5505f8a2#diff-f9c514e7e623e2b874104366a804a87afa0048ad5d7fda1c0d45fbfa7e3941b9

[High] LoxoHolders::buy has the same price for all modes

In the LoxoHolders contract, the buy function is used to handle NFT purchases for different sale statuses (Private Sale, WhiteList Sale, and Public Sale). However, the function uses the same NFT_PRICE variable to check the amount of Ether sent for all sale statuses. Since each sale status has a different NFT price (0.295 ETH for Private Sale, 0.325 ETH for WhiteList Sale, and 0.35 ETH for Public Sale), it is not possible to distinguish which sale status the user is participating in when the prices are the same.

// contracts/LoxoHolders.sol
function buy(uint256 amount, bytes32[] memory proof) public payable {
    require(presale != Status.Stop, 'Presale isnt available at this moment');
    if(presale == Status.PrivateSale){
        // Private mint price: 0.295 ETH and 5 NFTs per wallet
        require(NFT_PRICE.mul(amount) == msg.value, "ETH value sent is not correct");
        require(privateSale[msg.sender].add(amount) <= MAX_PER_MINT, "exceed maximum amount");

        privateSale[msg.sender] = privateSale[msg.sender].add(amount);
        originalMinters[msg.sender] = originalMinters[msg.sender].add(amount);
        _mintTo(msg.sender, amount);
    }if(presale == Status.WhiteListSale){
        // Whitelisted mint price: 0.325 ETH and 10 NFTs per wallet
        require(verifyLeaf(proof, msg.sender), "Not whitelisted.");
        require(NFT_PRICE.mul(amount) == msg.value, "ETH value sent is not correct");
        require(whiteListSale[msg.sender].add(amount) <= MAX_PER_MINT, "exceed maximum amount");

        whiteListSale[msg.sender] = whiteListSale[msg.sender].add(amount);
        originalMinters[msg.sender] = originalMinters[msg.sender].add(amount);
        _mintTo(msg.sender, amount);
    }else if(presale == Status.PublicSale){
        // Public mint price: 0.35 ETH and 10 NFTs per wallet
        require(NFT_PRICE.mul(amount) == msg.value, "ETH value sent is not correct");
        require(publicSale[msg.sender].add(amount) <= MAX_PER_MINT, "exceed maximum amount");

        publicSale[msg.sender] = publicSale[msg.sender].add(amount);
        originalMinters[msg.sender] = originalMinters[msg.sender].add(amount);
        _mintTo(msg.sender, amount);
    }
}

Recommendation:

In the buy function, set different prices based on the current presale status instead of using the same NFT_PRICE variable.