Munchables - Sentryx's results

Lines of code

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

Vulnerability details

Impact

Price feeds that vote against a proposal can still approve it and can influence the end result of the proposal effectively gaining an extra vote.

Proof of Concept

When a USD price proposal is made price feeds can effectively have 2 votes depending on whether they approve or disapprove first. If they first approve a proposal, they don't get to vote for the same proposal again no matter for or against it. If a price feed disapproves first, however, it can still call approveUSDPrice() and affect the outcome of a proposal.

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

We can see that the function checks for the following things:

• There is an actual ongoing proposal
• msg.sender is not the proposer themself
• msg.sender has not approved already
• the proposed price is the one that's being approved currently

It never checks, however, that the price feed has already disapproved the proposed price, which effectively gives them a second vote they can cast.

To give an illustrative example, suppose we have a threshold of 3 (as initially defined in the LockManager contract). At proposal creation, the proposer's vote's counted towards the approvals already, so we are starting off with a 1-0 ratio.

From then on, the 2nd price feed also approves, the 3rd one disapproves and the 4th one disapproves as well. Logically, the 5th price feed should have the final word, but in reality the 3rd and 4th price feeds still have power to approve the proposal and tilt the outcome in the opposite direction.

Even if the price feeds are trusted, this is still not how proposals are supposed to develop and each party involved should have the right to cast their vote exactly one time.

Tools Used

Manual review

Recommended Mitigation Steps

Add the following check to approveUSDPrice():

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

Assessed type

Invalid Validation