Munchables - typicalHuman's results

Lines of code

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

Vulnerability details

Impact

There is a check to prevent disapproving a proposal after the price feed has already approved it. However, you can disapprove the proposal and then approve it because approveUSDPrice only checks the approvals mapping. This creates an ambiguity in the vote, as you can both approve and disapprove it simultaneously. The relevant code can be found here: https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L194-L195.

Proof of Concept

    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();
// NO CHECK FOR DISSAPPROVALS
        if (usdUpdateProposal.proposedPrice != _price)
            revert ProposalPriceNotMatchedError();

Tools Used

Reading source code.

Recommended Mitigation Steps

Add dissapproval check to approveUSDPrice:

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

Assessed type

Error