Upgraded Q -> 2 from #546 [1717590748797]

[c4-judge]

Judge has assessed an item in Issue #546 as 2 risk. The relevant finding follows:

[Low-03] Role.PriceFeed can approve & disapprove same time as there is no check for disapproval in approveUSDPrice()

A PriceFeeder at same time can disapprove and approve same purposed price cause absence of disapproval check in approveUSDPrice()

approveUSDPrice() has following checks

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

https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/LockManager.sol#L191-L197 It never checks msg.sender already disapproved current purposal or not, So as a result priceFeeder can first call disapproveUSDPrice() and then call approveUSDPrice() which is not a intended behaviour.

Mitigation

Implement similar check present in disapproveUSDPrice()

    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();
+       if (usdUpdateProposal.disapprovals[msg.sender] == _usdProposalId)
+           revert ProposalAlreadyDisapprovedError();

[c4-judge]

alex-ppg marked the issue as duplicate of #495

[c4-judge]

alex-ppg marked the issue as partial-25

[0xhacksmithh]

I noticed those report which are Upgraded from QA, marked as Partial-25 although they explaining Bug correctly with mitigation similar like other satisfactory mid Bug report.

Why that so,

I give you my thought process why i submit this in QA rather than MID

When i asked to sponcor Q. Role.PriceFeed can approve even he disapprove with price same time, is this intended Replay not intended but not too much of an issue since its trusted

So As they said feeders are trusted and those are set by protocol itself. So i think its a just coding problem and submit it as a QA.

Even if it submited in QA, it explains Buggy Code part, its impact, and its mitigation similar like other MID report

[alex-ppg]

Hey @0xhacksmithh, thanks for your feedback. We have historically recommended against using Sponsor discussions when debating issues as a judge's assessment of an issue is distinct from the sponsor's. In a traditional security audit, the security auditor has the final say on the severity of items listed in their report rather than the client as it is in the client's best interest to have fewer issues / of different severity.

The submission is a valid vulnerability, and all duplicates submitted as QA reports have been awarded 25% and this will not change. I advise you to seek clarifications from sponsors rather than disclosing vulnerabilities you intend to submit as that may also result in a compromise of the contest's process.