Lines of code

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

Vulnerability details

Impact

An unwanted price change may occur since the function approveUSDPrice doesn't check for already disproven price proposals.

Proof of Concept

If we take a closer look at approveUSDPrice:

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

we can see that in lines 191-197 there are if statements which ensure that: there is an actual proposal to approve

if (usdUpdateProposal.proposer == address(0)) revert NoProposalError();

that the proposer isn't trying to approve his own proposal

if (usdUpdateProposal.proposer == msg.sender) revert ProposerCannotApproveError();

that the proposal isn't already approved

if (usdUpdateProposal.approvals[msg.sender] == _usdProposalId) revert ProposalAlreadyApprovedError();

and that the price being approved matches the proposed price

if (usdUpdateProposal.proposedPrice != _price) revert ProposalPriceNotMatchedError();

However there is no check to see if a price proposal hasn't already been disproven by disapproveUSDPrice which can lead to an unwanted price being approved and if the approvalsCount reaches APPROVE_THRESHOLD, a price change will occur.

For example, a price is proposed via the function proposeUSDPrice, it is then disproven by one of the price feeds, now only two price feeds need to approve the proposal for it to go through since APPROVE_THRESHOLD and DISAPPROVE_THRESHOLD can be set to 2 and 2 respectively with the function setUSDThresholds.

Here are links to all the mentioned code: proposeUSDPrice - https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L142C5-L174C6

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

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

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

Tools Used

Manual review

Recommended Mitigation Steps

Add an if statement like the one in disapproveUSDPriced to ensure that disproven proposals aren't approved.

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

Assessed type

Other

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

Possible approvals of disproven price proposals #509