Upgraded Q -> 2 from #535 [1717590581109]

[c4-judge]

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

Title

PriceFeed can vote both against and in favor of confirming the new price at the same time

Severity

Low

Target

src/managers/LockManager.sol

Description

One of the roles can first call disapproveUSDPrice() and then call approveUSDPrice(), thus increasing the count of those who agree and disagree for the new token price, since the approveUSDPrice function does not check if one of the roles has voted against it.

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

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

Recommendations

it is worth adding to the approveUSDPrice() function to check usdUpdateProposal.disapprovals[msg.sender] == _usdProposalId

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

[c4-judge]

alex-ppg marked the issue as duplicate of #495

[c4-judge]

alex-ppg marked the issue as partial-25