The approval process of the USD price can be manipulated (either intentionally or uintentionally) if the price proposal has been first disapproved and then approved. This can lead to the manipulation of the votes needed to reach the Approval and Disapproval threshold therefore breaking the functionality of the protocol
Proof of Concept
In the functions approveUSDPrice and disapproveUSDPrice there are certain thresholds that need to be met in order for proposal to be accepted or discarded.
According to the sponsor, resposible roles should not be able to change their opinions and the decision should be final.
In the disapproveUSDPrice there is a check that the price has not been already approved or disapproved
However, if the same user tries to approve the USD price proposal afterward, it will work since the function approveUSDPrice only checks if the proposal has been approved but does not check if it has been disapproved. That means that the decision in this case would not be final.
This is a problem, especially in the case when approval and disapproval thresholds are different, where a single user can have an impact and reduce thresholds in both functions with his vote.
Tools Used
Manual Review
Recommended Mitigation Steps
Check if the user has already disapproved the proposal when trying to approve it.
if (usdUpdateProposal.disapprovals[msg.sender] == _usdProposalId)
revert ProposalAlreadyDisapprovedError();
Lines of code
https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L177-L207
Vulnerability details
Impact
The approval process of the USD price can be manipulated (either intentionally or uintentionally) if the price proposal has been first disapproved and then approved. This can lead to the manipulation of the votes needed to reach the Approval and Disapproval threshold therefore breaking the functionality of the protocol
Proof of Concept
In the functions
approveUSDPrice
anddisapproveUSDPrice
there are certain thresholds that need to be met in order for proposal to be accepted or discarded. According to the sponsor, resposible roles should not be able to change their opinions and the decision should be final.In the
disapproveUSDPrice
there is a check that the price has not been already approved or disapprovedhttps://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L225-L228
which makes the decision final. In the same function there is a counter counting disapprovals and checking the threshold before discarding the price
https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L232-L237
However, if the same user tries to approve the USD price proposal afterward, it will work since the function
approveUSDPrice
only checks if the proposal has been approved but does not check if it has been disapproved. That means that the decision in this case would not be final.https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L191-L197
This is a problem, especially in the case when approval and disapproval thresholds are different, where a single user can have an impact and reduce thresholds in both functions with his vote.
Tools Used
Manual Review
Recommended Mitigation Steps
Check if the user has already disapproved the proposal when trying to approve it.
Assessed type
Error