pontifex - Unexpected revert at the `delegateMgCvg` and `delegateVeCvg` when delegation removal

[sherlock-admin2]

pontifex

medium

Unexpected revert at the delegateMgCvg and delegateVeCvg when delegation removal

Summary

Token owners can't remove a percentage of the mgCvG delegation from a specified address _to via the delegateMgCvg function when the _to address has maxTokenIdsDelegated delegations. The only way to remove delegation is to clean all associated with _tokenId delegatees. The same issue at the delegateVeCvg function but the impact is less because veCVG can be delegated only to one address.

Vulnerability Detail

The LockingPositionDelegate.delegateMgCvg and LockingPositionDelegate.delegateVeCvg functions allow the token owner to delegate and undelegate to the selected address. If the number of delegations to the address reaches the maxTokenIdsDelegated value, no more delegations can be performed at this address. Due to the fact that the checks are in inappropriate places, they also prevent the cancellation of delegation from such addresses.

Impact

The delegateMgCvg and delegateVeCvg functions do not work as expected during the normal usage.

Code Snippet

https://github.com/sherlock-audit/2023-11-convergence/blob/e894be3e36614a385cf409dc7e278d5b8f16d6f2/sherlock-cvg/contracts/Locking/LockingPositionDelegate.sol#L285 https://github.com/sherlock-audit/2023-11-convergence/blob/e894be3e36614a385cf409dc7e278d5b8f16d6f2/sherlock-cvg/contracts/Locking/LockingPositionDelegate.sol#L249

Tool used

Manual Review

Recommendation

Consider using this check only for a new delegatee. delegateMgCvg:

 284        uint256 tokenIdsDelegated = mgCvgDelegatees[_to].length;
-285        require(tokenIdsDelegated < maxTokenIdsDelegated, "TOO_MUCH_MG_TOKEN_ID_DELEGATED");

 299        /** @dev Delegating.*/
 300        if (_percentage > 0) {
 301            MgCvgDelegatee memory delegatee = MgCvgDelegatee({delegatee: _to, percentage: _percentage});
 302
 303            /** @dev Updating delegatee.*/
 304            if (_isUpdate) {
 305                delegatedMgCvg[_tokenId][_toIndex] = delegatee;
 306            } else {
 307                /** @dev Adding new delegatee.*/
+                   require(tokenIdsDelegated < maxTokenIdsDelegated, "TOO_MUCH_MG_TOKEN_ID_DELEGATED");
 308                delegatedMgCvg[_tokenId].push(delegatee);
 309                mgCvgDelegatees[_to].push(_tokenId);
 310            }
 311        } else {

delegateVeCvg:

    function delegateVeCvg(uint256 _tokenId, address _to) external onlyTokenOwner(_tokenId) {
-       require(veCvgDelegatees[_to].length < maxTokenIdsDelegated, "TOO_MUCH_VE_TOKEN_ID_DELEGATED");
        /** @dev Find if this tokenId is already delegated to an address. */
        address previousOwner = delegatedVeCvg[_tokenId];
        if (previousOwner != address(0)) {
            /** @dev If it is  we remove the previous delegation.*/
            uint256 _toIndex = getIndexForVeDelegatee(previousOwner, _tokenId);
            uint256 _delegateesLength = veCvgDelegatees[previousOwner].length;
            /** @dev Removing delegation.*/
            veCvgDelegatees[previousOwner][_toIndex] = veCvgDelegatees[previousOwner][_delegateesLength - 1];
            veCvgDelegatees[previousOwner].pop();
        }

        /** @dev Associate tokenId to a new delegated address.*/
        delegatedVeCvg[_tokenId] = _to;

        if (_to != address(0)) {
            /** @dev Add delegation to the new address.*/
+           require(veCvgDelegatees[_to].length < maxTokenIdsDelegated, "TOO_MUCH_VE_TOKEN_ID_DELEGATED");            
            veCvgDelegatees[_to].push(_tokenId);
        }
        emit DelegateVeCvg(_tokenId, _to);

Duplicate of #142