Upgraded Q -> 2 from #552 [1717590895402]

[c4-judge]

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

QA-03 Current voting logic is flawed as it allows for double votes for the same proposal

Proof of Concept

Take a look at LockManager._execUSDPriceUpdate()

    function _execUSDPriceUpdate() internal {
        if (
            usdUpdateProposal.approvalsCount >= APPROVE_THRESHOLD &&
            usdUpdateProposal.disapprovalsCount < DISAPPROVE_THRESHOLD
        ) {
            uint256 updateTokensLength = usdUpdateProposal.contracts.length;
            for (uint256 i; i < updateTokensLength; i++) {
                address tokenContract = usdUpdateProposal.contracts[i];
                if (configuredTokens[tokenContract].nftCost != 0) {
                    configuredTokens[tokenContract].usdPrice = usdUpdateProposal
                        .proposedPrice;

                    emit USDPriceUpdated(
                        tokenContract,
                        usdUpdateProposal.proposedPrice
                    );
                }
            }

            delete usdUpdateProposal;
        }
    }

This function is called when updating the price of a token if the proposal meets the APPROVE_THRESHOLD and has fewer than DISAPPROVE_THRESHOLD disapprovals. However, the logic for counting votes has a flaw that allows a voter to vote both for and against the same proposal. This occurs because while there is a check to prevent disapproving a proposal that has already been approved, there is no similar check to prevent approving a proposal that has already been disapproved.

Approve Function

In approveUSDPrice(), the checks are as follows: approveUSDPrice()

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

Disapprove Function

In disapproveUSDPrice(), the checks are as follows: disapproveUSDPrice()

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

A price feed updater who has previously disapproved an update can later approve the same update. This discrepancy occurs because only the disapproval state is being set in disapproveUSDPrice(), and the check in approveUSDPrice() would still pass.

Impact

Allowing a voter to vote both for and against the same proposal results in an inflated vote count, causing erroneous approvals or disapprovals. This can lead to incorrect execution of price updates since the thresholds (>= DISAPPROVE_THRESHOLD & >= APPROVE_THRESHOLD) are based on inaccurate vote counts. Consequently, proposals may be approved or disapproved inappropriately.

Recommended Mitigation Steps

Ensure that each price feeder can vote only once per proposal. A voter changing their decision should have their previous vote nullified rather than counting as an additional vote. Implement checks to prevent double voting and ensure that each voter's final decision is accurately reflected in the vote count.

[c4-judge]

alex-ppg marked the issue as duplicate of #495

[c4-judge]

alex-ppg marked the issue as partial-25