Lack of Validation for `_price` Parameter in `proposeUSDPrice` Function

[c4-bot-10]

Lines of code

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

Vulnerability details

Description

The proposeUSDPrice function in the LockManager contract does not validate the _price parameter. This oversight can lead to several issues, including the potential assignment of a zero value or other invalid prices, which could impact the contract's logic and functionality.

Potential Impact

1. Zero Price Assignment: Without validation, the function can be called with _price set to zero, which could lead to incorrect USD price updates. This would affect all dependent calculations and contract functionalities that rely on the USD price and lead to significant financial losses, incorrect calculations, and disruption of the intended functionality of the contract..
2. Arbitrary Price Assignment: Lack of upper and lower bounds for _price means it can be set to values that may not make sense within the business logic, potentially leading to financial discrepancies or exploitation.

Mitigation Steps

To address this vulnerability, the following validation steps should be added to the proposeUSDPrice function:

1. Check for Zero Value:

   solidity

• if (_price == 0) { revert InvalidPriceError(); // Define and use a custom error for better clarity. }

• Check for Reasonable Limits: Depending on the business logic, define reasonable upper and lower bounds for _price. For example, if _price must be between 1 and 1,000,000:

  solidity

• uint256 constant MIN_PRICE = 1; uint256 constant MAX_PRICE = 1_000_000; if (_price < MIN_PRICE || _price > MAX_PRICE) { revert PriceOutOfBoundsError(_price, MIN_PRICE, MAX_PRICE); }

• Updated Function with Validation:

  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();
  
      // Validation for _price
     ++if (_price == 0) {
     ++       revert InvalidPriceError();
     ++ }
  
     ++ uint256 constant MIN_PRICE = 1;
     ++ uint256 constant MAX_PRICE = 1_000_000;
  
     ++ if (_price < MIN_PRICE || _price > MAX_PRICE) {
     ++     revert PriceOutOfBoundsError(_price, MIN_PRICE, MAX_PRICE);
     ++ }
  
      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);
  }

Tools Used

Manual review, VS Code

Assessed type

Invalid Validation