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

3 stars 1 forks source link

Upgraded Q -> 2 from #74 [1717590525923] #554

Closed c4-judge closed 4 months ago

c4-judge commented 4 months ago

Judge has assessed an item in Issue #74 as 2 risk. The relevant finding follows:

[L-02] Inconsistent conditions for approving and disapproving the proposal price

The approval and disapproval proposer price mechanism does not allow the PriceFeed_X that was approved first to become disapproved.

However, this logic does not implement the vice versa scenario, as the mechanism allows PriceFeed_X that was disapproved first to become approved afterward.

This results in an inconsistent mechanism and incorrectly tracks usdUpdateProposal.approvalsCount and usdUpdateProposal.disapprovalsCount.

Location: LockManager::approveUSDPrice()

File: ..
    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);
    }
c4-judge commented 4 months ago

alex-ppg marked the issue as duplicate of #495

c4-judge commented 4 months ago

alex-ppg marked the issue as partial-25