Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Analysis #143

Open
c4-bot-10 opened this issue Feb 29, 2024 · 6 comments
Open

Analysis #143

c4-bot-10 opened this issue Feb 29, 2024 · 6 comments
Labels
A-03 analysis-advanced edited-by-warden grade-a selected for report This submission will be included/highlighted in the audit report sufficient quality report This report is of sufficient quality

Comments

@c4-bot-10
Copy link
Contributor

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

c4-bot-9 added a commit that referenced this issue Feb 29, 2024
c4-bot-8 added a commit that referenced this issue Feb 29, 2024
@c4-pre-sort c4-pre-sort added the sufficient quality report This report is of sufficient quality label Mar 3, 2024
@c4-pre-sort
Copy link

0xRobocop marked the issue as sufficient quality report

@c4-judge
Copy link

c4-judge commented Mar 8, 2024

OpenCoreCH marked the issue as grade-a

@c4-judge
Copy link

c4-judge commented Mar 8, 2024

OpenCoreCH marked the issue as selected for report

@c4-judge c4-judge added the selected for report This submission will be included/highlighted in the audit report label Mar 8, 2024
@Frankcastleauditor
Copy link

Frankcastleauditor commented Mar 15, 2024

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
Copy link

albahaca0000 commented Mar 15, 2024

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
Copy link

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.

@C4-Staff C4-Staff added the A-03 label Mar 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-03 analysis-advanced edited-by-warden grade-a selected for report This submission will be included/highlighted in the audit report sufficient quality report This report is of sufficient quality
Projects
None yet
Development

No branches or pull requests

8 participants