Stale Proposal Issue in USD Price Update Proposal Contract

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L142-L174

Vulnerability details

The USD price update proposal contract does not account for proposal expiration, potentially leading to the approval of stale proposals with outdated price information. This can result in inaccurate price updates and negatively impact the system's integrity.

The contract records the date a proposal is made but does not implement any mechanism to expire proposals. As a result, proposals can remain active indefinitely, and stale proposals with outdated information might be approved in the future.

Proof of Concept

Consider the following contract functions:

https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L142-L174

function proposeUSDPrice(
    uint256 _price,
    address[] calldata _contracts
)
    external
    onlyOneOfRoles(
        [
            Role.PriceFeed_1,
            Role.PriceFeed_2,
            Role.PriceFeed_3,
            Role.PriceFeed_4,
            Role.PriceFeed_5
        ]
    )
{
    if (usdUpdateProposal.proposer != address(0))
        revert ProposalInProgressError();
    if (_contracts.length == 0) revert ProposalInvalidContractsError();

    delete usdUpdateProposal;

    usdUpdateProposal.proposedDate = uint32(block.timestamp);
    usdUpdateProposal.proposer = msg.sender;
    usdUpdateProposal.proposedPrice = _price;
    usdUpdateProposal.contracts = _contracts;
    usdUpdateProposal.approvals[msg.sender] = _usdProposalId;
    usdUpdateProposal.approvalsCount++;

    emit ProposedUSDPrice(msg.sender, _price);
}

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

In the above code, there is no check to expire proposals based on their proposedDate.

Impact

Proposals with outdated information can be approved, leading to inaccurate price updates.

Recommended Mitigation Steps

Implement a proposal expiration mechanism to ensure that only timely and relevant proposals are considered. For example, proposals can be set to expire after a certain period, such as 7 days.

Add a proposal expiration check in the approval and disapproval functions:

uint32 constant PROPOSAL_EXPIRATION_TIME = 7 days; // Example expiration time

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.proposedPrice != _price) revert ProposalPriceNotMatchedError();
    if (block.timestamp > usdUpdateProposal.proposedDate + PROPOSAL_EXPIRATION_TIME) {
        delete usdUpdateProposal;
        revert ProposalExpiredError();
    }

    usdUpdateProposal.approvals[msg.sender] = _usdProposalId;
    usdUpdateProposal.approvalsCount++;

    if (usdUpdateProposal.approvalsCount >= APPROVE_THRESHOLD) {
        _execUSDPriceUpdate();
    }

    emit ApprovedUSDPrice(msg.sender, _price);
}

Assessed type

Context

[c4-judge]

alex-ppg marked the issue as not a duplicate

[c4-judge]

alex-ppg marked the issue as duplicate of #428

[c4-judge]

alex-ppg changed the severity to QA (Quality Assurance)

[c4-judge]

alex-ppg marked the issue as grade-c