The recoverRewards may revert when it should not, resulting in losses of rewards not recovered

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0x1fa8659dd2efe45d37c0008b42ac8093589363ff81837b5dd011af77e6e2347e Severity: medium

Description: Description\ The function Metrom.recoverRewards is intented to recover unassigned rewards.

    function recoverRewards(ClaimRewardBundle[] calldata _bundles) external override {
        for (uint256 _i; _i < _bundles.length; _i++) {
            ClaimRewardBundle calldata _bundle = _bundles[_i];

            Campaign storage campaign = _getExistingCampaign(_bundle.campaignId);
            if (msg.sender != campaign.owner) revert Forbidden();

            uint256 _claimedAmount = _processRewardClaim(campaign, _bundle, address(0));
            emit RecoverReward(_bundle.campaignId, _bundle.token, _claimedAmount, _bundle.receiver);
        }
    }

However, the function checks ownership of the campain for every bundle on the array. While the array may have many bundles, if just one owner is not the msg.sender, then the function reverts. It means that the caller will not be able to recover the funds. In fact, none of the bundles is going to be recovered. Even if the user could call the function again, if he keeps trying again and again, he will loose a lot of gas. Instead of reverting, the function should recover the ones that matches ownership.

Attack Scenario\ The function is likely to be denied (DDoS) if one of the owners of any campain in the bundle is not the msg.sender.

Attachments

1. Proof of Concept (PoC) File

2. Revised Code File (Optional)

    function recoverRewards(ClaimRewardBundle[] calldata _bundles) external override {
        for (uint256 _i; _i < _bundles.length; _i++) {
            ClaimRewardBundle calldata _bundle = _bundles[_i];

            Campaign storage campaign = _getExistingCampaign(_bundle.campaignId);
-            if (msg.sender != campaign.owner) revert Forbidden();

-            uint256 _claimedAmount = _processRewardClaim(campaign, _bundle, address(0));
-            emit RecoverReward(_bundle.campaignId, _bundle.token, _claimedAmount, _bundle.receiver);

            if (msg.sender == campaign.owner) {
                uint256 _claimedAmount = _processRewardClaim(campaign, _bundle, address(0));
                emit RecoverReward(_bundle.campaignId, _bundle.token, _claimedAmount, _bundle.receiver);
            }
        }
    }

[luzzif]

This is actually intended behavior and it's the same for all the functions that accept bundles and that allow performing multiple actions at once. I want a transaction to be atomic with bundles and either all the operations succeed or the tx reverts. Specifically here, in case a recover action fails because the caller is not the owner of a campaign, the caller needs to double check how he built the bundle, and in case it's his responsibility to remove the failing bundle from the array.