Problematic handling of token price updates

[howlbot-integration[bot]]

Lines of code

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

Vulnerability details

Impact

Harvesting calculations (here and here) and nftOverlord.addReveal() can use old token prices.

Proof of Concept

The protocol plans to support new tokens. Imagine that the price of 5 different tokens needs to be updated at the same time and all of the new prices are different.

Currently only 1 proposal can be active at a time because if a new one is proposed before gathering the needed amount of approvals, the process of changing the price for the specified tokens will be stopped (deleted).

Code reference: link

    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;

        // Approvals will use this because when the struct is deleted the approvals remain
@>      ++_usdProposalId;

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

As we will see from the code below, entities with priceFeed role cannot approve previous proposal. They can vote only for the current one.

Code Reference: link

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

Under these circumstances, assuming that all actors are trusted, it takes time to update 5 different tokens with 5 different prices (of course I use number 5 as an example, the number could be much bigger/smaller) because each proposal needs to gather the needed number of approvals from the other users with a priceFeed roles.

This could lead to locks and unlocks being called in the meantime and being executed according to an old token price.

This affects harvesting calculations and nft overlord addReveal().

Tools Used

Manual Review

Recommended Mitigation Steps

Change the proposal, approval and disapproval of prices in a way to handle the update of different tokens with different prices simultaneously.

Assessed type

Other

[c4-judge]

alex-ppg marked the issue as not a duplicate

[c4-judge]

alex-ppg marked the issue as primary issue

[alex-ppg]

The Warden and its duplicate describe different instances of stale proposals affecting the LockManager contract's operation.

This submission details how multiple price updates that are different for different contracts will require independent proposals and consensus to be reached, leading to potentially stale prices utilized for each as the proposals are being processed. I believe this vulnerability is mostly a QA risk due to the fact that the price is not utilized for anything sensitive but rather an "airdrop" in the form of a "lockdrop". Stale prices by a few minutes are acceptable in such a scenario.

The duplicate submission details how a proposal never expires and falls under the same explanation as the price measurements of the LockManager are not mission-critical and can be somewhat stale.

[c4-judge]

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

[c4-judge]

alex-ppg marked the issue as grade-c

[ilchovski98]

The contract that is in the scope of this audit has the purpose of locking funds for the user to earn rewards. Every issue connected with whether the user earns, due to a bug, less or more rewards should be considered mission critical as this is the purpose of this contract.

The developer clearly shows the importance of the correctness of the price by having in his code the following functions: approveUSDPrice(), disapproveUSDPrice(), proposeUSDPrice(), and the price feed roles.

Also, there is nowhere stated in the readme how much time would be needed for a proposal to be approved/rejected. So we cannot assume that it would take a minute and that this will not cause damage to the protocol or the user.

[alex-ppg]

Hey @ilchovski98, thanks for your feedback! A partially trusted role fulfilling their purpose in a timely manner is an expected assumption that we can safely make. Supporting otherwise would cause the price feeds to not be trusted which would be counter-intuitive. We also have to keep in mind that there is redundancy as there are multiple oracles. I cannot consider anything related to the abnormal behavior of multiple oracles as a valid HM vulnerability. In this case, the submission implies that more than half of the oracles have had a delay in accepting price measurements which is not a reasonable assumption.