Approved users cannot merge tokens for owners

Summary

To merge 2 tokens into 1, a user must be either be approved or the owner of both tokens. This is obvious in the checks in merge(...) function

Vulnerability Detail

The merge function has a check to ensure that a third party user has approval to merge any two tokens. And then it cals the _burn(..) function to burn the _from token.


1195:     function merge(uint _from, uint _to) external { 
1196:  >>     require(attachments[_from] == 0 && !voted[_from], "attached");
1197:         require(_from != _to);
1198: >>      require(_isApprovedOrOwner(msg.sender, _from)); 
....
1208:  >>     _burn(_from);
1209:         _deposit_for(_to, value0, end, _locked1, DepositType.MERGE_TYPE);
1210:     }

537:     function _burn(uint _tokenId) internal {
538:         require(_isApprovedOrOwner(msg.sender, _tokenId), "caller is not owner nor approved");
.........
542:         // Clear approval
543: >>>     approve(address(0), _tokenId);
....
548:         emit Transfer(tokenOwner, address(0), _tokenId);
549:     }

However, the _burn(...) function clears the approval of the _from token. It does this by calling approve(address(0), _tokenId) on the _from token which checks if the caller is the owner or approved for all, but it doesn't allow "regular" approved users to perform this task and hence the call reverts.

CODED POC

Add the test case below to the VotingEscrow.t.sol file and run forge test --mt testApprovedMergeReverts -vvv

    address dan = address(0x01);

    function testApprovedMergeReverts() public {
        flowDaiPair.approve(address(escrow), TOKEN_1);
        uint256 lockDuration = 7 * 24 * 3600; // 1 week

        // Balance should be zero before and 1 after creating the lock
        assertEq(escrow.balanceOf(address(owner)), 0);
        uint token1 = escrow.create_lock(TOKEN_1/2, lockDuration);
        uint token2 = escrow.create_lock(TOKEN_1/2, lockDuration);

        vm.prank(address(owner));
        escrow.approve(dan, token1);

        vm.prank(dan);
        vm.expectRevert();
        escrow.merge(token1, token2);
    }

Impact

Approved users cannot merger tokens if they are not approved for all and this breaks core protocol functionality and also leads to a DOS.

Code Snippet

https://github.com/sherlock-audit/2024-06-velocimeter/blob/main/v4-contracts/contracts/VotingEscrow.sol#L1195-L1210 https://github.com/sherlock-audit/2024-06-velocimeter/blob/main/v4-contracts/contracts/VotingEscrow.sol#L537-L543

Tool used

Foundry test

Recommendation

In the _burn(...) function replace approve(address(0), _tokenId) with _clearApproval(owner, _tokenId) as shown below

537:     function _burn(uint _tokenId) internal {
538:         require(_isApprovedOrOwner(msg.sender, _tokenId), "caller is not owner nor approved");
.........
542:         // Clear approval
-543:         approve(address(0), _tokenId);
+543:         _clearApproval(owner, _tokenId);
....
548:         emit Transfer(tokenOwner, address(0), _tokenId);
549:     }

Duplicate of #42

sherlock-audit / 2024-06-velocimeter-judging

Audinarey - Approved users cannot merge tokens for owners #564