zraxx - When `delegateMgCvg` is used for update or remove, it will be reverted due to improper require checks.

[sherlock-admin2]

zraxx

medium

When delegateMgCvg is used for update or remove, it will be reverted due to improper require checks.

Summary

When delegateMgCvg is used for update or remove, it will be reverted due to improper require checks.

Vulnerability Detail

In function delegateMgCvg from LockingPositionDelegate.sol, there are two require checks to prevent the array length from exceeding the setting.

uint256 _delegateesLength = delegatedMgCvg[_tokenId].length;
require(_delegateesLength < maxMgDelegatees, "TOO_MUCH_DELEGATEES");

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

These two checks are correct when adding delegation. However, when encountering the situation of updating or deleting the delegate, these two checks are too strict, because updating the delegate will not change the array length, while deleting the delegate will reduce the array length.

Please consider this situation, that is, if the user wants to update delegatedMgCvg[_tokenId] when delegatedMgCvg[_tokenId].length is exactly equal to maxMgDelegatees, he or she will have to call cleanDelegatees first to remove all delegatees and then add them one by one, which is extremely cumbersome. Also, it will consume much more gas. I believe this issue affects normal functionality, albeit less severely.

Impact

Normal functions are limited, which causes users spend much more gas.

Code Snippet

https://github.com/sherlock-audit/2023-11-convergence/blob/main/sherlock-cvg/contracts/Locking/LockingPositionDelegate.sol#L278

Tool used

Manual Review

Recommendation

Check the array length based on the value of _isUpdate

function delegateMgCvg(uint256 _tokenId, address _to, uint96 _percentage) external onlyTokenOwner(_tokenId) {
        require(_percentage <= 100, "INVALID_PERCENTAGE");

        uint256 _delegateesLength = delegatedMgCvg[_tokenId].length;
        uint256 tokenIdsDelegated = mgCvgDelegatees[_to].length;

        (uint256 _toPercentage, uint256 _totalPercentage, uint256 _toIndex) = getMgDelegateeInfoPerTokenAndAddress(
            _tokenId,
            _to
        );
        bool _isUpdate = _toIndex != 999;

        if (_isUpdate) {
            require(_delegateesLength <= maxMgDelegatees, "TOO_MUCH_DELEGATEES");
            require(tokenIdsDelegated <= maxTokenIdsDelegated, "TOO_MUCH_MG_TOKEN_ID_DELEGATED");
        }
        else {
            require(_delegateesLength < maxMgDelegatees, "TOO_MUCH_DELEGATEES");
            require(tokenIdsDelegated < maxTokenIdsDelegated, "TOO_MUCH_MG_TOKEN_ID_DELEGATED");
        }

        // ......

[nevillehuang]

Invalid, spending additional gas is not considered an acceptable finding based on sherlock rules

1. Gas optimizations: The user/protocol ends up paying a little extra gas because of this issue.