Lines of code

https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/LockManager.sol#L225-L226 https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/LockManager.sol#L191-L197

Vulnerability details

Impact

LockManager allows users to lock tokens in return for rewards. They can unlock their tokens once their respective unlockTime has passed.

When users unlock their tokens, AccountManager::forceHarvest() is called, which gives users reward based off the USD value of their locked tokens.

This USD value can be changed at anytime by a proposer with Pricefeed role via a call to proposeUSDPrice().

If enough Pricefeed roles approve the new USD value, that value will then be updated.

Pricefeed roles can also disapprove the proposed USD value. If enough disapprove, then the proposal will be rejected.

There is a check in place to ensure that if a Pricefeed role has already approved a proposal, then they cannot disapprove it.

However, this invariant can be broken by disapproving first and then proceeding to approve the same proposal.

Proof of Concept

When an address with Pricefeed role disapproves a proposed USD value, there is a check to see if they already approved

LockManager.sol#L225-L226

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

This is to ensure that when a proposal is approved by a Pricefeed role, the same address cannot disapprove the same proposal. This is an invariant where the same msg.sender cannot approve and disapprove the same proposal.

However, when approving, there is no check to see if the Pricefeed role has already disapproved the proposal.

#L191-L197

    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();

The same Pricefeed role can approve a proposal they already disapproved, breaking a protocol invariant.

Tools Used

Manual Review.

Recommended Mitigation Steps

Upon approval, revert if the caller had already disapproved the proposal:

    function approveUSDPrice(
        uint256 _price
    )
        external
        onlyOneOfRoles(
            [
                Role.PriceFeed_1,
                Role.PriceFeed_2,
                Role.PriceFeed_3,
                Role.PriceFeed_4,
                Role.PriceFeed_5
            ]
        )
    {
        if (usdUpdateProposal.proposer == address(0)) revert NoProposalError();
        if (usdUpdateProposal.proposer == msg.sender)
            revert ProposerCannotApproveError();
        if (usdUpdateProposal.approvals[msg.sender] == _usdProposalId)
            revert ProposalAlreadyApprovedError();
+       if (usdUpdateProposal.disapprovals[msg.sender] == _usdProposalId)
+           revert ProposalAlreadyDisapprovedError();
        if (usdUpdateProposal.proposedPrice != _price)
            revert ProposalPriceNotMatchedError();

        usdUpdateProposal.approvals[msg.sender] = _usdProposalId;
        usdUpdateProposal.approvalsCount++;

        if (usdUpdateProposal.approvalsCount >= APPROVE_THRESHOLD) {
            _execUSDPriceUpdate();
        }

        emit ApprovedUSDPrice(msg.sender);
    }

Assessed type

Invalid Validation

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

`Pricefeed roles` who disapproved USD price can still approve the same USD price, breaking protocol invariant #499