0xNazgul - `VotingEscrow` Can Have Inflated Supply

[sherlock-admin3]

0xNazgul

Medium

VotingEscrow Can Have Inflated Supply

Summary

The supply storage variable can be inflated due to the merge function not properly updating it. Any function that would rely on this would have unexpected behavior.

Vulnerability Detail

When a user goes to split there NFT in VotingEscrow it will:

1. First do some checks if the user is allowed and checks their input variables
2. Call _remove_from to remove the old data from the initial NFT and supply
3. Call _mint to create the new NFT

4. Finally call _deposit_for to update for the new NFT

   function split(uint _tokenId,uint amount) external {
   
       // check permission and vote
       require(attachments[_tokenId] == 0 && !voted[_tokenId], "attached");
       require(_isApprovedOrOwner(msg.sender, _tokenId));
       require(!blockedSplit[_tokenId],"split blocked");
   
       // save old data and totalWeight
       address _to = idToOwner[_tokenId];
       LockedBalance memory _locked = locked[_tokenId];
       uint end = _locked.end;
       uint value = uint(int256(_locked.amount));
       require(value > amount,"amount > value");
   
       // save end
       uint unlock_time = end;
       require(unlock_time > block.timestamp, 'Can only lock until time in the future');
       require(unlock_time <= block.timestamp + MAXTIME, 'Voting lock can be 52 weeks max');
   
       // remove old data
       _remove_from(_tokenId, amount, unlock_time, _locked);
   
       // mint 
       ++tokenId;
       uint _newTokenId = tokenId;
       _mint(_to, _newTokenId);
       _deposit_for(_newTokenId, amount, unlock_time, locked[_newTokenId], DepositType.SPLIT_TYPE);
   }

   When a user goes to merge their two NFTs if will do the following:

5. First do some checks if the user is allowed and checks their from and to NFT data
6. Call _burn to remove the _from NFT

7. Finally call _deposit_for to update for the _to NFT

   function merge(uint _from, uint _to) external {
       require(attachments[_from] == 0 && !voted[_from], "attached");
       require(_from != _to);
       require(_isApprovedOrOwner(msg.sender, _from));
       require(_isApprovedOrOwner(msg.sender, _to));
   
       LockedBalance memory _locked0 = locked[_from];
       LockedBalance memory _locked1 = locked[_to];
       uint value0 = uint(int256(_locked0.amount));//locked0 amount
       uint end = _locked0.end >= _locked1.end ? _locked0.end : _locked1.end;
   
       locked[_from] = LockedBalance(0, 0);
       _checkpoint(_from, _locked0, LockedBalance(0, 0));
       _burn(_from);//smell burning right one and is end times updating correctly
       _deposit_for(_to, value0, end, _locked1, DepositType.MERGE_TYPE);
   }

   The major difference between the two is that merge does not call _remove_from which is not directly required. However, there is a part of _remove_from that is important, which is updating supply. Thus, merge is only adding to the supply variable with _deposit_for. Leading to these two function flows to not be asymmetric when updating the supply. This would allow anyone to easily inflate the supply variable by:

8. Calling split
9. Calling merge
10. Repeating steps 1 and 2

Impact

Add this POC to VotingEscrow.t.sol:

    function testSplitMerge() public {
        flowDaiPair.approve(address(escrow), 10 * TOKEN_1);
        escrow.create_lock(10 * TOKEN_1, 7 * 24 * 3600);
        vm.roll(block.number + 1); // fwd 1 block because escrow.balanceOfNFT() returns 0 in same block

        uint256[] memory amounts = new uint256[](2);
        amounts[0] = 3 * TOKEN_1;
        amounts[1] = 7 * TOKEN_1;

        escrow.split(1, amounts[1]);

        (int256 amount1, uint256 duration1) = escrow.locked(1);
        (int256 amount2, uint256 duration2) = escrow.locked(2);

        assertEq(amount1, int256(3 * TOKEN_1));
        assertEq(amount2, int256(7 * TOKEN_1));
        assertEq(duration1, duration2);

        flowDaiPair.approve(address(escrow), TOKEN_1);
        uint supplyB = escrow.supply();
        escrow.merge(1, 2);
        uint supplyA = escrow.supply();
        (int256 amount, uint256 duration) = escrow.locked(2);
        assertEq(supplyB, supplyA);
        assertEq(amount, int256(10 * TOKEN_1));
        assertEq(duration, duration2);
    }

Code Snippet

VotingEscrow.sol#L1195

Tool used

Manual Review

Recommendation

Consider properly updated the supply storage variable when merging two NFT tokens.

Duplicate of #214