Fix/concentrate fees

[Sunnesoft]

No description provided.

[claude]

Claude finished @Sunnesoft's task —— View job

---

PR Review: Fix/concentrate fees

Summary

This PR introduces significant changes to fee handling and concentrated liquidity mechanisms in the SwapVM. The changes address critical issues with fee additivity violations and precision improvements in concentrated liquidity calculations.

🟢 Positive Aspects

1. Comprehensive Test Coverage: The PR includes extensive test files (FeeOutAdditivityViolation.t.sol, XYCFeesInvariants.t.sol) that thoroughly test the fee mechanisms and invariants.

2. Precision Improvements: The concentrated liquidity calculations in XYCConcentrate.sol have been improved to use full 512-bit precision and store newL directly without division, reducing precision loss.

3. New Fee Mechanism: Introduction of _feeOutAsInXYCXD that converts output fees to equivalent input fees, helping preserve additivity for the XYC formula.

4. Detailed Documentation: The code includes helpful Russian comments explaining the problems being addressed (though these should be translated to English for consistency).

🔴 Critical Issues

1. Code Comments in Russian: The comments in Fee.sol (lines 135-159) are in Russian. These should be translated to English for consistency and maintainability.

2. Additivity Violations: The PR acknowledges but doesn't fully resolve additivity violations:

   • _flatFeeAmountOutXD violates additivity (split swaps can get more output)
   • _feeOutAsInXYCXD also violates additivity due to variable feeInBps
   • Progressive fees inherently violate additivity by design

3. Protocol Fee Accounting Issue: The comment at line 166-173 in Fee.sol mentions an accounting issue with protocol fees for exactIn swaps that needs to be addressed.

🟡 Suggestions for Improvement

1. Translation of Comments: All Russian comments should be translated to English. For example:

   // Current problems:
   // 1) Additivity violation when using output fees - splitting swaps can be profitable
   // 2) Precision loss when compensating for price range expansion in concentrated liquidity
   // 3) Protocol fees violate maker accounting - fees taken from amountOut but maker balance decreases by full amountOut

2. Consider Removing Output Fees: Given the fundamental additivity violations with output fees, consider deprecating _flatFeeAmountOutXD in favor of input fees only.

3. Protocol Fee Accounting Fix: The protocol fee accounting issue should be properly addressed, possibly by adjusting the maker's balance tracking when protocol fees are taken.

4. Gas Optimization: The gas snapshots show minimal changes, which is good. However, the concentrated liquidity operations could potentially be optimized further.

🐛 Potential Bugs

1. Division by Zero: In _feeAmountIn and _feeAmountOut, there's potential for division by zero if feeBps = BPS (100% fee).

2. Precision Loss: While improved, there's still potential for precision loss in fee calculations, especially with very small amounts or extreme fee percentages.

📊 Performance Considerations

The gas usage appears reasonable based on the snapshots:

• XYCSwap with fees: ~140k gas
• Concentrated liquidity operations: ~165k-225k gas

🔒 Security Concerns

1. Fee Manipulation: The additivity violations could potentially be exploited by sophisticated traders to extract value by splitting trades.

2. Rounding Errors: Ensure all rounding is done in favor of the protocol/maker to prevent dust attacks.

✅ Recommendations

1. Translate all comments to English
2. Add explicit warnings in documentation about additivity violations
3. Consider deprecating output fee mechanisms that violate additivity
4. Fix the protocol fee accounting issue
5. Add bounds checking for extreme fee values
6. Consider adding circuit breakers for unusual trading patterns that might exploit additivity violations

Verdict

The PR addresses important issues but introduces some concerning trade-offs. The additivity violations are particularly problematic and should be clearly documented. Consider whether the benefits of output fees outweigh the risks of arbitrage opportunities they create.

[claude]

Claude finished @Sunnesoft's task —— View job

---

PR Review: Fix/concentrate fees

Summary

This PR addresses critical issues with fee handling mechanisms and concentrated liquidity precision in the SwapVM. The changes are substantial and technically sound, but there are significant concerns about code documentation and the fundamental design trade-offs being made.

🟢 Strengths

1. Precision Improvements: The concentrated liquidity implementation now uses 512-bit precision and stores newL directly, eliminating a division operation that was causing precision loss. This is a solid technical improvement.

2. Comprehensive Testing: Excellent test coverage with two new test files that thoroughly document and demonstrate the additivity violations:

   • FeeOutAdditivityViolation.t.sol: Clear demonstrations of when splitting swaps becomes profitable
   • XYCFeesInvariants.t.sol: Thorough invariant testing for different fee mechanisms

3. New Fee Mechanism: The _feeOutAsInXYCXD function is an interesting approach to preserve additivity by converting output fees to equivalent input fees using the XYC formula.

🔴 Critical Issues

1. Russian Language Comments (Lines 135-159 in Fee.sol):

   // текущие проблемы:
   // 1) Нарушается аддитивность при использовании физов на выходе...

   These extensive Russian comments must be translated to English for maintainability. They contain critical technical information about:

   • Additivity violations with output fees
   • Precision loss in concentrated liquidity
   • Protocol fee accounting issues
   • Problems with progressive fees

2. Fundamental Additivity Violations:
   The PR acknowledges but doesn't fully resolve several additivity violations:

   • _flatFeeAmountOutXD: Split swaps can extract more output than single swaps
   • _feeOutAsInXYCXD: Still violates additivity because feeInBps depends on swap amount
   • Progressive fees: Inherently violate additivity by design

   These violations create arbitrage opportunities for sophisticated traders who can profit by splitting trades.

3. Protocol Fee Accounting Issue (Lines 166-173):
   The comment describes an unresolved accounting issue where protocol fees are deducted from amountOut but the maker's balance decreases by the full amount, creating an accounting mismatch for exactIn swaps.

🟡 Suggestions

1. Code Organization: Consider extracting the fee conversion logic into a separate library for better modularity.

2. Documentation: Add comprehensive documentation about:

   • Which fee mechanisms violate additivity and under what conditions
   • Recommended usage patterns to minimize arbitrage risks
   • Clear warnings about the trade-offs

3. Consider Deprecation: Given the fundamental issues with output fees, consider deprecating _flatFeeAmountOutXD entirely in favor of input-only fees.

4. Gas Optimization: The gas snapshots show reasonable performance (~140k for basic swaps with fees), but the concentrated liquidity operations could potentially be optimized further.

🐛 Potential Issues

1. Division by Zero Risk:

   // Line 209 in Fee.sol
   feeAmountIn = ctx.swap.amountIn * feeBps / (BPS - feeBps);

   If feeBps = BPS (100% fee), this will revert with division by zero.

2. Precision Loss in Fee Calculations: While concentrated liquidity precision was improved, fee calculations still have potential precision loss with very small amounts or extreme fee percentages.

3. TODO Comments: Multiple TODO comments (lines 205, 211, 230, 243) indicate unfinished analysis of the additivity implications.

📊 Performance

Gas usage is reasonable:

• XYC swaps with fees: ~140k gas
• Concentrated liquidity with growth: ~165k-225k gas
• No significant regression from the changes

🔒 Security Concerns

1. Arbitrage Vulnerability: The acknowledged additivity violations create real arbitrage opportunities that could be exploited at scale.

2. MEV Opportunities: Sophisticated actors could use the additivity violations to extract value through strategic trade splitting.

3. Rounding Behavior: Ensure all rounding favors the protocol/maker to prevent dust attacks.

✅ Recommendations

1. Immediate Actions:

   • Translate all Russian comments to English
   • Add explicit bounds checking for feeBps to prevent division by zero
   • Document the additivity violations clearly in the code

2. Medium Term:

   • Consider removing output fee mechanisms entirely
   • Fix the protocol fee accounting issue
   • Add monitoring/alerting for suspicious trading patterns

3. Long Term:

   • Redesign fee mechanisms to preserve additivity
   • Consider a fee collection mechanism that doesn't reinvest fees into maker balances

Verdict

This PR makes important improvements but introduces concerning trade-offs. The additivity violations are well-documented in tests but create real arbitrage risks. The Russian comments must be translated before merging. Consider whether the benefits of output fees justify the arbitrage opportunities they create.