It is possible for a Role.PriceFeed to both approve and disapprove the same proposal

[howlbot-integration]

Lines of code

https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/LockManager.sol#L210-L242
https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/LockManager.sol#L177-L207

Vulnerability details

Impact

It is possible for a Role.PriceFeed to both approve and disapprove the same proposal unethically.

Proof of Concept

According to the contest page, Role.PriceFeed is not a trusted role.
In the disapproveUSDPrice function:
https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/LockManager.sol#L210-L242

       function disapproveUSDPrice(
        uint256 _price
    )
        external
        onlyOneOfRoles(
            [
                Role.PriceFeed_1,
                Role.PriceFeed_2,
                Role.PriceFeed_3,
                Role.PriceFeed_4,
                Role.PriceFeed_5
            ]
        )
    {
        if (usdUpdateProposal.proposer == address(0)) revert NoProposalError();
        if (usdUpdateProposal.approvals[msg.sender] == _usdProposalId)
            revert ProposalAlreadyApprovedError();
        if (usdUpdateProposal.disapprovals[msg.sender] == _usdProposalId)
            revert ProposalAlreadyDisapprovedError();
        if (usdUpdateProposal.proposedPrice != _price)
            revert ProposalPriceNotMatchedError();

        usdUpdateProposal.disapprovalsCount++;
        usdUpdateProposal.disapprovals[msg.sender] = _usdProposalId;

        emit DisapprovedUSDPrice(msg.sender);

        if (usdUpdateProposal.disapprovalsCount >= DISAPPROVE_THRESHOLD) {
            delete usdUpdateProposal;

            emit RemovedUSDProposal();
        }
    }

Here it is observed that Role.PriceFeed is not allowed to call this function after calling the approveUSDPrice function due to ProposalAlreadyApprovedError revert.
This eventually increases the disapprovalsCount.

However in the approveUSDPrice function:

https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/LockManager.sol#L177-L207

    function approveUSDPrice(
        uint256 _price
    )
        external
        onlyOneOfRoles(
            [
                Role.PriceFeed_1,
                Role.PriceFeed_2,
                Role.PriceFeed_3,
                Role.PriceFeed_4,
                Role.PriceFeed_5
            ]
        )
    {
        if (usdUpdateProposal.proposer == address(0)) revert NoProposalError();
        if (usdUpdateProposal.proposer == msg.sender)
            revert ProposerCannotApproveError();
        if (usdUpdateProposal.approvals[msg.sender] == _usdProposalId)
            revert ProposalAlreadyApprovedError();
        if (usdUpdateProposal.proposedPrice != _price)
            revert ProposalPriceNotMatchedError();

        usdUpdateProposal.approvals[msg.sender] = _usdProposalId;
        usdUpdateProposal.approvalsCount++;

        if (usdUpdateProposal.approvalsCount >= APPROVE_THRESHOLD) {
            _execUSDPriceUpdate();
        }

        emit ApprovedUSDPrice(msg.sender);
    }

Here, it is observed that the same Role.PrceFeed can call the approveUSDPrice function after calling the disapproveUSDPrice function due to lack of validation. This eventually increases the approvalsCount unethically as a single Role.PriceFeed can both disapprove as well as approve a proposal.

This means that a Role.PriceFeed cannot approve a proposal and then disprove the same proposal however a Role.PriceFeed can first disapprove and then approve the same proposal.

This leads to a waste of vote or favor towards a particular decision.

Tools Used

Manual Review

Recommended Mitigation Steps

Add this check in the approveUSDPrice function:

            if (usdUpdateProposal.disapprovals[msg.sender] == _usdProposalId)
            revert ProposalAlreadyDisapprovedError();

Assessed type

Invalid Validation

[c4-judge]

alex-ppg marked the issue as satisfactory

[c4-judge]

alex-ppg changed the severity to 2 (Med Risk)