howlbot-integration[bot] commented 3 months ago

Lines of code

https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L171

Vulnerability details

The setUSDThresholds() function in the LockManager contract is intended to configure the APPROVE_THRESHOLD and DISAPPROVE_THRESHOLD. If APPROVE_THRESHOLD is set to 1, it should execute upon the proposal creation, but it actually requires a minimum of 2 approvals to execute the proposal, leading to potential rejection even when the required threshold is met.

Impact

If the admin sets the APPROVE_THRESHOLD to 1, the proposal effectively requires 2 approvals to execute. Consequently, there is a risk that the proposal could be rejected even if it has reached the required APPROVE_THRESHOLD, especially if the DISAPPROVE_THRESHOLD is reached first.

Proof of Concept

The price update is performed by the internal function _execUSDPriceUpdate(), which is only called from approveUSDPrice(). The relevant code snippet is shown below:

200:    usdUpdateProposal.approvalsCount++;
202:    if (usdUpdateProposal.approvalsCount >= APPROVE_THRESHOLD) {
203:        _execUSDPriceUpdate();
204:    }

https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L200-L204

However, if the APPROVE_THRESHOLD is set to 1, it is achieved while creating the proposal. Refer to the proposeUSDPrice() function:

171:    usdUpdateProposal.approvalsCount++;

https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L171

This means that in order to execute _execUSDPriceUpdate(), one additional approver must call approveUSDPrice() and increase the approval count to 2.

Tools Used

Manual Review

Recommended Mitigation Steps

To ensure the proposal is executed immediately if the APPROVE_THRESHOLD is met upon creation, include a check and call to _execUSDPriceUpdate() in the proposeUSDPrice() function.

Here is the suggested code modification:

function proposeUSDPrice(
    uint256 _price,
    address[] calldata _contracts
)
    external
    onlyOneOfRoles(
        [
            Role.PriceFeed_1,
            Role.PriceFeed_2,
            Role.PriceFeed_3,
            Role.PriceFeed_4,
            Role.PriceFeed_5
        ]
    )
{
    if (usdUpdateProposal.proposer != address(0))
        revert ProposalInProgressError();
    if (_contracts.length == 0) revert ProposalInvalidContractsError();

    delete usdUpdateProposal;

    // Approvals will use this because when the struct is deleted the approvals remain
    ++_usdProposalId;

    usdUpdateProposal.proposedDate = uint32(block.timestamp);
    usdUpdateProposal.proposer = msg.sender;
    usdUpdateProposal.proposedPrice = _price;
    usdUpdateProposal.contracts = _contracts;
    usdUpdateProposal.approvals[msg.sender] = _usdProposalId;
    usdUpdateProposal.approvalsCount++;

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

    emit ProposedUSDPrice(msg.sender, _price);
}

This ensures that the proposal is executed immediately if the APPROVE_THRESHOLD is met upon creation, preventing the need for an additional approval and mitigating the risk of the proposal being rejected due to simultaneous threshold reaching.

Assessed type

Other

c4-judge commented 3 months ago

alex-ppg changed the severity to QA (Quality Assurance)