radin200 - DoS if approved address tries to harvest multiple positions with MlumStaking::harvestPositionsTo

[sherlock-admin3]

radin200

High

DoS if approved address tries to harvest multiple positions with MlumStaking::harvestPositionsTo

Summary

DoS if approved address tries to harvest multiple positions with MlumStaking::harvestPositionsTo, due to bad validation

Vulnerability Detail

When approved to tokenIds user(not owner), calls MlumStaking::harvestPositionsTo, he is going to face "FORBIDDEN" revert, due to require((msg.sender == tokenOwner && msg.sender == to),"FORBIDDEN");, that always requires msg.sender == tokenOwner, which is not intended as we can see from the comment Can only be called by lsNFT's owner or approved address

/**
     * @dev Harvest from multiple staking positions to "to" address
     *
     * Can only be called by lsNFT's owner or approved address
     */
function harvestPositionsTo(uint256[] calldata tokenIds, address to) external override nonReentrant {
        _updatePool();

        uint256 length = tokenIds.length;

        for (uint256 i = 0; i < length; ++i) {
            uint256 tokenId = tokenIds[i];
            _requireOnlyApprovedOrOwnerOf(tokenId);
            address tokenOwner = ERC721Upgradeable.ownerOf(tokenId);
            // if sender is the current owner, must also be the harvest dst address
            // if sender is approved, current owner must be a contract
            require(
                (msg.sender == tokenOwner && msg.sender == to), // legacy || tokenOwner.isContract()
                "FORBIDDEN"
            );

            _harvestPosition(tokenId, to);
            _updateBoostMultiplierInfoAndRewardDebt(_stakingPositions[tokenId]);
        }
    }

Impact

DoS when approved address tries to harvest multiple positions

Code Snippet

https://github.com/sherlock-audit/2024-06-magicsea/blob/42e799446595c542eff9519353d3becc50cdba63/magicsea-staking/src/MlumStaking.sol#L465-L489

Tool used

Manual Review

Recommendation

function harvestPositionsTo(uint256[] calldata tokenIds, address to) external override nonReentrant {
        _updatePool();

        uint256 length = tokenIds.length;

        for (uint256 i = 0; i < length; ++i) {
            uint256 tokenId = tokenIds[i];
            _requireOnlyApprovedOrOwnerOf(tokenId);
            address tokenOwner = ERC721Upgradeable.ownerOf(tokenId);
            // if sender is the current owner, must also be the harvest dst address
            // if sender is approved, current owner must be a contract
---         require(
---             (msg.sender == tokenOwner && msg.sender == to), // legacy || tokenOwner.isContract()
---             "FORBIDDEN"
---         );
+++         if((msg.sender == tokenOwner && msg.sender != to) && tokenOwner.code.length == 0) revert("FORBIDDEN");

            _harvestPosition(tokenId, to);
            _updateBoostMultiplierInfoAndRewardDebt(_stakingPositions[tokenId]);
        }
    }

Duplicate of #329