Lines of code

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

Vulnerability details

Description

A single Role.PriceFeed_X (where X belongs to {1, 2, 3, 4, 5}) can 1st disapprove a proposal and then approve the same proposal.

Proof of Concept

The following is a step-by-step explanation of the issue.

Consider a scenario where Role.PriceFeed_1 proposes a USD price proposal through the proposeUSDPrice() function:

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

    function proposeUSDPrice(
        uint256 _price,
        address[] calldata _contracts
    )
        external
        onlyOneOfRoles(
            [
                Role.PriceFeed_1,
                Role.PriceFeed_2,
                Role.PriceFeed_3,
                Role.PriceFeed_4,
                Role.PriceFeed_5
            ]
        )
    {
        if (usdUpdateProposal.proposer != address(0))
            revert ProposalInProgressError();
        if (_contracts.length == 0) revert ProposalInvalidContractsError();

        delete usdUpdateProposal;

        // Approvals will use this because when the struct is deleted the approvals remain
        ++_usdProposalId;

        usdUpdateProposal.proposedDate = uint32(block.timestamp);
        usdUpdateProposal.proposer = msg.sender;
        usdUpdateProposal.proposedPrice = _price;
        usdUpdateProposal.contracts = _contracts;
        usdUpdateProposal.approvals[msg.sender] = _usdProposalId;
        usdUpdateProposal.approvalsCount++;

        emit ProposedUSDPrice(msg.sender, _price);
    }

Now, consider the following steps:

Role.PriceFeed_2 decides to disapprove the proposal.
He calls the disapproveUSDPrice() function to do so:

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

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

Now, the disapproveUSDPrice() function checks for the approvals[] mapping to not contain the current _usdProposalId:

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

if (usdUpdateProposal.approvals[msg.sender] == _usdProposalId)

This ensures that Role.PriceFeed_2 could not disapprove after he approved the proposal.

The disapproveUSDPrice() function also sets the disapprovals[] mapping for Role.PriceFeed_2 to the current _usdProposalId:

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

usdUpdateProposal.disapprovals[msg.sender] = _usdProposalId;

This ensures that Role.PriceFeed_2 would not be able to replay the disapproveUSDPrice() function.

One thing to note here is that the disapproveUSDPrice() function does not set the approvals[] mapping for Role.PriceFeed_2, that is, usdUpdateProposal.approvals[msg.sender] (with msg.sender == address of Role.PriceFeed_2) still does not contain the current _usdProposalId.

After disapproving, Role.PriceFeed_2 tries to approve the proposal by calling the approveUSDPrice() function:

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

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

The 1st, 2nd, and 4th if statements check are easily tackled since: a) The proposal is still active. b) Role.PriceFeed_2 is not the proposer. c) Role.PriceFeed_2 sets the correct _price.

The check of our interest is the 3rd if statement. This also gets tackled as (mentioned above) the usdUpdateProposal.approvals[Role.PriceFeed_2] would still not contain the current _usdProposalId. Hence, the transaction is not reverted. Finally, usdUpdateProposal.approvals[Role.PriceFeed_2] is set to the current _usdProposalId in LOC-199 and the usdUpdateProposal.approvalsCount is increased by 1:

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

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

Hence, Role.PriceFeed_2 was able to first disapprove and then approve a proposal.

The vice-versa cannot happen, that is, first approving and then disapproving cannot happen.

Note

This should not be seen as a facility for the Role.PriceFeed_Xs to rectify their mistake, that is, giving them a chance to approve a proposal that they earlier disapproved by mistake due to the following reasons:

There is no such "facility" for the vice-versa case.
The usdUpdateProposal.disapprovalsCount is not reduced when the Role.PriceFeed_X decides to approve the proposal after disapproving it earlier.

Impact

A single Role.PriceFeed_X would be able to vote 2 times as well as both approve and disapprove a single proposal.

This is an invariant of the protocol that "should not" be broken, making this a clear case for Medium.

The approve/disapprove functionality clearly would want to restrict a single Role.PriceFeed_X from voting multiple times, either only approving (or, disapproving) multiple times, or, doing both in a combination.

This functionality of the protocol is clearly hindered, thus, impacting its availability. This also consolidates this case to be a Medium.

Tools Used

VS Code Manual review of code

Recommended Mitigation Steps

It is recommended to add the following check in the approveUSDPrice() function:

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

Assessed type

Invalid Validation

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

A single Role.PriceFeed_X can both disapprove and approve the same proposal #475