Lines of code

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

Vulnerability details

Impact

proposer can approve and disapprove one proposal at the same time, which is not reasonable.

Proof of Concept

when disapprove, we have check to make sure user is not approve this proposal before.


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

  ...

But is approve, we don't have disapprove check. We only have the approve check, without this check , user can approve a proposal which he has disapproved before.

 function approveUSDPrice(
        uint256 _price
    )

    if (usdUpdateProposal.proposer == msg.sender)
    revert ProposerCannotApproveError();
@>    if (usdUpdateProposal.approvals[msg.sender] == _usdProposalId)
    revert ProposalAlreadyApprovedError();
    if (usdUpdateProposal.proposedPrice != _price)
    revert ProposalPriceNotMatchedError();

...

Tools Used

manual

Recommended Mitigation Steps

add disapprove check in approveUSDPrice function.

Assessed type

Access Control

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

`approveUSDPrice` don't have a check to make sure proposal has disapproval before approval. #507