Lines of code

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

Vulnerability details

Proof of Concept

Take a look at https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L506-L527

    function _execUSDPriceUpdate() internal {
        if (
            usdUpdateProposal.approvalsCount >= APPROVE_THRESHOLD &&
            usdUpdateProposal.disapprovalsCount < DISAPPROVE_THRESHOLD
        ) {
            uint256 updateTokensLength = usdUpdateProposal.contracts.length;
            for (uint256 i; i < updateTokensLength; i++) {
                address tokenContract = usdUpdateProposal.contracts[i];
                if (configuredTokens[tokenContract].nftCost != 0) {
                    configuredTokens[tokenContract].usdPrice = usdUpdateProposal
                        .proposedPrice;

                    emit USDPriceUpdated(
                        tokenContract,
                        usdUpdateProposal.proposedPrice
                    );
                }
            }

            delete usdUpdateProposal;
        }
    }

This is the function that's inherently called upon when the price is to be updated and when more than/ or equal to APPROVE_THRESHOLD have been, have voted for the proposal, issue however is from how protocol currently counts the for & against votes for a specific update. Going through both approveUSDPrice() & disapproveUSDPrice() we can see that there whereas there is a check when disapproving that a pricefeeder has not approved of the same update, a similar check does not exist during approving and a price feeder who has previously disapproved can still approve. These are the checks applied for the functions:

In approveUSDPrice() https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L191-L198

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

In disapproveUSDPrice() https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L224-L231

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

This means that a price feed updater that previously disapproved an update can now approve the same update cause only their disapproval state is being set in disapproveUSDPrice()
which would then make approving the same update to pass since this check in approveUSDPrice would pass.

Impact

Just like with any voting logic, in this case a price feeder should be given only one vote for an update and as such they should not be able to vote both for & against the same update.

Currently one is allowed to vote twice for the same price update and this erroneously counts as two votes, this would then mean that on the next instance of approve or disapprove the price update could then be approved/disapproved depending on which function gets called, main case is that the >= DISAPPROVE_THRESHOLD& >= APPROVE_THRESHOLD would now integrate with wrong data since the count has wrongly been inflated, leading to approvals where they shouldn't be or disapprovals where they shouldn't be one.

Recommended Mitigation Steps

Either not allow more than one vote for the same price update or a pricefeeder re-voting with a different decision should cancel his previous vote instead of counting as twice (and this should be time restricted).

Assessed type

Context

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