Inadequate Approval and Disapproval Thresholds for USD Price Proposal

[c4-bot-1]

Lines of code

https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/LockManager.sol#L129-L139 https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/LockManager.sol#L15-L18

Vulnerability details

Impact

While the default values for APPROVE_THRESHOLD and DISAPPROVE_THRESHOLD are both set to 3, the issue arises when these thresholds are modified. Given that the proposer’s approval is automatically counted, only 4 out of the 5 designated addresses can participate in the disapproval process. If the thresholds are increased to e.g. 4, proposals can become stuck if only 2 approvals (2 + 1 in this case) or 2 disapprovals are obtained, as the required threshold cannot be met. This situation prevents the proposal from progressing or being terminated, which can disrupt the governance process and lead to operational inefficiencies. Without a mechanism to reset or resolve a stuck proposal, the contract's functionality can be severely impacted.

Proof of Concept

Here's a typical scenario:

• There are 5 designated addresses that can approve or disapprove proposals.

https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/LockManager.sol#L181-L189 https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/LockManager.sol#L214-L222

        onlyOneOfRoles(
            [
                Role.PriceFeed_1,
                Role.PriceFeed_2,
                Role.PriceFeed_3,
                Role.PriceFeed_4,
                Role.PriceFeed_5
            ]
        )

• The proposer’s approval is automatically counted, leaving 4 addresses to approve or disapprove.
• The APPROVE_THRESHOLD is set to 4 and DISAPPROVE_THRESHOLD is set to 4 via:

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

    function setUSDThresholds(
        uint8 _approve,
        uint8 _disapprove
    ) external onlyAdmin {
        if (usdUpdateProposal.proposer != address(0))
            revert ProposalInProgressError();
        APPROVE_THRESHOLD = _approve;
        DISAPPROVE_THRESHOLD = _disapprove;

        emit USDThresholdUpdated(_approve, _disapprove);
    }

• If the proposal receives 2 approvals and 2 disapprovals, it will be stuck because the required threshold of 4 cannot be reached for either approval or disapproval. Neither _execUSDPriceUpdate() nor delete usdUpdateProposal can be triggered.
• This leaves the proposal in an indefinite limbo state with no mechanism to reset or resolve the issue.
• This is because in this state, the usdUpdateProposal.proposer has already been set, blocking any new proposals or threshold adjustments because the contract logic reverts such attempts with ProposalInProgressError.

https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/LockManager.sol#L133-L134 https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/LockManager.sol#L157-L158

        if (usdUpdateProposal.proposer != address(0))
            revert ProposalInProgressError();

• As a result, the contract's functionality is effectively frozen, preventing any further governance actions.

Tools Used

Manual

Recommended Mitigation Steps

Ensure that the sum of APPROVE_THRESHOLD and DISAPPROVE_THRESHOLD does not exceed the total number of designated addresses plus one (to account for the proposer’s automatic approval).

    function setUSDThresholds(
        uint8 _approve,
        uint8 _disapprove
    ) external onlyAdmin {
+        require(_approve + _disapprove <= 6, "Combined thresholds too high");
        if (usdUpdateProposal.proposer != address(0))
            revert ProposalInProgressError();
        APPROVE_THRESHOLD = _approve;
        DISAPPROVE_THRESHOLD = _disapprove;

        emit USDThresholdUpdated(_approve, _disapprove);
    }

Assessed type

Invalid Validation

[mystery0x]

@alex-ppg When #495 is fixed, it's going to make the issue described in this report a ready vulnerability. Please let me know if you would need further context, and thank you for looking into this report.

[alex-ppg]

Hey @mystery0x, thanks for your PJQA feedback! First of all, using the alleviation of a vulnerability as evidence of a submission's severity escalation is not accepted practice.

In any case, the submission does indeed detail a flaw in the code albeit a QA (L) one as we are talking about an administrative function that we can assume is responsibly invoked. As such, it cannot constitute a valid HM vulnerability.