Delete should not be done on a `struct` with a mapping inside it as it will lead to storage corruption.

[c4-bot-5]

Lines of code

https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/interfaces/ILockManager.sol#L65-L66 https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L161 https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L238 https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L525

Vulnerability details

In Lockmanager.sol when creating a new update proposal to propose a new USD price for an asset in proposeUSDPrice(), in disapproveUSDPrice() when the disapproval threshold has been reached and in _execUSDPriceUpdate() when the approval threshold for the new price has been reached. The struct usdUpdateProposal is reset to its default values using the delete keyword.

proposeUSDPrice()

    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;

//More Code...
}

disapproveUSDPrice()

    function disapproveUSDPrice(
        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();
//More Code...
        emit DisapprovedUSDPrice(msg.sender);

        if (usdUpdateProposal.disapprovalsCount >= DISAPPROVE_THRESHOLD) {
 @>         delete usdUpdateProposal;

            emit RemovedUSDProposal();
        }
    }

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

The issue here is that this struct usdUpdateProposal has a mapping in it and in solidity using delete on a struct with mapping in it will lead to storage corruption and other unexpected results

    /// @notice Struct to manage USD price update proposals
    /// @param proposedDate Timestamp when the price was proposed
    /// @param proposer Address of the oracle proposing the new price
    /// @param contracts Array of contracts whose prices are proposed to be updated
    /// @param proposedPrice New proposed price in USD
    struct USDUpdateProposal {
        uint32 proposedDate;
        address proposer;
        address[] contracts;
        uint256 proposedPrice;
@>      mapping(address => uint32) approvals;
@>      mapping(address => uint32) disapprovals;
        uint8 approvalsCount;
        uint8 disapprovalsCount;
    }

Impact

The mapping in the struct usdUpdateProposal will not be properly deleted leading to unwanted issues

Tools Used

Manual Review

Recommended Mitigation Steps

Instead of deleting the whole struct simply reset the proposer in the struct to address zero after executing the update or rejecting it.

Assessed type

Error

[0xJuancito]

Since approvals and disapprovals aren't cleared, I considered a possible scenario where price feeds couldn't approve a proposal after a previous proposal was executed because of this check and this assignment. But the ++_usdProposalId increment on proposeUSDPrice() solves any potential issues.

This is relevant for similar issues #31, #32, #33, #88, #215, #391

A POC is provided as here as well. Add this code to tests/managers/LockManager/approveUSDPrice.test.ts and run the tests:

  describe("second proposals failure", () => {
    beforeEach(async () => {
      // Propose
      const setUSDThresholdsTxHash =
        await testContracts.lockManager.contract.write.setUSDThresholds([3, 3], { account: admin });
      await assertTxSuccess({ txHash: setUSDThresholdsTxHash });

      let txHash = await testContracts.lockManager.contract.write.proposeUSDPrice(
        [price, [zeroAddress]],
        {
          account: priceFeed1,
        }
      );
      await assertTxSuccess({ txHash });

      // Cast two more votes to approve the proposal and execute it
      txHash = await testContracts.lockManager.contract.write.approveUSDPrice([price], {
        account: priceFeed2,
      });
      await assertTxSuccess({ txHash });
      txHash = await testContracts.lockManager.contract.write.approveUSDPrice([price], {
        account: priceFeed3,
      });
      const txReceipt = await assertTxSuccess({ txHash });

      assertTransactionEvents({
        abi: testContracts.lockManager.contract.abi,
        logs: txReceipt.logs,
        expectedEvents: [
          {
            eventName: "ApprovedUSDPrice",
            args: {
              _approver: priceFeed3,
            },
          },
        ],
      });

      await assertApproveUSDPriceSuccess({
        tokenPrices: [{ tokenContractAddress: zeroAddress, price }],
      });
    });

    it("doesn't fail when trying to propose or vote on a second proposal", async () => {
      let txHash = await testContracts.lockManager.contract.write.proposeUSDPrice(
        [price, [zeroAddress]],
        {
          account: priceFeed1,
        }
      );
      await assertTxSuccess({ txHash });

      txHash = await testContracts.lockManager.contract.write.approveUSDPrice([price], {
        account: priceFeed2,
      });
      await assertTxSuccess({ txHash });
    });
  });