function approveUSDPrice(uint256 _price) external onlyOneOfRoles([ ....])
{
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();
usdUpdateProposal.approvals[msg.sender] = _usdProposalId;
usdUpdateProposal.approvalsCount++;
....
}
As a result any user who is already disapproved can approve the Proposal thus making the voting power of other roles irrelevant
Tools Used
Manual Review
Recommended Mitigation Steps
If contract doesnt want to allow a user who has already disapproved to approve the proposal.
Function should add this check
if (usdUpdateProposal.disapprovals[msg.sender] == _usdProposalId)
revert ProposalAlreadyDisapprovedError();
Else if , the contract allows a user who has already disapproved to approve the proposal.
Function should the code to decrement the usdUpdateProposal.disapprovalsCount and set the usdUpdateProposal.disapprovals[msg.sender] to 0;
Lines of code
https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L210-L243
Vulnerability details
Impact
A User can Approve the proposed Price even though he has disapproved before and no decrement is done for disapprovalCounts.
Proof of Concept
In the below code snippet the function is not checking whether the msg.sender has already disapproved.
https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L210-L243
As a result any user who is already disapproved can approve the Proposal thus making the voting power of other roles irrelevant
Tools Used
Manual Review
Recommended Mitigation Steps
If contract doesnt want to allow a user who has already disapproved to approve the proposal. Function should add this check
Else if , the contract allows a user who has already disapproved to approve the proposal. Function should the code to decrement the usdUpdateProposal.disapprovalsCount and set the usdUpdateProposal.disapprovals[msg.sender] to 0;
Assessed type
Context