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

0 stars 0 forks source link

QA Report #545

Open howlbot-integration[bot] opened 1 month ago

howlbot-integration[bot] commented 1 month ago

See the markdown file with the details of this report here.

0xinsanity commented 1 month ago

QA Response

[L-01] This is on purpose. ConfigStorage should never be changed. [L-02] Acknowledged. [L-03] Acknowledged. [L-04] This is on purpose. Pausing is only meant for gameplay, not admin functions. [L-05] This is an incorrect interpretation of the function. Prices are meant to be set one token address at a time. The only reason we have an array there is so that we can set WETH and ETH at the same time. [L-06] Acknowledged [L-07] I don't understand this critique. [L-08] This was already fixed and requested in the error report. [L-09] Acknowledged. [L-10] Acknowledged. We only calculate based on what is left. [L-11] Acknowledged. We have no plans to support tokens like that. [L-12] lockOnBehalf has been locked down to only allow for MigrationManager calls. Calling force harvest does the same as the user calling harvest so it doesn't matter. [L-13] Acknowledged.

[N-1] Acknowledged. [N-2] Acknowledged. [N-3] Acknowledged. [N-4] Acknowledged.

michaeljyeates commented 1 month ago

[L-07] is intentional, we would only propose prices for multiple tokens if they are the same price (it is designed to update WETH and ETH at the same time)

c4-judge commented 1 month ago

alex-ppg marked the issue as grade-a

mcgrathcoutinho commented 1 month ago

Hi @alex-ppg, why is the upgraded issue being marked as partial-25?

I believe it clearly mentions the issue with a POC and solution as well. When I submitted this issue, my judgement was that price feed roles are trusted so they would not engage in such a behaviour even if it is openly available, thus marking it as QA.

Can you please re-consider this for satisfactory since I do not think it is lacking anywhere in explaining the issue?

Thank you!

alex-ppg commented 1 month ago

Hey @mcgrathcoutinho, the project's page does not mention that the Oracle members are trusted. Given that they are multiple, we cannot assume they are fully trusted as otherwise they would be only one role present. From the code alone, one can deduce that the roles are partially trusted based on their structure.

QA reported submissions that were ultimately HM cannot receive a full duplicate reward as they lack the equivalent effort of a full report. Additionally, they were misjudged as a lower risk than they actually have, indicating that a further penalty should be applied. I appreciate your contribution and will retain my original judgment.

DecentralDisco commented 1 month ago

For transparency and awarding purposes, the selected for report label has been added by staff.

DecentralDisco commented 1 month ago

For awarding purposes, staff have marked as 1st place.