Chinmay - Merging a max locked nft will always revert

[sherlock-admin3]

Chinmay

Medium

Merging a max locked nft will always revert

Summary

Merge function is meant to facilitate joing together two locked NFT positions.

But if the 'from' NFT has max_lock enabled then the call will revert during burning.

Vulnerability Detail

This is the merge function :

    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));
        uint end = _locked0.end >= _locked1.end ? _locked0.end : _locked1.end;

        locked[_from] = LockedBalance(0, 0);
        _checkpoint(_from, _locked0, LockedBalance(0, 0));
        _burn(_from);
        _deposit_for(_to, value0, end, _locked1, DepositType.MERGE_TYPE);
    }

We can see that it first sets locked[] struct data to zero, and then later calls _burn on the tokenID.

The flow in _burn is : _burn() => isApprovedOrOwner() => max_lock()

Now max_lock reverts when the locked.end <= block.timestamp because it doesn't allow expired locks to be inetarcted with (which creates a different problem reported separately).

In the above merge logic, we are first setting locked[_from] == LockedBalance(0, 0) which means the locked.end will also be zero => which leads to a revert inside the _burn => max_lock logic.

This will prevent max lock enabled nfts from being merged.

Impact

Max locked nfts will be prevented from merging due to faulty logic.

Code Snippet

https://github.com/sherlock-audit/2024-06-velocimeter/blob/main/v4-contracts/contracts/VotingEscrow.sol#L1206-L1208

Tool used

Manual Review

Recommendation

In merge, change the order of operations to first _burn and then set locked[_from] == LockedBalance(0, 0)

[nevillehuang]

Invalid, user can simply disable max lock before merging in which case it will acheive the same outcome of allowing a max merging lock of 52 weeks. Also, we wouldn't want to allow max lock period to exceed 52 weeks

[nevillehuang]

request poc

[sherlock-admin4]

PoC requested from @chinmay-farkya

Requests remaining: 20