Mansa11 - User can bypass gauge detachment through withdrawals

[sherlock-admin4]

Mansa11

Medium

User can bypass gauge detachment through withdrawals

Summary

A user trying to withdraw all his balance can bypass the invariant which states that such tokenId should be detached from the gauge and leads to a bypass of the contract logic and also other impacts.

Referenced Link

https://github.com/sherlock-audit/2024-06-velocimeter/blob/main/v4-contracts/contracts/GaugeV4.sol#L513-L555

Vulnerability Detail

There are numerous ways by which a user can withdraw their tokens like withdrawAll and withdraw. This two functions are very similar only that the withdrawAll is meant to be called when a user decides to withdraw all his balances, while the withdraw function is used when he wishes to withdraw some portions of his total balance. However, it should be noted that when the user is withdrawing their entire balance i.e balanceOf[msg.sender] the protocol has a design choice to detach the user token from the gauge, using this internal method IVoter(voter).detachTokenFromGauge(tokenId, msg.sender); so as not to enable the said user to continue earning from the rewards streams from the gauge.

The above invariant has been found to be broken due to a little oversight, which allows the user to withdraw his total balance without being detached from the gauge and can be said to be a break in protocols logic.

This break in logic is possible due to the main implementation in which contains the withdrawal logic withdrawToken that is being called by both the withdraw and the withdrawAll functions having a public visibility.

function withdrawToken(uint amount, uint tokenId) public lock {
        ...

        if (tokenId > 0) { 
            require(tokenId == tokenIds[msg.sender]);
            tokenIds[msg.sender] = 0;
            IVoter(voter).detachTokenFromGauge(tokenId, msg.sender);  //@audit bypass
        } else {
            tokenId = tokenIds[msg.sender];
        }

       ...
    }

Making the function having a public visibility makes it easy for a user to override the desired contract implementations and allows a user who wants to withdraw his entire balance to pass in a tokenId of 0 which will skip this check if (tokenId > 0) in the withdrawToken function.

Impact

User can still be earning benefits from the gauge without having any balances

Code Snippet

function withdrawToken(uint amount, uint tokenId) public lock {
        _updateRewardForAllTokens();

        uint256 totalBalance = balanceOf[msg.sender];
        uint256 lockedAmount = balanceWithLock[msg.sender];
        uint256 freeAmount = totalBalance - lockedAmount;
        // Update lock related mappings when withdraw amount greater than free amount
        if (amount > freeAmount) {
            // Check if lock has expired
            require(block.timestamp >= lockEnd[msg.sender], "The lock didn't expire");
            uint256 newLockedAmount = totalBalance - amount; /// 
            if (newLockedAmount == 0) {
                delete lockEnd[msg.sender];
                delete balanceWithLock[msg.sender];
            } else {
                balanceWithLock[msg.sender] = newLockedAmount;
            }
        }

        totalSupply -= amount;
        balanceOf[msg.sender] -= amount;
        _safeTransfer(stake, msg.sender, amount);

        if (tokenId > 0) { //@audit this function is a public one taht accepts user specified token,  a user can decide to pass in tokenId as zero even when withdrawing all tokens, which breaks the invariant of detaching a token from gauge whenever the user specifies an amount == balanceOf[msg.sender] impact: if the gauge is still present, he can be earing some incentives fix: make this function internal or restructure the logic
            require(tokenId == tokenIds[msg.sender]);
            tokenIds[msg.sender] = 0;
            IVoter(voter).detachTokenFromGauge(tokenId, msg.sender); 
        } else {
            tokenId = tokenIds[msg.sender];
        }

        uint _derivedBalance = derivedBalances[msg.sender];
        derivedSupply -= _derivedBalance;
        _derivedBalance = derivedBalance(msg.sender); 
        derivedBalances[msg.sender] = _derivedBalance;
        derivedSupply += _derivedBalance;

        _writeCheckpoint(msg.sender, derivedBalances[msg.sender]);
        _writeSupplyCheckpoint();

        IVoter(voter).emitWithdraw(tokenId, msg.sender, amount);
        emit Withdraw(msg.sender, tokenId, amount);
    }

Tool used

Manual Review

Recommendation

The function should be made to have a visibility of internal.

Duplicate of #460