Skip to content

Conversation

@BoogerWooger
Copy link

@BoogerWooger BoogerWooger commented Nov 27, 2025

Change Summary

What does this PR change?
Adds PeggedSwap instruction to SwapVM

Related Issue/Ticket:
https://1inch.atlassian.net/browse/PT1-245

Testing & Verification

How was this tested?

  • Unit tests

Risk Assessment

Risk Level:

  • High - Significant changes, potential operational impact, complex rollback

Risks & Impact
Needs further studyy and checks

@claude
Copy link

claude bot commented Nov 27, 2025

Claude finished @BoogerWooger's task —— View job


Code Review: PeggedSwap Implementation

I've reviewed the implementation of the PeggedSwap instruction for SwapVM. Here's my comprehensive feedback:

Todo List:

  • Check for repository-specific guidelines (CLAUDE.md)
  • Read and analyze PeggedSwap.sol implementation
  • Review PeggedSwapMath.sol for mathematical correctness
  • Check Opcodes.sol modifications
  • Evaluate test coverage in PeggedSwap.t.sol
  • Provide comprehensive review feedback

Summary

The implementation adds a specialized AMM curve for pegged assets (stablecoins, wrapped tokens) based on the formula: √(x/X₀) + √(y/Y₀) + A(x/X₀ + y/Y₀) = 1 + A. This is a well-designed addition to SwapVM's instruction set.

✅ Strengths

  1. Solid Mathematical Foundation

    • The curve formula is appropriate for pegged assets
    • Analytical solution avoids iterative solving, improving gas efficiency
    • Hardcoded p=0.5 (square root) is a sensible choice for pegged assets
  2. Good Documentation

    • Clear ASCII art documentation explaining the curve behavior
    • Helpful parameter guidance for different asset types
    • Well-commented mathematical derivations
  3. Rounding Protection

    • Proper use of divRoundUp in critical calculations (lines 115, 127 in PeggedSwap.sol)
    • Ensures protocol-favorable rounding in both ExactIn and ExactOut modes
    • Test coverage validates rounding behavior
  4. Reasonable Test Coverage

    • Tests for different A parameter values (small/large)
    • Rounding protection tests
    • Fee integration tests
    • Price impact calculations

⚠️ Issues & Recommendations

  1. Insufficient Overflow Protection in Square Root

    // PeggedSwapMath.sol:127
    y = y * 1e9;  // This can overflow for large values of y

    Recommendation: Add overflow check or use safe multiplication to prevent potential overflow when scaling the result.

  2. Potential Precision Loss in Solve Function

    // PeggedSwapMath.sol:82
    uint256 fourARightSide = (4 * a * rightSide) / ONE;

    Recommendation: Consider reordering operations to minimize precision loss: (4 * (a * rightSide)) / ONE

  3. Missing Edge Case Tests

    • No tests for A = 0 (pure square root curve)
    • No tests for A = 2.0 (maximum allowed value)
    • No tests for extreme imbalances (e.g., 99% of liquidity on one side)

    Recommendation: Add comprehensive edge case testing.

  4. Gas Optimization Opportunities

    • The sqrt function could be optimized using assembly or Babylon's method
    • Consider caching repeated calculations like x * ONE in invariant calculations
  5. Security Consideration - Invariant Drift
    While the tests show minimal invariant differences, there's no explicit check that the invariant is maintained within acceptable bounds after swaps.

    Recommendation: Consider adding invariant validation in debug mode or as an optional safety check.

  6. Missing Validation in Args Builder
    The PeggedSwapArgsBuilder.parse function uses unsafe assembly without validating the data length first.

    function parse(bytes calldata data) internal pure returns (Args calldata args) {
        assembly ("memory-safe") {
            args := data.offset // Zero-copy to calldata pointer casting
        }
    }

    Recommendation: Validate data length before parsing to prevent reading beyond bounds.

🔍 Performance Considerations

  1. Gas Efficiency: The analytical solution is more gas-efficient than iterative approaches
  2. Storage Access: All parameters are passed in calldata, minimizing storage reads
  3. Mathematical Operations: Heavy use of multiplication/division could be optimized with bit shifting where appropriate

🛡️ Security Analysis

  1. Reentrancy: No external calls, so no reentrancy risk
  2. Integer Overflow: Most operations use Solidity 0.8.30's built-in overflow protection
  3. Access Control: Properly integrated into the SwapVM opcode system
  4. Rounding: Correctly implements protocol-favorable rounding

📊 Test Coverage Assessment

  • Good Coverage: Basic functionality, rounding protection, fee integration
  • Missing Coverage: Edge cases (A=0, A=2), extreme pool imbalances, negative test cases
  • Recommendation: Increase test coverage to ~100% with edge cases and failure scenarios

Overall Assessment

This is a well-implemented feature with solid mathematical foundations and appropriate design choices for pegged assets. The code quality is good, but there are opportunities for improvement in overflow protection, test coverage, and gas optimization. I recommend addressing the highlighted issues before merging, particularly the overflow protection in the square root function and adding comprehensive edge case tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant