Munchables - Topmark's results

Lines of code

https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/LockManager.sol#L225-L228 https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/LockManager.sol#L194

Vulnerability details

Impact

• Price Feed Caller can Approve and Disapprove USD Price At The Same Time breaking Price Feed Vote Count functionality Due to break in proper Threshold for Executing or Removing a Proposal in LockManager contract
• The number of Price Votes can go above the number of Price Feed Voters

Proof of Concept

   function disapproveUSDPrice(
        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.approvals[msg.sender] == _usdProposalId)
            revert ProposalAlreadyApprovedError();
 >>>       if (usdUpdateProposal.disapprovals[msg.sender] == _usdProposalId)
            revert ProposalAlreadyDisapprovedError();
        if (usdUpdateProposal.proposedPrice != _price)
            revert ProposalPriceNotMatchedError();

        usdUpdateProposal.disapprovalsCount++;
        usdUpdateProposal.disapprovals[msg.sender] = _usdProposalId;

        emit DisapprovedUSDPrice(msg.sender);

        if (usdUpdateProposal.disapprovalsCount >= DISAPPROVE_THRESHOLD) {
            delete usdUpdateProposal;

            emit RemovedUSDProposal();
        }
    }

The function above from the LockManager contract shows how price disapproval is implemented, it can be noted from the validation from the two pointers above that if a msg.sender or caller has previously approved or disapproved a USD price proposal, the the function will revert, the summary of this validation is simply to ensure a caller does not get two votes for the same Proposal i.e

1. Double Disapproval
2. An Approval and a Disapproval

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

On the other hand the function above from the same contract shows how price approval is implemented, it can be noted from the validation from the pointer above that if a msg.sender or caller has previously approved a USD price proposal, the the function will revert, the summary of this validation is also to ensure a caller does not get two votes for the same Proposal i.e Double Approval. The problem is this implementation is not complete as a caller can still get a double vote for an Approval and a Disapproval. This would not be possible by first doing Approval since Disapproval function is correctly implemented and would stop the second vote. But the protocol missed the fact that a caller can simply take the backward direction by first calling Disapproval function to get the first vote then calling Approval function to get the second vote without reversion.

Note: This issue might seem like it has no impact since the caller simply gets 1 votes of both approval and disapproval which means this votes cancel out each other and could be simply be interpreted that the caller didn't vote at all. But why this is a problem is that the vote count system is working with a threshold as noted at L135-L136, this means this irrelevant votes affects the power that threshold have as bunch of useless votes end up filling the threshold affecting actual votes counts that hold tangible value

Scenario Proof

lets assume there are 5 callers or voters and Approval threshold = 3 Disapproval threshold = 2 current Approval count = 0 current Disapproval count = 0 lets assume 2 of the callers made this useless disapproval and approval vote current Approval count = 2 current Disapproval count = 2 Disenfranchised Voters = 3

• Already the total number of possible votes(2+2+3) have gone above number of voters (5)
• At this point The Proposal can be disapproved based on the 2 Disapproval threshold value, even with this intangible votes when real voters have not made an input yet.

Tools Used

Manual Review

Recommended Mitigation Steps

As adjusted below the approval of USDPrice should not only check to to see if caller has previously approved Price but also check to ensure that caller has not previously disapprove Price before proceeding with Approval

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

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

        usdUpdateProposal.approvals[msg.sender] = _usdProposalId;
        usdUpdateProposal.approvalsCount++;

        if (usdUpdateProposal.approvalsCount >= APPROVE_THRESHOLD) {
            _execUSDPriceUpdate();
        }

        emit ApprovedUSDPrice(msg.sender);
    }

Assessed type

Access Control