Conversation
Summary of ChangesHello @luke-truitt, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the protocol's financial infrastructure by upgrading its oracle integration, refining core financial calculations, and introducing a critical liquidation price mechanism. The changes aim to improve the accuracy and robustness of price feeds, asset under management (AUM) calculations, and LP token pricing, while also providing essential tools for risk assessment and management within the system. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors how oracle prices and pool equity are handled, adds new functions for real-time price fetching and liquidation price calculation, and updates the Pool and Custody structs, improving modularity and adding new features. However, a critical security vulnerability exists due to a missing program ID constraint in the GetLiquidationPrice account validation struct, which could enable account substitution attacks. Furthermore, missing bounds checks on remaining_accounts in flash-compute could lead to transaction panics and denial of service. Beyond security, there are critical and high-severity issues related to potential integer overflows and functional discrepancies in price calculations, and a magic number should be replaced with a named constant for better maintainability.
| let mut custody_details: Box<Vec<CustodyDetails>> = Box::new(Vec::new()); | ||
| let mut pool_equity: u128 = 0; | ||
| let mut custody_prices: Vec<OraclePrice> = Vec::new(); | ||
| let mut pool_equity: u64 = 0; |
There was a problem hiding this comment.
The type of pool_equity has been changed from u128 to u64. Given that the previous implementation used u128, there's a possibility that the total pool equity could exceed u64::MAX (approximately USD_DECIMALS is 6). If the protocol is designed to handle larger values, this change could lead to an integer overflow, resulting in incorrect AUM calculations and potential loss of funds. Please confirm that u64 is sufficient for the maximum expected pool equity.
| pub max_aum_usd: u64, | ||
| pub buffer: u64, | ||
| pub raw_aum_usd: u64, | ||
| pub equity_usd: u64, |
There was a problem hiding this comment.
The fields max_aum_usd, raw_aum_usd, and equity_usd in the Pool struct have been changed from u128 to u64. Similar to the pool_equity variable in flash-compute/src/lib.rs, this change could lead to integer overflows if the total AUM or equity of the pool exceeds u64::MAX. Please ensure that u64 provides sufficient capacity for these values under all expected conditions.
| #[account( | ||
| seeds = [b"perpetuals"], | ||
| bump = perpetuals.perpetuals_bump | ||
| )] | ||
| pub perpetuals: Box<Account<'info, Perpetuals>>, | ||
|
|
||
| #[account( | ||
| seeds = [b"pool", | ||
| pool.name.as_bytes()], | ||
| bump = pool.bump | ||
| )] | ||
| pub pool: Box<Account<'info, Pool>>, | ||
|
|
||
| #[account( | ||
| seeds = [b"position", | ||
| position.owner.as_ref(), | ||
| market.key().as_ref()], | ||
| bump = position.bump | ||
| )] | ||
| pub position: Box<Account<'info, Position>>, | ||
|
|
||
| #[account( | ||
| seeds = [b"market", | ||
| target_custody.key().as_ref(), | ||
| collateral_custody.key().as_ref(), | ||
| &[market.side as u8]], | ||
| bump = market.bump | ||
| )] | ||
| pub market: Box<Account<'info, Market>>, | ||
|
|
||
| #[account( | ||
| seeds = [b"custody", | ||
| pool.key().as_ref(), | ||
| target_custody.mint.key().as_ref()], | ||
| bump = target_custody.bump | ||
| )] | ||
| pub target_custody: Box<Account<'info, Custody>>, | ||
|
|
||
| /// CHECK: oracle account for the target token | ||
| #[account( | ||
| constraint = target_oracle_account.key() == target_custody.oracle.ext_oracle_account | ||
| )] | ||
| pub target_oracle_account: AccountInfo<'info>, | ||
|
|
||
| #[account( | ||
| seeds = [b"custody", | ||
| pool.key().as_ref(), | ||
| collateral_custody.mint.key().as_ref()], | ||
| bump = collateral_custody.bump, | ||
| )] | ||
| pub collateral_custody: Box<Account<'info, Custody>>, |
There was a problem hiding this comment.
The GetLiquidationPrice struct is missing the seeds::program = FLASH_PROGRAM constraint for several accounts, including perpetuals, pool, position, market, target_custody, and collateral_custody. This causes Anchor to derive the PDA addresses using the current program's ID instead of the intended FLASH_PROGRAM ID. An attacker can exploit this by providing fake accounts owned by the current program that match these seeds, allowing them to manipulate the liquidation price calculation. This is a critical security flaw as it bypasses intended account validation.
| math::checked_as_u64(math::checked_div( | ||
| math::checked_mul( | ||
| math::checked_sub(position.collateral_usd, liabilities_usd)? as u128, | ||
| math::checked_pow(10_u128, (position.size_decimals + 3) as usize)?, |
There was a problem hiding this comment.
The magic number 3 in (position.size_decimals + 3) as usize should be replaced with a named constant. This improves readability and makes the purpose of the offset clearer. For example, LIQUIDATION_PRICE_DECIMAL_OFFSET.
| math::checked_pow(10_u128, (position.size_decimals + 3) as usize)?, | |
| math::checked_pow(10_u128, (position.size_decimals + LIQUIDATION_PRICE_DECIMAL_OFFSET) as usize)?, |
| math::checked_mul( | ||
| math::checked_sub(liabilities_usd, position.collateral_usd)? as u128, | ||
| math::checked_pow(10_u128, (position.size_decimals + 3) as usize)?, | ||
| )?, |
There was a problem hiding this comment.
The magic number 3 in (position.size_decimals + 3) as usize should be replaced with a named constant. This improves readability and makes the purpose of the offset clearer. For example, LIQUIDATION_PRICE_DECIMAL_OFFSET.
| )?, | |
| math::checked_pow(10_u128, (position.size_decimals + LIQUIDATION_PRICE_DECIMAL_OFFSET) as usize)?, |
2 participants
No description provided.