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

3 stars 1 forks source link

wrong "approvalsCount" in LockManager #377

Closed howlbot-integration[bot] closed 5 months ago

howlbot-integration[bot] commented 5 months ago

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding. we are increasing usdUpdateProposal.approvalsCount in both proposeUSDPrice and approveUSDPrice.This will cause an extra increase in approvalsCount.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

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++;

    emit ProposedUSDPrice(msg.sender, _price);
}

/// @inheritdoc ILockManager
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.proposedPrice != _price)
        revert ProposalPriceNotMatchedError();

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

Tools Used

Recommended Mitigation Steps

Do not implement usdUpdateProposal.approvalsCount++ in proposeUSDPrice.

Assessed type

Context