Munchables - AgileJune's results

Lines of code

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

Vulnerability details

Impact

The price can be disapproved, but it's correct price which is intended to be approved by more than APPROVE_THRESHOLD(=3)

Proof of Concept

There are 5 roles to approve/disapprove the proposed price. If at least 3 roles approve or disapprove to assess the proposed price. Through inspect of approve, disapprove functions, approved user can't disapprove the price again, but user who did already disapprove can approve the price again. So it means that user feels his previous disapproving decision is not correct, and changed his assessment result as approvement, but at this time, usdUpdateProposal.disapprovals[msg.sender] is not cleared and disapprovalsCount is not deducted according to it. So it cause disapprovalsCount is remained unchanged. If another role disapproves the price, proposed price is not accepted as result.

ex: role1 proposed price X -- approvalsCount = 1, disapprovalsCount = 0 role2 disapprove -- approvalsCount = 1, disapprovalsCount = 1 role3 disapprove -- approvalsCount = 1, disapprovalsCount = 2 role3 approve -- approvalsCount = 2, disapprovalsCount = 2 role4 disapprove -- approvalsCount = 2, disapprovalsCount = 3

Without role5's assessment, the price can be rejected.

Tools Used

Manual review

Recommended Mitigation Steps

add the below checker in approve() function

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

Assessed type

Other