Lines of code

https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L191-L197

Vulnerability details

Impact

Due to a missing check on approveUSDPrice() a single price feed role can first disapprove a price and then approve it, basically casting two votes instead of one.

Proof of Concept

When a price feed role wants to disapprove a price there are checks which make sure that they are not the proposer, or they haven't already approved the price:

        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();

Although some of the above-mentioned validations are not present in the approveUSDPrice() function.

A price feed role is able to both disapprove and approve a proposed price, as long as they first disapprove it, and then approve it.

    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++;

A price feed role can "disapprove" a price, which will set the disapprovals[msg.sender] to the current id.
Then due to the non-existing checks if the price feed role who is calling the approveUSDPrice() if they've casted a vote for the price disapproval, will be able to also approve the price, effectively casting two votes.

Tools Used

Manual Review

Recommended Mitigation Steps

Include the following check in approveUSDPrice():

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

Assessed type

Invalid Validation

code-423n4 / 2024-05-munchables-findings

A Price Feed role can disapprove and approve a price on the same proposal due to a missing check #492