`QVBaseStrategy::reviewRecipients()` doesn't check if the recipient is already accepted or rejected, and overwrites the current status
In the QV strategy contracts, recipients register themselves and wait for a pool manager to accept the registration. Pool managers can accept or reject recipients with the `reviewRecipients()` function. There is also a threshold (reviewThreshold) for recipients to be accepted. For example, if the `reviewThreshold` is 2, a pending recipient gets accepted when two managers accept this recipient and the `recipientStatus` is updated.\\nHowever, `QVBaseStrategy::reviewRecipients()` function doesn't check the recipient's current status. This one alone may not be an issue because managers may want to change the status of the recipient etc.\\nBut on top of that, the function also doesn't take the previous review counts into account when updating the status, and overwrites the status immediately after reaching the threshold. I'll share a scenario later about this below.\\n```\\nfile: QVBaseStrategy.sol\\n    function reviewRecipients(address[] calldata _recipientIds, Status[] calldata _recipientStatuses)\\n        external\\n        virtual\\n        onlyPoolManager(msg.sender)\\n        onlyActiveRegistration\\n    {\\n        // make sure the arrays are the same length\\n        uint256 recipientLength = _recipientIds.length;\\n        if (recipientLength != _recipientStatuses.length) revert INVALID();\\n\\n        for (uint256 i; i < recipientLength;) {\\n            Status recipientStatus = _recipientStatuses[i];\\n            address recipientId = _recipientIds[i];\\n\\n            // if the status is none or appealed then revert\\n            if (recipientStatus == Status.None || recipientStatus == Status.Appealed) { //@audit these are the input parameter statuse not the recipient's status.\\n                revert RECIPIENT_ERROR(recipientId);\\n            }\\n\\n            reviewsByStatus[recipientId][recipientStatus]++;\\n\\n -->        if (reviewsByStatus[recipientId][recipientStatus] >= reviewThreshold) { //@audit recipientStatus is updated right after the threshold is reached. It can overwrite if the status is already set.\\n                Recipient storage recipient = recipients[recipientId];\\n                recipient.recipientStatus = recipientStatus;\\n\\n                emit RecipientStatusUpdated(recipientId, recipientStatus, address(0));\\n            }\\n\\n            emit Reviewed(recipientId, recipientStatus, msg.sender);\\n\\n            unchecked {\\n                ++i;\\n            }\\n        }\\n    }\\n```\\n\\nAs I mentioned above, the function updates the `recipientStatus` immediately after reaching the threshold. Here is a scenario of why this might be an issue.\\nExample Scenario\\nThe pool has 5 managers and the `reviewThreshold` is 2.\\nThe first manager rejects the recipient\\nThe second manager accepts the recipient\\nThe third manager rejects the recipient. -> `recipientStatus` updated -> `status = REJECTED`\\nThe fourth manager rejects the recipient -> status still `REJECTED`\\nThe last manager accepts the recipient ->recipientStatus updated again -> `status = ACCEPTED`\\n3 managers rejected and 2 managers accepted the recipient but the recipient status is overwritten without checking the recipient's previous status and is ACCEPTED now.\\nCoded PoC\\nYou can prove the scenario above with the PoC. You can use the protocol's own setup for this.\\n- Copy the snippet below and paste it into the `QVBaseStrategy.t.sol` test file.\\n- Run forge test `--match-test test_reviewRecipient_reviewTreshold_OverwriteTheLastOne`\\n```\\n//@audit More managers rejected but the recipient is accepted\\n    function test_reviewRecipient_reviewTreshold_OverwriteTheLastOne() public virtual {\\n        address recipientId = __register_recipient();\\n\\n        // Create rejection status\\n        address[] memory recipientIds = new address[](1);\\n        recipientIds[0] = recipientId;\\n        IStrategy.Status[] memory Statuses = new IStrategy.Status[](1);\\n        Statuses[0] = IStrategy.Status.Rejected;\\n\\n        // Reject three times with different managers\\n        vm.startPrank(pool_manager1());\\n        qvStrategy().reviewRecipients(recipientIds, Statuses);\\n\\n        vm.startPrank(pool_manager2());\\n        qvStrategy().reviewRecipients(recipientIds, Statuses);\\n\\n        vm.startPrank(pool_manager3());\\n        qvStrategy().reviewRecipients(recipientIds, Statuses);\\n\\n        // Three managers rejected. Status will be rejected.\\n        assertEq(uint8(qvStrategy().getRecipientStatus(recipientId)), uint8(IStrategy.Status.Rejected));\\n        assertEq(qvStrategy().reviewsByStatus(recipientId, IStrategy.Status.Rejected), 3);\\n\\n        // Accept two times after three rejections\\n        Statuses[0] = IStrategy.Status.Accepted;\\n        vm.startPrank(pool_admin());\\n        qvStrategy().reviewRecipients(recipientIds, Statuses);\\n\\n        vm.startPrank(pool_manager4());\\n        qvStrategy().reviewRecipients(recipientIds, Statuses);\\n\\n        // 3 Rejected, 2 Accepted, but status is Accepted because it overwrites right after passing threshold.\\n        assertEq(uint8(qvStrategy().getRecipientStatus(recipientId)), uint8(IStrategy.Status.Accepted));\\n        assertEq(qvStrategy().reviewsByStatus(recipientId, IStrategy.Status.Rejected), 3);\\n        assertEq(qvStrategy().reviewsByStatus(recipientId, IStrategy.Status.Accepted), 2);\\n    }\\n```\\n\\nYou can find the test results below:\\n```\\nRunning 1 test for test/foundry/strategies/QVSimpleStrategy.t.sol:QVSimpleStrategyTest\\n[PASS] test_reviewRecipient_reviewTreshold_OverwriteTheLastOne() (gas: 249604)\\nTest result: ok. 1 passed; 0 failed; 0 skipped; finished in 10.92ms\\n```\\n
Checking the review counts before updating the state might be helpful to mitigate this issue
Recipient status might be overwritten with less review counts.
```\\nfile: QVBaseStrategy.sol\\n    function reviewRecipients(address[] calldata _recipientIds, Status[] calldata _recipientStatuses)\\n        external\\n        virtual\\n        onlyPoolManager(msg.sender)\\n        onlyActiveRegistration\\n    {\\n        // make sure the arrays are the same length\\n        uint256 recipientLength = _recipientIds.length;\\n        if (recipientLength != _recipientStatuses.length) revert INVALID();\\n\\n        for (uint256 i; i < recipientLength;) {\\n            Status recipientStatus = _recipientStatuses[i];\\n            address recipientId = _recipientIds[i];\\n\\n            // if the status is none or appealed then revert\\n            if (recipientStatus == Status.None || recipientStatus == Status.Appealed) { //@audit these are the input parameter statuse not the recipient's status.\\n                revert RECIPIENT_ERROR(recipientId);\\n            }\\n\\n            reviewsByStatus[recipientId][recipientStatus]++;\\n\\n -->        if (reviewsByStatus[recipientId][recipientStatus] >= reviewThreshold) { //@audit recipientStatus is updated right after the threshold is reached. It can overwrite if the status is already set.\\n                Recipient storage recipient = recipients[recipientId];\\n                recipient.recipientStatus = recipientStatus;\\n\\n                emit RecipientStatusUpdated(recipientId, recipientStatus, address(0));\\n            }\\n\\n            emit Reviewed(recipientId, recipientStatus, msg.sender);\\n\\n            unchecked {\\n                ++i;\\n            }\\n        }\\n    }\\n```\\n