search

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

3 stars 1 forks source link

PriceFeed may approve the wrong proposal #91

Closed howlbot-integration[bot] closed 3 months ago

howlbot-integration[bot] commented 3 months ago

Lines of code

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

Vulnerability details

Impact

if the PriceFeed's transaction takes a long time in the mempool, then another prop node for another token with the same price may be approved, which was not originally intended.

Proof of Concept

Function approveUSDPrice dont have parameter _usdProposalId, only _price. So, if priceFeed's tx is in mempool too long, the old proposal (for which the approval was intended) was deleted (it was accepted or rejected - it doesn’t matter), and at the moment another proposal is already active, with the same price, but for a different token. The price feeder may not agree with this price for another token, but the transaction will be accepted and his vote will be counted. Explain:

  1. PriceFeed1 call proposeUSDPrice(price:1000, _contracts:0x123,0x456)
  2. PriceFeed2 agree with this price and call approveUSDPrice(1000), but this tx there is too long in mempool and still didnt included in the block.
  3. Other priceFeeds accounts approve this price and proposal has executed and deleted.
  4. PriceFeed1 call proposeUSDPrice(price:1000, _contracts:0x789) // <-- another contract!
  5. PriceFeed2's tx (from step 2) has executed and new proposal now has approve from priceFeed2, but was intended for another proposal.

Also, accounts with priceFeeed roles, could not revoke his votes. The same situation with function disapproveUSDPrice.

Tools Used

Manual review

Recommended Mitigation Steps

Add parameter address[] _contracts or uint32 _usdProposalId in approveUSDPrice() and disapproveUSDPrice()

Assessed type

Oracle

CloudEllie commented 3 months ago

See sponsor comment on #23

alex-ppg commented 3 months ago

The Blast Network operates with a sequencer model and thus does not suffer from stuck transactions in a mem-pool that may not be executed. While I accept that there is a risk associated with the current confirmation model, the likelihood of it manifesting in practice is extremely low (i.e. a price feed submitted a confirmation transaction, and a new proposal transaction, and another price feed attempted to confirm the original proposal but would confirm the new proposal) thus rendering the submission to be of QA risk.

c4-judge commented 3 months ago

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

c4-judge commented 3 months ago

alex-ppg marked the issue as grade-c