Lines of code

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

Vulnerability details

Impact

The vulnerability in the price update mechanism allows for disapproving and reapprove of the same price. The current implementation allows a user to both approve and disapprove the same price for a proposal, leading to double approvals.

Proof of Concept:

The code snippet reveals an inconsistency in how approvals and disapprovals are handled:

// disapproveUSDPrice function
 function disapproveUSDPrice(
        uint256 _price
    )
      .................................
 if (usdUpdateProposal.proposer == address(0)) revert NoProposalError();
   @ audit >>       if (usdUpdateProposal.approvals[msg.sender] == _usdProposalId)
            revert ProposalAlreadyApprovedError();
 @ audit >>       if (usdUpdateProposal.disapprovals[msg.sender] == _usdProposalId)
            revert ProposalAlreadyDisapprovedError();
        if (usdUpdateProposal.proposedPrice != _price)
            revert ProposalPriceNotMatchedError();
}

// approveUSDPrice function (missing check)
// ... no check for disapprovals before approval

  function approveUSDPrice(
        uint256 _price
    )
       ............................................
        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();

The disapproveUSDPrice function prevents a user from disapproving a proposal they already approved of.

       if (usdUpdateProposal.approvals[msg.sender] == _usdProposalId)
            revert ProposalAlreadyApprovedError();
      if (usdUpdateProposal.disapprovals[msg.sender] == _usdProposalId)
           revert ProposalAlreadyDisapprovedError();

However, there's no corresponding check in the approveUSDPrice function to prevent a user from approving a price they previously disapproved.

        if (usdUpdateProposal.approvals[msg.sender] == _usdProposalId)
            revert ProposalAlreadyApprovedError();
 @ audit >>    // This check is missing after the above  if (usdUpdateProposal.disapprovals[msg.sender] == _usdProposalId)
// revert ProposalAlreadyDisapprovedError();

Impact:

This missing check allows a user to:

Disapprove a price, potentially influencing the voting process.
Later, approve the same price.

Tools Used

Manual Code analysis

Recommended Mitigation Steps:

Implement a check for disapprovals in the approveUSDPrice function, mirroring the logic in disapproveUSDPrice:

function approveUSDPrice(uint256 _price) external {
............................................
 // ... rest of the approval logic
  if (proposal.disapprovals[msg.sender] == _usdProposalId) {
    revert ProposalAlreadyDisapprovedError();
  }
  // ... rest of the approval logic
}

Assessed type

Error

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

Double Approval Vulnerability in Approve and Disapprove Update Mechanism #500