Audit Fixes - HackMD

Status * fixed ✅: fix is implemented * no fix 🟨: we are aware of the issue, but leave it as an intended behaviour of the system * no issue 🟥: we don't think this is an issue All the fixes we have made to the repos after the audited commit, can also be found in the following PRs: * [core](https://github.com/lyra-finance/v2-core/pull/253) * [matching](https://github.com/lyra-finance/v2-matching/pull/13) # Critical Findings ## LYRA-01 Funds can be Drained from the Protocol by Liquidating an Account During an Asset Transfer - fixed ✅ ### Response from Lyra team This issue is fixed in PR: [core#273](https://github.com/lyra-finance/v2-core/pull/273) We implemented the following fixes: 1. Issues 1 and 2 are resolved by adding a whitelist to manager data callees 2. 3 is resolved by ensuring insolvent auction bidders are solvent after taking over the account ## LYRA-02 Bids Can Be Blocked By Sending Option To Liquidator - fixed ✅ ### Response from Lyra team This issue is fixed in PR: [core#263](https://github.com/lyra-finance/v2-core/pull/263) We now disable transferring option assets and base assets to another accounts without recipient approval. The recipient must set a positive allowance for the sender. ## LYRA-03 Liquidator Can Bid With Insufficient Cash - fixed ✅ ### Response from Lyra team This issue is fixed in PR: [core#265](https://github.com/lyra-finance/v2-core/pull/265) # High Findings ## LYRA-04 Account Can Be Created With Arbitrary Manager Address - fixed ✅ ### Response from Lyra team This issue should be renamed to: **Account can be constructed to hold undesirable asset under the provided managers**. Creating accounts with arbitrary managers is intended and allowing arbitrary assets is also intended. The issue here is that switching managers did not check the assets in the incoming accounts. This issue is fixed in PR: [core#264](https://github.com/lyra-finance/v2-core/pull/264) ## LYRA-05 Fully Liquidated Auctions Cannot Be Terminated - fixed ✅ ### Response from Lyra team This issue is fixed in commit: [core#270](https://github.com/lyra-finance/v2-core/pull/270) # Medium Findings ## LYRA-06 Assets Can Be Transferred After Auction Started - fixed ✅ ### Response from Lyra team This issue should be rename to: **Liquidators can spend more than intended during liquidation**. Because asset transfer during liquidation is intended, (only risk reducing trades is permitted), the problem is the liquidator might be forced to lock more cash because of change of account. This issue is fixed in commit: [core#274](https://github.com/lyra-finance/v2-core/pull/274) ## LYRA-07 Allowances Can Be Frontran - no fix 🟨 ### Response from Lyra team We're aware of the issue and consider this a reasonable trade off in our use cases. ## LYRA-08 Session Key Deregistration Cooldowns can be Avoided - fixed ✅ ### Response from Lyra team This issue is fixed in PR: [maching#15](https://github.com/lyra-finance/v2-matching/pull/15) ## LYRA-09 Perpetual Asset Funding can be Manipulated by Applying at Peaks and Troughs - no fix 🟨 ### Response from Lyra team We are aware of this implication of the design, and we do intend to run a regular settler worker to ensure fairness. # Low Findings ## LYRA-10 Potential DoS Risk In Lyra Protocol Due To Unbounded Loop - fixed ✅ ### Response from Lyra team This issue is fixed in PR: [core#271](https://github.com/lyra-finance/v2-core/pull/271) ## LYRA-11 Dust Asset Amounts Can Amplify Gas Costs - fixed ✅ ### Response from Lyra team This issue is also fixed by the fix of [Lyra02](#Lyra02), we no longer allow transfering option asset from un-authrorized party to account. ## LYRA-12 AllowList Can Be Bypassed - no issue 🟥 ### Response from Lyra team This is the expected behavior. The current AllowList contract acts as a reference to how the system could be operated, but ideally no check is required on the protocol level. The user will also be unable to force transactions to go through on the matching contract side (as transactions can only be submitted by trusted executors who would be checking the allowlist) This has been made more explicit by some naming/comment changes in [core#276](https://github.com/lyra-finance/v2-core/pull/276) ## LYRA-13 Allowances Not Cleared On Account Transfer - no fix 🟨 ### Response from Lyra team This is an expected behaviour. Given the complexity of our codebase, it would be inefficient to clear allowance (both asset wise and subID wise) on all assets upon transfer. We did make sure that previously set allowance doesn't affect the new owner when receiving a token, so the security risk is minimized. ## LYRA-14 Id Can Overflow In Loop Causing DOS - fixed ✅ ### Response from Lyra team This issue is fixed in commit: [core#271](https://github.com/lyra-finance/v2-core/pull/271) ## LYRA-15 ForceWithdraw Can Be Blocked - no fix 🟨 ### Response from Lyra team Force withdraw is designed for someone who is no longer a legitimate user to leave the system. As the user will still be collecting interest from others even if blocked from trading; there should be a way to remove them completely. # Informational ## LYRA-16 ForceWithdraw Can Cause USDC To Become Stuck - fixed ✅ ### Response from Lyra team This does pose some risk to our matching contracts. Considering our matching contract will hold custody of everyone's account during trading on Lyra exchange, it's possible that we misconfig the Allowlist to be enabled but matching is not allowed. To avoid people just trolling the system and forceWithdraw all USDC to our matching, we put an owner only function to withdarw funds in both matching and action modules. Fixes can be found in this PR: [matching#19](https://github.com/lyra-finance/v2-matching/pull/19) ## LYRA-17 Fee-On-Transfer Tokens Are Not Supported - no fix 🟨 ### Response from Lyra team Noted ## LYRA-18 High Reentrancy Potential in subAccounts.submitTransfers() - fixed ✅ ### Response from Lyra team Noted, we added reentryguard at the `SubAccounts` level in this PR [core#279](https://github.com/lyra-finance/v2-core/pull/279), and will do more investigation on other potential path of reentrancy throughout the codebase. ## LYRA-19 Expired Options can be Created - no fix 🟨 ### Response from Lyra team Noted ## LYRA-20 Incorrect Emits - fixed ✅ ### Response from Lyra team This is fixed in PR: [core#271](https://github.com/lyra-finance/v2-core/pull/271) # Informational Findings ## LYRA-21 Maximum Values can be Set Below Current Values - no fix 🟨 ### Response from Lyra team We are aware of this situation, but will keep it as a flexibility to govern the contracts. Users are able to transfer to new subaccounts given all resulting accounts are within the limits. ## LYRA-22 Trading with Self to Make a Loss is Profitable - no fix 🟨 ### Response from Lyra team This is a expected behaviour of the system: there is a buffer between users' maintenance margin requirement and actually being insolvent. Even after someone's net maintenance margin drops below 0, they will still be far from "insolvent". The Auction system offers a market for liquidators to off-load the risk of these accounts that fall below the margin requirments. If no one take action during the solvent auction, it's expected that the whole system will suffer a loss, and everyone holding cash will be affected. ## LYRA-23 Oracles Using Perp Market Prices Can Be Manipulated - no fix 🟨 ### Response from Lyra team Perp prices will be a combination of the spot price and a TWAP of the difference of spot to perp market price (very reactive to spot, less reactive to the perp price). In the case of a sustained TWAP attack, the confidence score would also decrease (this would use a comparison of the long twap to the short twap value used on chain), which would increase margin requirements.