code-423n4 / 2024-02-hydradx-findings

1 stars 0 forks source link

Analysis #143

Open c4-bot-10 opened 8 months ago

c4-bot-10 commented 8 months ago

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

c4-pre-sort commented 8 months ago

0xRobocop marked the issue as sufficient quality report

c4-judge commented 7 months ago

OpenCoreCH marked the issue as grade-a

c4-judge commented 7 months ago

OpenCoreCH marked the issue as selected for report

Frankcastleauditor commented 7 months ago

Hi @OpenCoreCH
I think that this report has a lot of generics about any protocol of the same type . such as

Asset Price Calculation: Incorrect calculation of asset prices may lead to systemic risks, affecting the accuracy of trade executions and liquidity provision within the system.

which is applied to all the protocols in the ecosystem , there a lot of examples of this is the report such as

Inaccurate calculation of withdrawal fees based on spot prices and oracle prices may introduce systemic risks, impacting the fairness and efficiency of liquidity withdrawals. Centralization risks arise from the calculation of imbalances in liquidity provision, as discrepancies in determining the appropriate imbalance may impact the stability and fairness of the system.

and some of the risks are not true such as this , so the balance BalanceUpdate uses checked math of the arithmetic operation

Unchecked Arithmetic Operations: The use of unchecked arithmetic operations (CheckedAdd, CheckedSub) in the implementation of balance updates (BalanceUpdate) introduces technical risks. While these operations aim to prevent arithmetic overflow or underflow, there is still a risk of unexpected behavior if the checks fail or if the checks are not comprehensive.

the function add which is applied for the type BalanceUpdate uses the checkedAdd function as shown here , so this recommendation is invalid .

impl<Balance: Into<<FixedU128 as FixedPointNumber>::Inner> + CheckedAdd + CheckedSub + Copy + Default> Add<Balance>
    for BalanceUpdate<Balance>
{
    type Output = Option<Balance>;

    fn add(self, rhs: Balance) -> Self::Output {
        match &self {
            BalanceUpdate::Increase(amount) => rhs.checked_add(amount),
            BalanceUpdate::Decrease(amount) => rhs.checked_sub(amount),
        }
    }
}

the report has also this generic in the technical risks

Numerical Precision: The contract involves numerous calculations with fixed-point arithmetic and conversions between different numeric representations. Any miscalculations or inaccuracies in these operations could result in incorrect financial outcomes or vulnerabilities to attacks.

I need also to mention that the warden only submit an analysis without finding any high , medium , or QA issues , which indicates that the risks mentioned in the report are just generics which can be applied on all the protocol of this type .
I am asking to reconsider the validation of this report and choosing it to be selected for the final report .

albahaca0000 commented 7 months ago

Hi everyone, I often read analyses of protocols and quickly learn a lot about the protocol. Although I'm not participating in this contest, I found this analysis to be the best in explaining everything clearly compared to others, understand a lot about the HydraDX protocol.

OpenCoreCH commented 7 months ago

While the report contains some generic recommendations and errors (like most other reports), there are various good recommendations (many of which have been reported as separate issues), for instance checking the convergence of Newton's method, centralization issues because of the amplification parameter changes, valid improvement suggestions, and good attack ideas.