Lines of code

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

Vulnerability details

Impact

The disapproveUSDPrice function does not revert when the DISAPPROVE_THRESHOLD is not reached. This oversight can lead to unexpected behavior and potential security vulnerabilities. In this vulnerability disapproveUSDPrice() does not handle the case in which DISAPPROVE_THRESHOLD is not reached and goes ahead to increase usdUpdateProposal.disapprovalsCount and sets the usdUpdateProposal.disapprovals mapping to msg.sender

Proof of Concept

the disapproveUSDPrice function. after the disapproveUSDPrice checks the DISAPPROVE_THRESHOLD in the ending IF/ELSE STATEMENTS. It doesn't revert if the threshold is not met and goes to increase the usdUpdateProposal.disapprovalsCount. This leads to incosistencies in the usdUpdateProposal.disapprovalsCount in which the number of dissaprovals increases even the disapproval didn't wasn't successful

    /// @inheritdoc ILockManager
    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();
        }
    }

Tools Used

manual review, vs code.

Recommended Mitigation Steps

After incrementing usdUpdateProposal.disapprovalsCount, check whether it reaches the DISAPPROVE_THRESHOLD. If the threshold is not met, revert the transaction to maintain consistency.

Assessed type

Other

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

Lack of Revert When DISAPPROVE_THRESHOLD Is Not Reached #322