The `approveUSDPrice()` Function Allows a Price Feed Updater to Approve a Proposal After Disapproving It Without Decreasing `disapprovalsCount`

[howlbot-integration[bot]]

Lines of code

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

Vulnerability details

The approveUSDPrice() function in the LockManager contract is designed to approve a price update proposal, while the disapproveUSDPrice() function is meant to mark disapproval for the proposed price. The vulnerability is, the approveUSDPrice() function allows price feed updaters to mark their approval even after their disapproval to the same proposal without reducing the disapprovalsCount.

Impact

This vulnerability allows price feed updaters to double-cast their votes. This means a price feed updater can disapprove a proposal and then approve it without their disapproval being properly accounted for, potentially manipulating the proposal approval process.

Proof of Concept

The vulnerability lies in the code below. The approveUSDPrice() function checks if the user has already approved in line 194, but it fails to check whether the user has already marked their 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();
194: @> if (usdUpdateProposal.approvals[msg.sender] == _usdProposalId)
            revert ProposalAlreadyApprovedError();
        if (usdUpdateProposal.proposedPrice != _price)
            revert ProposalPriceNotMatchedError();

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

The disapproveUSDPrice() function successfully implements this check, preventing double voting. The checks in the disapproveUSDPrice() function are shown below:

225:    if (usdUpdateProposal.approvals[msg.sender] == _usdProposalId)
226:        revert ProposalAlreadyApprovedError();
227:    if (usdUpdateProposal.disapprovals[msg.sender] == _usdProposalId)
228:        revert ProposalAlreadyDisapprovedError();

Tools Used

Manual Review

Recommended Mitigation Steps

Include a check to ensure that the user has not disapproved the proposal before allowing them to approve it.

Sample code snippet:

    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

Other

[c4-judge]

alex-ppg changed the severity to 2 (Med Risk)

[c4-judge]

alex-ppg marked the issue as satisfactory

[c4-judge]

alex-ppg removed the grade

[c4-judge]

alex-ppg marked the issue as satisfactory