Missing disapproval check in `LockManager.sol::approveUSDPrice` allows simultaneous approval and disapproval of a price proposal

[howlbot-integration[bot]]

Lines of code

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

Vulnerability details

Impact

Due to the missing disapproval check, a price feed can both disapprove and subsequently approve a newly proposed price. Price feeds are intended to vote either for approval or disapproval, not both. Hence, this can be considered an unintended functionality.

Proof of Concept

Code

Place this in `MunchablesTest.sol`. ```javascript function test_priceFeedCanVoteBoth() public { address priceFeed2 = makeAddr("priceFeed2"); address tokenUpdated = makeAddr("token"); address[] memory tokensUpdated = new address[](1); tokensUpdated[0] = tokenUpdated; deployContracts(); console.log("--------------- PoC ---------------"); cs.setRole(Role.PriceFeed_1, address(lm), address(this)); cs.setRole(Role.PriceFeed_2, address(lm), priceFeed2); lm.proposeUSDPrice(1 ether, tokensUpdated); vm.startPrank(priceFeed2); lm.disapproveUSDPrice(1 ether); lm.approveUSDPrice(1 ether); vm.stopPrank(); } ```

Tools Used

Manual review.

Recommended Mitigation Steps

Add a check to see if the price feed has already disapproved the price proposal. If so, revert with a custom error.

    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 marked the issue as selected for report

[alex-ppg]

The Warden outlines a misbehavior in the LockManager code that permits an oracle to disapprove and then approve a particular price measurement. The current approval and disapproval thresholds are meant to indicate that no stalemate should be possible, as they are configured at 3 with the total price feed roles being 5.

In reality, this will not lead to a quorum discrepancy as the only action permitted is disapproval and then approval. As such, a state with both approval and disapproval being enabled is impossible as either function reaching a quorum will cause the price measurement to be processed.

There is still an interesting edge case whereby 2 for and 2 against votes will, under normal operations, result in the final role who has not cast their vote yet being the tie-breaker. In the current system, any of the individuals who voted against can place a for vote incorrectly regardless of what the final voter believes.

Even though the price voters are privileged roles, their multitude does permit them to exploit this issue before they are removed if they are deemed malicious by the administrator team of the Munchables system, and in such a scenario the damage will already have been done. As such, I consider this to be a medium-risk rating due to the combination of a privileged action albeit not entirely trusted with an observable but not significant impact on the voting process.

Selecting the best submission out of this duplicate group was very hard as multiple submissions clearly demonstrated the vulnerability and went into depth as to its ramifications. This submission was selected as the best due to being concise, offering a very short and sweet PoC, and outlining the full details needed to grasp the vulnerability.

To note, any submission awarded with a 25% pie (i.e. a 75% reduction) was submitted via a QA report and thus cannot be eligible for the full reward.

[c4-judge]

alex-ppg marked the issue as satisfactory

[McCoady]

The sponsor had previously stated that the PriceFeed roles were trusted. Is this not the case? Also there hasn't been a significant impact outlined to deem this higher than QA.

[niser93]

I would add that there is no case the quorum will be not reachable because even in that case, the admin can assign the price feed role to another address and break the tie: ConfigStorage.sol#L27-L33:

ConfigStorage.sol#L27-L33

function setRole(
    Role _role,
    address _contract,
    address _addr
) public onlyOwner {
    roleStorage[keccak256(abi.encode(_role, _contract))] = _addr;
}

[Ys-Prakash]

Hi @alex-ppg

Thanks a lot for judging this contest.

I would like to reply to the above comments made by fellow security researchers.

Hi @McCoady

The sponsor had previously stated that the PriceFeed roles were trusted. Is this not the case? Also there hasn't been a significant impact outlined to deem this higher than QA.

It has already been assumed by the judge that the PriceFeed roles are trusted / privileged in his comment above:

Even though the price voters are privileged roles, their multitude does permit them to exploit this issue before they are removed if they are deemed malicious by the administrator team of the Munchables system, and in such a scenario the damage will already have been done.

It seems plausible to mark this issue as a Medium because of the following reason:

The approve/disapprove functionality clearly would want to restrict a single PriceFeed role from voting multiple times, either only approving (or, disapproving) multiple times, or, doing both in a combination. This is in accordance with the most used definition of Medium:

2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

Moreover, this issue seems to break an invariant that "should not" be broken, making it a valid case for Medium.

Thank you

Hi @niser93

I would add that there is no case the quorum will be not reachable because even in that case, the admin can assign the price feed role to another address and break the tie: ConfigStorage.sol#L27-L33:

ConfigStorage.sol#L27-L33

function setRole(
    Role _role,
    address _contract,
    address _addr
) public onlyOwner {
    roleStorage[keccak256(abi.encode(_role, _contract))] = _addr;
}

The admin of the protocol definitely holds the power to transfer the roles among different addresses. However, this has been addressed by the judge above:

Even though the price voters are privileged roles, their multitude does permit them to exploit this issue before they are removed if they are deemed malicious by the administrator team of the Munchables system, and in such a scenario the damage will already have been done.

Thank you

[niser93]

Even though the price voters are privileged roles, their multitude does permit them to exploit this issue before they are removed if they are deemed malicious by the administrator team of the Munchables system, and in such a scenario the damage will already have been done.

I think this would be a different scenario from the one proposed in this issue. To exploit the price feed proposal mechanism, malicious addresses should be at least the min(approve_threshold, disapprove_threshold). I want to stress this: price feed roles are trusted. Even assuming one of them is compromised, the only consequence is a momentary impossibility to reach consensus.

[alex-ppg]

Hello @McCoady, @niser93, and @Ys-Prakash; I appreciate everyone's PJQA contributions! I believe that @Ys-Prakash has summed up a lot of your concerns, but I will provide some additional feedback myself to ensure everyone is aligned.

I would add that there is no case the quorum will be not reachable because even in that case

I think this would be a different scenario from the one proposed in this issue. To exploit the price feed proposal mechanism, malicious addresses should be at least the min(approve_threshold, disapprove_threshold). I want to stress this: price feed roles are trusted. Even assuming one of them is compromised, the only consequence is a momentary impossibility to reach consensus.

The vulnerability demonstrates a way to have a proposal pass rather than be unable to reach a consensus. Given that an oracle can toggle their vote whenever they wish from disapproval to approval, a "malicious" oracle can simply cast a disapproving vote on a potentially bad price measurement and toggle it to approval if they make sufficient profit from it (i.e. via the Schnibbles harvested). Being able to appear trustworthy and be able to toggle your vote at will is a significant issue in a theoretically democratic voting system.

The sponsor had previously stated that the PriceFeed roles were trusted. Is this not the case?

To my knowledge, the price feeds are not stated as trusted roles in the contest's README. As I elaborated in my original reply, the only thing we can deduce from the code alone is that they are partially trusted and not fully trusted; otherwise, there would be a single price role and no voting process in place. As such, oracles being in disagreement is a valid and expected operational scenario that necessitates the introduction of a voting process.

At the very least, being able to cast a disapproving and approving vote simultaneously can be considered an egregious error in the code per the C4 SC verdicts and thus still merit a medium risk rating, as it is behavior that should obviously be prohibited and has direct impact to the voting process. The maximum impact of the exhibit is an incorrect price measurement being capitalized by a seemingly honest voting participant who will lose their privilege after they have executed the attack which is not QA level.