Lines of code

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

Vulnerability details

Impact

The current implementation of USDPriceFeed in the Munchables LockManager.sol contract allows only users with the PriceFeed role to initiate a price update proposal. This proposal then requires approval from three out of five other PriceFeed roles. This creates a vulnerability: a sharp price increase for the given asset could cause the value of NFTs to drop and become cheaper to buy, allowing users to capitalize on the cheaper price. Even worse, if there is already an active proposal under consideration for another token, it would need to be resolved before a new proposal could be submitted to react to the new price.

Proof of Concept

https://github.com/code-423n4/2024-05-munchables/blob/main/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;

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

https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/LockManager.sol#L177-L207

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

Tools Used

Manual Analysis

Recommended Mitigation Steps

To address this vulnerability, the best approach would be to implement an oracle feed to obtain the USD price of the asset automatically. However, if you prefer to maintain manual price updates, it's recommended to use an oracle feed as a sanity check threshold. The contract should then revert the update if the proposed price exceeds this threshold.

Assessed type

Other

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

PriceFeed will report stale prices #400