Lines of code

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

Vulnerability details

Impact

A price feed role owner can possibly cast a disapprove vote and approve vote in the same USD proposal.

Proof of Concept

In the approveUSDPrice() function, a price feed role owner can cast an approve vote on a USD price proposal even if they already set a disapprove previously because there is no check to see if the msg.sender had previously disapproved the proposal. This will potentially cause both usdUpdateProposal.approvalsCount++ and usdUpdateProposal.disapprovalsCount++ to be incremented by the same user.

It's implied that a price feed role owner can only vote once and not have their vote changed because if we take a look at the disapproveUSDPrice() function, it checks to see if the msg.sender previously approved or disapproved. If either option was already made, then the function reverts. See code below inside disapproveUSDPrice():

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

The same must be done inside approveUSDPrice() function so that a user who previously disapproved a proposal can not decide to approve the same proposal

Tools Used

Manual Review

Recommended Mitigation Steps

Consider adding a check inside approveUSDPrice() to make sure a user did not already disapprove before. Please add to the beginning of the function:

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

Assessed type

Other

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

approveUSDPrice function does not check if a user already disapproved a specific price already, leading to a user casting multiple votes on the same price proposal #480