feat: UniswapV4 mint/increase/decrease liquidity support #189

deluca-mike · 2025-11-10T15:25:09Z

Summary by CodeRabbit

New Features
- Uniswap V4 integration: create, increase, and decrease positions plus swaps via relayer-enabled actions.
- Per-pool configurable tick limits for risk control.
Interfaces
- Added token/permit and position/router interfaces to support approvals and position queries.
Tests
- Expanded mainnet-fork test suite covering liquidity flows, swaps, tick limits, rate-limits, and access control.
Chores
- Added Uniswap V4 submodules and build remapping for integration.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

coderabbitai · 2025-11-10T15:25:20Z

Walkthrough

Adds Uniswap V4 support: new git submodules and Foundry remapping, a UniswapV4Lib library, MainnetController relayer actions and per-pool tick limits, new interfaces, rate-limit helper signature changes, and extensive mainnet-fork tests.

Changes

Cohort / File(s)	Summary
Git submodules & Foundry config `.gitmodules`, `lib/uniswap-v4-periphery`, `lib/uniswap-v4-core`, `foundry.toml`	Adds `lib/uniswap-v4-core` and `lib/uniswap-v4-periphery` submodules and a Foundry remapping entry `@uniswap/v4-core/=lib/uniswap-v4-core/`. Submodule entries record commit hashes.
Core Uniswap V4 library `src/libraries/UniswapV4Lib.sol`	New library providing mint/increase/decrease position helpers, Permit2 approval flow, tick limits struct/constants (`LIMIT_DEPOSIT`, `LIMIT_WITHDRAW`), balance normalization, and rate-limit delta computations.
MainnetController extensions `src/MainnetController.sol`	Adds `uniswapV4TickLimits` mapping, `UniswapV4TickLimitsSet` event, `setUniswapV4TickLimits` admin setter, public LIMIT constants, and relayer-accessible actions: `mintPositionUniswapV4`, `increaseLiquidityUniswapV4`, `decreaseLiquidityUniswapV4`.
Interfaces `src/interfaces/Common.sol`, `src/interfaces/UniswapV4.sol`	Adds `IERC20Like` and `IPermit2Like` interfaces in `Common.sol`. Adds `IPositionManagerLike` interface and related type imports in `UniswapV4.sol`.
Rate-limit helpers `src/RateLimitHelpers.sol`	Renames internal parameter identifiers and key helpers: replaces the old `makeUint32Key(bytes32,uint32 domain)` signature with `makeBytes32Key(bytes32,bytes32)` and updated `makeUint32Key(bytes32,uint32)`; also renames address params in helpers and updates keccak256 encodings.
Tests `test/mainnet-fork/Uniswapv4.t.sol`, `test/mainnet-fork/Curve.t.sol`	Adds comprehensive Uniswap V4 mainnet-fork test suite (`MainnetControllerUniswapV4Tests`) with mocks and many scenarios; removes an unused `IERC4626` import from `Curve.t.sol`.
Submodule pointers `lib/uniswap-v4-core/...`, `lib/uniswap-v4-periphery/...`	Records specific submodule commit hashes for core and periphery; no in-repo code changes beyond pointers.

Sequence Diagram(s)

sequenceDiagram
    actor Relayer
    participant MC as MainnetController
    participant V4L as UniswapV4Lib
    participant P2 as Permit2
    participant PM as PositionManager
    participant RL as RateLimits

    Relayer->>MC: mintPositionUniswapV4(poolId, tickLower, tickUpper, liquidity, ...)
    activate MC
    MC->>V4L: mintPosition(proxy, rateLimits, poolId, tickLower, tickUpper, liquidity, ...)
    activate V4L
    V4L->>V4L: validate poolId & tickLimits
    V4L->>P2: approve token allowances (Permit2)
    V4L->>PM: call modifyLiquidities / router execute (calldata)
    V4L->>V4L: compute pre/post balances, normalize deltas
    V4L->>RL: update rate-limits (LIMIT_DEPOSIT / LIMIT_WITHDRAW)
    V4L->>P2: reset approvals
    deactivate V4L
    MC-->>Relayer: return success
    deactivate MC

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Pay special attention to:
- src/libraries/UniswapV4Lib.sol (approval flows, calldata construction, tick-limit enforcement, rate-limit math)
- src/MainnetController.sol (access control, nonReentrant usage, public exposure of limits)
- src/RateLimitHelpers.sol (signature renames and all call-sites)
- Tests in test/mainnet-fork/Uniswapv4.t.sol (mock correctness and alignment with new interfaces)

Possibly related PRs

feat: Add Reentrancy Guards (SC-1224) #185 — overlaps on ReentrancyGuard/nonReentrant additions in MainnetController and similar Uniswap action protections.

Suggested reviewers

hacker-DOM
lucas-manuel
supercontracts
hivtushok

Poem

🐰 In tick-lined burrows we hop and play,
Approvals set, limits marked today,
Liquidity sown, positions rise and fall,
Tests chase the carrots down each call,
A rabbit cheers for V4 — hop, hooray! 🥕

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'feat: UniswapV4 mint/increase/decrease liquidity support' follows the required format (word + colon prefix), is concise, and accurately describes the main changes (adding Uniswap V4 liquidity management support).
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch sc-1234-uniswap-v4-support

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

lucas-manuel · 2025-11-11T04:12:26Z

@coderabbitai Review this PR and be very strict. Also provide a summary of how exactly this interacts with UniswapV4 and consider potential attack vectors.

coderabbitai · 2025-11-11T04:17:26Z

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Get PR details
gh pr view 189 --json title,body,files,additions,deletions,changedFiles

# Get the full diff
gh pr diff 189

Length of output: 83390

@lucas-manuel I'll conduct a thorough review of this PR. Let me examine the code in detail.

🏁 Script executed:

#!/bin/bash
# Get the full content of the new UniswapV4Lib.sol file
cat src/libraries/UniswapV4Lib.sol

Length of output: 15672

🏁 Script executed:

#!/bin/bash
# Check the MainnetController changes for the poolId to address casting
rg -n "address\(uint160\(uint256\(" src/MainnetController.sol

# Also check how maxSlippages is defined
rg -n "mapping.*maxSlippages" src/MainnetController.sol

# Check for any other poolId usage
rg -n "poolId" src/MainnetController.sol -A 2 -B 2

Length of output: 3415

🏁 Script executed:

#!/bin/bash
# Check if there's any ownership validation in burn/decrease functions
rg -n "ownerOf" src/libraries/UniswapV4Lib.sol -B 5 -A 5

# Check the test for burnPosition to see if there's any ownership expectations
rg -n "test_burnPosition" test/mainnet-fork/Uniswapv4.t.sol -A 20

# Check what happens with the proxy parameter in burn
rg -n "commonParams.proxy" src/libraries/UniswapV4Lib.sol -B 2 -A 2

Length of output: 7641

🏁 Script executed:

#!/bin/bash
# Check if burnPosition and decreasePosition validate ownership anywhere
rg -n "burnPosition|decreasePosition" src/libraries/UniswapV4Lib.sol -B 10 -A 30 | head -100

# Check if Position Manager has any ownership checks built in
rg -n "BURN_POSITION|DECREASE_LIQUIDITY" test/mainnet-fork/Uniswapv4.t.sol -B 5 -A 5

Length of output: 3184

🏁 Script executed:

#!/bin/bash
# Check if Uniswap V4's Position Manager has built-in ownership checks
# Let's look at the actual burn and decrease test implementations
rg -n "_burnPosition|_decreasePosition" test/mainnet-fork/Uniswapv4.t.sol -A 40 | head -100

# Also check if there are any comments about ownership in the main functions
rg -n "owner" src/libraries/UniswapV4Lib.sol -B 3 -A 3

# Check the proxy doCall implementation
rg -n "doCall" src/interfaces/IALMProxy.sol

Length of output: 4695

🏁 Script executed:

#!/bin/bash
# Let's verify the critical ownership issue by checking the actual implementation
cat src/libraries/UniswapV4Lib.sol | grep -A 20 "function burnPosition"
cat src/libraries/UniswapV4Lib.sol | grep -A 20 "function decreasePosition"

# Check if Uniswap's Position Manager has built-in ownership checks
rg -n "ownerOf" test/mainnet-fork/Uniswapv4.t.sol

# Let's also check the bytes25 casting
rg -n "bytes25" src/libraries/UniswapV4Lib.sol -B 2 -A 2

Length of output: 2106

🏁 Script executed:

#!/bin/bash
# Let me verify the slippage logic interpretation by checking the tests
rg -n "amount0Max\|amount1Max" test/mainnet-fork/Uniswapv4.t.sol -B 2 -A 2 | head -50

# Check how maxSlippage is set in tests
rg -n "setMaxSlippage.*0.98" test/mainnet-fork/Uniswapv4.t.sol -B 2 -A 2

# Check the actual slippage calculation in tests
rg -n "maxSlippage.*1e18" test/mainnet-fork/Uniswapv4.t.sol -B 3 -A 3

Length of output: 4055

🏁 Script executed:

#!/bin/bash
# Check if there's any test that verifies burning a position owned by someone else
rg -n "transferFrom.*almProxy" test/mainnet-fork/Uniswapv4.t.sol -B 5 -A 10

# Check the ERC4626Lib changes for any issues
cat src/libraries/ERC4626Lib.sol

# Check if there are proper checks for the poolId bytes25 casting in interfaces
cat src/interfaces/UniswapV4.sol

Length of output: 5162

@lucas-manuel I've completed a thorough security review of this PR. Here are my findings:

🔴 CRITICAL SECURITY ISSUES

1. Missing Ownership Validation in `burnPosition` and `decreasePosition`

Location: src/libraries/UniswapV4Lib.sol Lines 127-166 and 168-206

Issue: The burnPosition and decreasePosition functions do NOT verify that the proxy owns the NFT position before burning/decreasing it. In contrast, increasePosition correctly validates ownership:

// increasePosition has this check (Line 88-91):
require(
    IPositionManagerLike(_POSITION_MANAGER).ownerOf(tokenId) == commonParams.proxy,
    "MC/non-proxy-position"
);

// But burnPosition and decreasePosition are missing this check!

Impact: A malicious or compromised relayer could potentially burn or decrease liquidity from positions that don't belong to the proxy. While Uniswap V4's Position Manager likely has built-in ownership checks, you should never rely solely on external contract protections. The contract must enforce its own invariants.

Recommendation: Add ownership validation to both functions:

function burnPosition(
    CommonParams calldata commonParams,
    uint256               tokenId,
    uint256               amount0Min,
    uint256               amount1Min
) external returns (uint256 rateLimitDecrease) {
    // ADD THIS CHECK:
    require(
        IPositionManagerLike(_POSITION_MANAGER).ownerOf(tokenId) == commonParams.proxy,
        "MC/non-proxy-position"
    );
    
    _requirePoolIdMatch(commonParams.poolId, tokenId);
    // ... rest of function
}

Do the same for decreasePosition.

2. PoolId to Address Casting Creates Collision Risk

Location: src/MainnetController.sol Lines 661, 691, 717, 743

Issue: The code converts bytes32 poolId to address by casting to uint160, taking only the lower 160 bits:

maxSlippage : maxSlippages[address(uint160(uint256(poolId)))]

The comment acknowledges this: "NOTE: maxSlippages is a mapping from address to uint256, so we have to take the lower 160 bits of the id. It is possible, buit highly unliekly there us a collision."

Impact:

Different poolIds could theoretically map to the same address, causing them to share the same maxSlippage value
An attacker could potentially find collisions to exploit misconfigurations
Typo in comment: "buit highly unliekly there us" should be "but highly unlikely there is"

Recommendation: Create a dedicated mapping for poolId-specific configuration:

mapping(bytes32 poolId => uint256 maxSlippage) public uniswapV4MaxSlippages;

This eliminates the collision risk entirely and makes the code clearer.

3. bytes32 to bytes25 Casting Truncates Data

Location: src/libraries/UniswapV4Lib.sol Line 387

Issue: The _getPoolKey function casts bytes32 to bytes25, truncating 7 bytes:

function _getPoolKey(bytes32 poolId) internal view returns (PoolKey memory poolKey) {
    return IPositionManagerLike(_POSITION_MANAGER).poolKeys(bytes25(poolId));
}

Impact: If the poolId uses all 32 bytes, this silently discards data, potentially causing incorrect pool lookups.

Recommendation:

Verify that Uniswap V4's design only uses 25 bytes for poolId
Add validation to ensure the truncated bytes are zero:

function _getPoolKey(bytes32 poolId) internal view returns (PoolKey memory poolKey) {
    // Ensure the last 7 bytes are zero
    require(uint56(uint256(poolId)) == 0, "MC/invalid-poolId-format");
    return IPositionManagerLike(_POSITION_MANAGER).poolKeys(bytes25(poolId));
}

🟠 HIGH SEVERITY ISSUES

4. TODO Comments in Production Code

Location: src/MainnetController.sol Lines 660, 690, 716, 742

Issue: Multiple // TODO: Use central state contract comments indicate incomplete implementation.

Recommendation: Either implement the central state contract or remove the TODOs and document why the current approach is acceptable for production.

5. Unconventional Slippage Calculation

Location: src/libraries/UniswapV4Lib.sol Lines 279-287 and 343-351

Issue: The slippage validation logic is non-standard:

For deposits:

require(
    amount0Max * commonParams.maxSlippage <= 
    _clampedSub(startingBalance0, endingBalance0) * 1e18,
    "MC/amount0Max-too-high"
);

This checks: amountMax * slippage <= actualSpent * 1e18

With maxSlippage = 0.98e18, this ensures: amountMax <= actualSpent / 0.98, or actualSpent >= 0.98 * amountMax

Analysis: This is correct but unusual. The typical pattern checks actualSpent <= amountMax * (1 + tolerance). Your pattern enforces actualSpent >= amountMax * slippage.

Recommendation: Add comprehensive documentation explaining the slippage semantics, as this is not intuitive:

// maxSlippage represents the minimum efficiency ratio (e.g., 0.98 = 98%)
// We require: actualSpent >= amountMax * maxSlippage
// This means: if maxSlippage is 0.98, actualSpent must be at least 98% of amountMax

🟡 MEDIUM SEVERITY ISSUES

6. Permit2 Approval Pattern Uses Low-Level Call

Location: src/libraries/UniswapV4Lib.sol Lines 218-222

Issue: The first approval to Permit2 uses .call() and ignores failures:

// Approve the Permit2 contract to spend none of the token (success is optional).
proxy.call(
    abi.encodeCall(
        IALMProxy.doCall,
        (token, abi.encodeCall(IERC20Like.approve, (_PERMIT2, 0)))
    )
);

Recommendation: Add a comment explaining why failures are acceptable here (likely to reset approvals for tokens that revert on non-zero approval changes).

7. Rate Limit Based on Normalized Balance

Location: src/libraries/UniswapV4Lib.sol Lines 289-297

Issue: Rate limits sum both tokens normalized to 18 decimals. This means:

A pool with USDC (6 decimals) and WETH (18 decimals) treats $1 USDC = $1 WETH equivalently
This doesn't account for actual USD values

Example: If WETH is $3000 and USDC is $1, depositing 1 WETH + 3000 USDC would count as:

Normalized: 1e18 + 3000e18 = 3001e18
Actual value: $3000 + $3000 = $6000

Recommendation: Document this limitation and consider if USD-normalized rate limits would be more appropriate.

How This Interacts with UniswapV4

Architecture:

Permit2 Approval Flow: Token → Permit2 → Position Manager (standard for Uniswap V4)
NFT-Based Positions: Each liquidity position is an ERC-721 NFT with a unique tokenId
Action Encoding: Operations use Uniswap V4's Actions enum (MINT_POSITION, INCREASE_LIQUIDITY, etc.)
Balance-Based Validation: Instead of trusting return values, the code measures actual token balance changes

Protection Mechanisms:

Tick Range Limits: uniswapV4Limits mapping restricts positions to admin-approved tick ranges
Slippage Protection: maxSlippage ensures operations execute within acceptable efficiency bounds
Rate Limiting: Per-pool deposit/withdrawal limits prevent rapid capital movements
Ownership Validation: (MISSING for burn/decrease - see critical issue feat: Set up repo with ACL and upgradeability (SC-550) #1)

Attack Vectors:

Relayer Compromise: If a relayer is compromised:
- ✅ Cannot mint positions outside allowed tick ranges
- ✅ Cannot exceed rate limits
- ❌ Could burn/decrease ANY position (if ownership check is missing)
- ✅ Limited by slippage controls
PoolId Collision Attack:
- Attacker finds two poolIds that map to same address after uint160 casting
- Misconfigures one pool to exploit the other
- Likelihood: Low but non-zero
Sandwich Attacks:
- Relayer front-runs with unfavorable price movement
- Mitigation: Slippage checks and deadline parameters
Rate Limit Bypass:
- Multiple poolIds with colliding addresses could share rate limit
- Mitigation: Fix issue feat: Add AllocatorVault mint/burn NST basic functionality and testing structure (SC-493) #2

Recommendations Priority

IMMEDIATE: Add ownership checks to burnPosition and decreasePosition
IMMEDIATE: Replace poolId→address casting with dedicated mapping
HIGH: Validate bytes25 casting or add checks for truncation safety
HIGH: Resolve or remove TODO comments
MEDIUM: Document slippage calculation semantics
MEDIUM: Document rate limiting assumptions about token values

Test Coverage: The test suite is comprehensive (949 lines) and covers edge cases well. However, add tests for the missing ownership validation scenario.

Would you like me to generate the specific code fixes for these issues or open GitHub issues for tracking?

🐰

deluca-mike · 2025-11-11T19:42:43Z

This will address @coderabbitai's thorough security review.

Missing Ownership Validation in burnPosition and decreasePosition
- Not needed since burning someone else's position and claiming the underlying tokens would be a, however unlikely, a benefit for this system.
PoolId to Address Casting Creates Collision Risk
- Not much of an issue because the max slippages would already be set to reasonable values (if not, zero), so finding a collision with 2 pool Ids would be hard, but is made more unlikely by the fact that one of these pool IDs needs to be supported by the system, and the other pool ID needs to be one that maps to an actual pool with valid and supported tokens.
bytes32 to bytes25 Casting Truncates Data
- Not an issue as Uniswap V4's Position Manager uses the same 25-byte pool IDs
TODO Comments in Production Code
- We will remove these TODOs as this will be handled in a future major version where integrations define their own storage
Unconventional Slippage Calculation
- Yes, our definition of slippage in this system is "inverted", but it is consistent so we will keep it is as, unaddressed. A future major version may likely correct this.
Permit2 Approval Pattern Uses Low-Level Call
- A note in comments will address this.
Rate Limit Based on Normalized Balance
- Will make a note in comments.

coderabbitai · 2025-11-11T19:43:17Z

Tip

For best results, initiate chat on the files or code changes.

@deluca-mike Thank you for the detailed response and thoughtful explanations! Your reasoning is sound on all points:

Ownership Validation - Understood. If an attacker burns someone else's position, the underlying tokens flow back to the proxy, which indeed benefits the system rather than harming it. This is a clever design property.
PoolId Collision - Agreed. The attack surface is minimal given that both poolIds would need to map to valid, supported pools with reasonable maxSlippage configurations already in place.
bytes32→bytes25 Casting - Confirmed that this aligns with Uniswap V4's Position Manager design, so no data loss occurs.
TODO Comments - Sounds good. Looking forward to the centralized storage approach in the future version.
Inverted Slippage - Acknowledged. Consistency across the system is key, and your implementation is consistent.
Permit2 Low-Level Call - Great, the comment will help future reviewers understand why failure is acceptable here.
Rate Limit Normalization - Perfect, documentation will clarify the token-value assumptions.

Thank you for considering the security review so thoroughly! Let me know if you need any assistance with the comment additions or have other questions.

✏️ Learnings added

Learnt from: deluca-mike
URL: https://github.com/sparkdotfi/spark-alm-controller/pull/189

Timestamp: 2025-11-11T19:43:16.018Z
Learning: In spark-alm-controller, the burnPosition and decreasePosition functions in UniswapV4Lib.sol intentionally do not validate ownership because burning someone else's position would cause the underlying tokens to flow to the proxy, which benefits the system rather than harming it.

Learnt from: deluca-mike
URL: https://github.com/sparkdotfi/spark-alm-controller/pull/189

Timestamp: 2025-11-11T19:43:16.018Z
Learning: In spark-alm-controller, the slippage calculation is intentionally "inverted" compared to conventional patterns - maxSlippage represents a minimum efficiency ratio (e.g., 0.98 = 98%), where actualSpent >= amountMax * maxSlippage. This is consistent across the system.

Learnt from: deluca-mike
URL: https://github.com/sparkdotfi/spark-alm-controller/pull/189

Timestamp: 2025-11-11T19:43:16.018Z
Learning: In spark-alm-controller, rate limits for Uniswap positions sum both tokens normalized to 18 decimals, which doesn't account for actual USD values. This is a known design decision where normalized balance is used rather than USD-denominated values.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

coderabbitai

Actionable comments posted: 3

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 241b131 and 0122e2c.

📒 Files selected for processing (15)

.gitmodules (1 hunks)
.lintstagedrc (1 hunks)
.prettierignore (1 hunks)
.prettierrc (1 hunks)
.solhint.json (1 hunks)
foundry.toml (1 hunks)
lib/uniswap-v4-core (1 hunks)
lib/uniswap-v4-periphery (1 hunks)
src/MainnetController.sol (8 hunks)
src/RateLimitHelpers.sol (1 hunks)
src/interfaces/Common.sol (1 hunks)
src/interfaces/UniswapV4.sol (1 hunks)
src/libraries/UniswapV4Lib.sol (1 hunks)
test/mainnet-fork/Curve.t.sol (0 hunks)
test/mainnet-fork/Uniswapv4.t.sol (1 hunks)

💤 Files with no reviewable changes (1)

test/mainnet-fork/Curve.t.sol

🧰 Additional context used

🧠 Learnings (6)

📓 Common learnings

Learnt from: deluca-mike
Repo: sparkdotfi/spark-alm-controller PR: 0
File: :0-0
Timestamp: 2025-11-11T19:43:16.018Z
Learning: In spark-alm-controller, the burnPosition and decreasePosition functions in UniswapV4Lib.sol intentionally do not validate ownership because burning someone else's position would cause the underlying tokens to flow to the proxy, which benefits the system rather than harming it.

Learnt from: deluca-mike
Repo: sparkdotfi/spark-alm-controller PR: 0
File: :0-0
Timestamp: 2025-11-11T19:43:16.018Z
Learning: In spark-alm-controller, rate limits for Uniswap positions sum both tokens normalized to 18 decimals, which doesn't account for actual USD values. This is a known design decision where normalized balance is used rather than USD-denominated values.

📚 Learning: 2025-11-04T17:15:21.481Z

Learnt from: deluca-mike
Repo: sparkdotfi/spark-alm-controller PR: 185
File: test/base-fork/Aave.t.sol:59-63
Timestamp: 2025-11-04T17:15:21.481Z
Learning: In the spark-alm-controller repository, test files in test/base-fork/ use ForkTestBase from test/base-fork/ForkTestBase.t.sol where _setControllerEntered() defaults to writing the reentrancy guard state to foreignController, while test files in test/mainnet-fork/ use ForkTestBase from test/mainnet-fork/ForkTestBase.t.sol where _setControllerEntered() defaults to mainnetController.

Applied to files:

test/mainnet-fork/Uniswapv4.t.sol

📚 Learning: 2025-11-04T17:15:39.489Z

Learnt from: deluca-mike
Repo: sparkdotfi/spark-alm-controller PR: 185
File: test/base-fork/Morpho.t.sol:122-126
Timestamp: 2025-11-04T17:15:39.489Z
Learning: In the spark-alm-controller repository, tests in the `test/base-fork/` directory use `ForkTestBase` from `test/base-fork/ForkTestBase.t.sol`, where the `_setControllerEntered()` helper function defaults to setting the reentrancy guard state on `foreignController`. Tests in `test/mainnet-fork/` use their own `ForkTestBase` which defaults to `mainnetController`.

Applied to files:

test/mainnet-fork/Uniswapv4.t.sol

📚 Learning: 2025-11-11T19:43:16.018Z

Learnt from: deluca-mike
Repo: sparkdotfi/spark-alm-controller PR: 0
File: :0-0
Timestamp: 2025-11-11T19:43:16.018Z
Learning: In spark-alm-controller, the burnPosition and decreasePosition functions in UniswapV4Lib.sol intentionally do not validate ownership because burning someone else's position would cause the underlying tokens to flow to the proxy, which benefits the system rather than harming it.

Applied to files:

test/mainnet-fork/Uniswapv4.t.sol
src/MainnetController.sol
src/libraries/UniswapV4Lib.sol
src/interfaces/UniswapV4.sol

📚 Learning: 2025-11-11T19:43:16.018Z

Learnt from: deluca-mike
Repo: sparkdotfi/spark-alm-controller PR: 0
File: :0-0
Timestamp: 2025-11-11T19:43:16.018Z
Learning: In spark-alm-controller, rate limits for Uniswap positions sum both tokens normalized to 18 decimals, which doesn't account for actual USD values. This is a known design decision where normalized balance is used rather than USD-denominated values.

Applied to files:

test/mainnet-fork/Uniswapv4.t.sol
src/MainnetController.sol
src/libraries/UniswapV4Lib.sol

📚 Learning: 2025-10-16T17:40:33.927Z

Learnt from: supercontracts
Repo: sparkdotfi/spark-alm-controller PR: 170
File: test/mocks/Mocks.sol:25-134
Timestamp: 2025-10-16T17:40:33.927Z
Learning: In the spark-alm-controller repository, test mock contracts like MockTokenkReturnNull do not need to include ERC20 standard events (Transfer, Approval) for testing purposes. Full ERC20 compliance with events is not required for test mocks.

Applied to files:

test/mainnet-fork/Uniswapv4.t.sol

🔇 Additional comments (7)

lib/uniswap-v4-core (1)

1-1: Submodule configuration is correct, but commit pinning requires verification.

Configuration verification passed:

.gitmodules is correctly configured with proper URL and path

foundry.toml remapping (@uniswap/v4-core/=lib/uniswap-v4-core/) is consistent

Submodule path and status are properly aligned

However, the pinned commit 59d3ecf53afa9264a16bba0e38f4c5d2231f80bc does not correspond to any tagged release. The only GitHub tag for Uniswap v4-core is v4.0.0 (tagged Jan 21, 2025), and its commit hash differs from the one pinned here.

Confirm that:

This non-release commit pinning is intentional (e.g., for testing, pre-release evaluation, or specific bug fixes).

The rationale is documented or agreed upon with the team.

There is a plan to migrate to a tagged release version once available.

.solhint.json (1)

10-73: Note the security-relevant rule disables.

The configuration intentionally disables several security-focused linting rules:

avoid-call-value and avoid-low-level-calls (lines 10-11): Permit low-level calls required for patterns like Permit2

no-inline-assembly (line 72): Allows assembly blocks

not-rely-on-time (line 73): Permits time-dependent logic

These are standard for DeFi projects but require careful review of the actual low-level calls and assembly usage in the implementation.

foundry.toml (1)

21-21: LGTM!

The Uniswap V4 remapping is correctly configured and follows the project's pattern for external dependencies.

.gitmodules (1)

50-55: LGTM!

The Uniswap V4 submodule additions are correctly configured with official repository URLs and align with the remapping in foundry.toml.

src/interfaces/Common.sol (2)

4-12: LGTM!

The IERC20Like interface provides the minimal necessary functions for token operations used in the integration.

14-18: LGTM!

The IPermit2Like interface correctly defines the approve function signature matching Permit2's specification (uint160 for amount, uint48 for expiration).

src/interfaces/UniswapV4.sol (1)

9-21: LGTM!

The IPositionManagerLike interface correctly defines the essential functions for interacting with Uniswap V4's Position Manager, including pool/position queries, liquidity modification, and ownership checks.

lib/uniswap-v4-periphery

src/MainnetController.sol

coderabbitai

Actionable comments posted: 9

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

foundry.toml (1)
13-22: Add remap for @uniswap/v4-periphery to match Uniswap imports

You’ve added a remap for @uniswap/v4-core, but src/interfaces/UniswapV4.sol still imports v4‑periphery via a long relative path. For consistency and portability, add a matching remap and use both remapped paths in the interface.

Example:
 remappings = [
@@
-    '@uniswap/v4-core/=lib/uniswap-v4-core/'
+    '@uniswap/v4-core/=lib/uniswap-v4-core/',
+    '@uniswap/v4-periphery/=lib/uniswap-v4-periphery/'
 ]

♻️ Duplicate comments (3)

lib/uniswap-v4-periphery (1)

1-1: Submodule commit hash remains invalid; prior critical issue unresolved.

The commit hash 60cd93803ac2b7fa65fd6cd351fd5fd4cc8c9db5 does not exist in the official Uniswap v4-periphery repository, as confirmed in the prior review. This will cause the build/test to fail when initializing the submodule. Update to a valid commit from the official repository (e.g., 3779387e5d296f39df543d23524b050f89a62917), and ensure coordination with the lib/uniswap-v4-core submodule for API compatibility.
src/interfaces/UniswapV4.sol (1)
4-7: Prefer remapped Uniswap imports instead of long relative paths

These imports bypass your Foundry remappings and hard‑code the repo layout:
import { PoolId }  from "../../lib/uniswap-v4-core/src/types/PoolId.sol";
import { PoolKey } from "../../lib/uniswap-v4-core/src/types/PoolKey.sol";
import { PositionInfo } from "../../lib/uniswap-v4-periphery/src/libraries/PositionInfoLibrary.sol";
Now that you have a remap for v4‑core (and can add one for v4‑periphery), it’s cleaner to use the remapped paths.

Suggested change (after adding the v4‑periphery remap in foundry.toml):
-import { PoolId }  from "../../lib/uniswap-v4-core/src/types/PoolId.sol";
-import { PoolKey } from "../../lib/uniswap-v4-core/src/types/PoolKey.sol";
-
-import { PositionInfo } from "../../lib/uniswap-v4-periphery/src/libraries/PositionInfoLibrary.sol";
+import { PoolId }  from "@uniswap/v4-core/src/types/PoolId.sol";
+import { PoolKey } from "@uniswap/v4-core/src/types/PoolKey.sol";
+import { PositionInfo } from "@uniswap/v4-periphery/src/libraries/PositionInfoLibrary.sol";
src/MainnetController.sol (1)
635-752: Restore nonReentrant on Uniswap V4 relayer entrypoints

mintPositionUniswapV4 is external nonReentrant, but the three sibling entrypoints:

increaseLiquidityUniswapV4

burnPositionUniswapV4

decreaseLiquidityUniswapV4

are now plain external and still delegate to UniswapV4Lib, which ultimately reaches Uniswap V4 pools where arbitrary hook contracts can execute.

Even with _checkRole(RELAYER), a RELAYER‑level aggregator contract can be invoked from a hook and re‑enter these functions within the same transaction, giving a hook‑controlled call graph access to additional privileged paths and to your rate‑limit updates. The rest of the relayer surface is consistently guarded with nonReentrant, so these three functions stand out as the only Uniswap V4 paths without that protection.

I recommend re‑adding nonReentrant to align with the rest of the contract and to close this hook‑driven reentrancy window.

Suggested diff:
-    function increaseLiquidityUniswapV4(
+    function increaseLiquidityUniswapV4(
         bytes32 poolId,
         uint256 tokenId,
         uint128 liquidityIncrease,
         uint256 amount0Max,
         uint256 amount1Max
     )
-        external
+        external nonReentrant
     {
@@
-    function burnPositionUniswapV4(
+    function burnPositionUniswapV4(
         bytes32 poolId,
         uint256 tokenId,
         uint256 amount0Min,
         uint256 amount1Min
     )
-        external
+        external nonReentrant
     {
@@
-    function decreaseLiquidityUniswapV4(
+    function decreaseLiquidityUniswapV4(
         bytes32 poolId,
         uint256 tokenId,
         uint128 liquidityDecrease,
         uint256 amount0Min,
         uint256 amount1Min
     )
-        external
+        external nonReentrant
     {

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0122e2c and 6c26dfc.

📒 Files selected for processing (15)

.gitmodules (1 hunks)
.lintstagedrc (1 hunks)
.prettierignore (1 hunks)
.prettierrc (1 hunks)
.solhint.json (1 hunks)
foundry.toml (1 hunks)
lib/uniswap-v4-core (1 hunks)
lib/uniswap-v4-periphery (1 hunks)
src/MainnetController.sol (8 hunks)
src/RateLimitHelpers.sol (1 hunks)
src/interfaces/Common.sol (1 hunks)
src/interfaces/UniswapV4.sol (1 hunks)
src/libraries/UniswapV4Lib.sol (1 hunks)
test/mainnet-fork/Curve.t.sol (0 hunks)
test/mainnet-fork/Uniswapv4.t.sol (1 hunks)

💤 Files with no reviewable changes (1)

test/mainnet-fork/Curve.t.sol

🧰 Additional context used

🧠 Learnings (6)

📓 Common learnings

Learnt from: deluca-mike
Repo: sparkdotfi/spark-alm-controller PR: 0
File: :0-0
Timestamp: 2025-11-11T19:43:16.032Z
Learning: In spark-alm-controller, the burnPosition and decreasePosition functions in UniswapV4Lib.sol intentionally do not validate ownership because burning someone else's position would cause the underlying tokens to flow to the proxy, which benefits the system rather than harming it.

Learnt from: deluca-mike
Repo: sparkdotfi/spark-alm-controller PR: 0
File: :0-0
Timestamp: 2025-11-11T19:43:16.032Z
Learning: In spark-alm-controller, rate limits for Uniswap positions sum both tokens normalized to 18 decimals, which doesn't account for actual USD values. This is a known design decision where normalized balance is used rather than USD-denominated values.

📚 Learning: 2025-11-11T19:43:16.032Z

Learnt from: deluca-mike
Repo: sparkdotfi/spark-alm-controller PR: 0
File: :0-0
Timestamp: 2025-11-11T19:43:16.032Z
Learning: In spark-alm-controller, the burnPosition and decreasePosition functions in UniswapV4Lib.sol intentionally do not validate ownership because burning someone else's position would cause the underlying tokens to flow to the proxy, which benefits the system rather than harming it.

Applied to files:

src/interfaces/UniswapV4.sol
src/libraries/UniswapV4Lib.sol
src/MainnetController.sol
test/mainnet-fork/Uniswapv4.t.sol

📚 Learning: 2025-11-11T19:43:16.032Z

Learnt from: deluca-mike
Repo: sparkdotfi/spark-alm-controller PR: 0
File: :0-0
Timestamp: 2025-11-11T19:43:16.032Z
Learning: In spark-alm-controller, rate limits for Uniswap positions sum both tokens normalized to 18 decimals, which doesn't account for actual USD values. This is a known design decision where normalized balance is used rather than USD-denominated values.

Applied to files:

src/libraries/UniswapV4Lib.sol
src/MainnetController.sol
test/mainnet-fork/Uniswapv4.t.sol

📚 Learning: 2025-11-04T17:15:21.481Z

Learnt from: deluca-mike
Repo: sparkdotfi/spark-alm-controller PR: 185
File: test/base-fork/Aave.t.sol:59-63
Timestamp: 2025-11-04T17:15:21.481Z
Learning: In the spark-alm-controller repository, test files in test/base-fork/ use ForkTestBase from test/base-fork/ForkTestBase.t.sol where _setControllerEntered() defaults to writing the reentrancy guard state to foreignController, while test files in test/mainnet-fork/ use ForkTestBase from test/mainnet-fork/ForkTestBase.t.sol where _setControllerEntered() defaults to mainnetController.

Applied to files:

test/mainnet-fork/Uniswapv4.t.sol

📚 Learning: 2025-11-04T17:15:39.489Z

Learnt from: deluca-mike
Repo: sparkdotfi/spark-alm-controller PR: 185
File: test/base-fork/Morpho.t.sol:122-126
Timestamp: 2025-11-04T17:15:39.489Z
Learning: In the spark-alm-controller repository, tests in the `test/base-fork/` directory use `ForkTestBase` from `test/base-fork/ForkTestBase.t.sol`, where the `_setControllerEntered()` helper function defaults to setting the reentrancy guard state on `foreignController`. Tests in `test/mainnet-fork/` use their own `ForkTestBase` which defaults to `mainnetController`.

Applied to files:

test/mainnet-fork/Uniswapv4.t.sol

📚 Learning: 2025-10-16T16:47:34.627Z

Learnt from: supercontracts
Repo: sparkdotfi/spark-alm-controller PR: 170
File: src/MainnetController.sol:1153-1181
Timestamp: 2025-10-16T16:47:34.627Z
Learning: In src/MainnetController.sol, the USDS token always returns true on successful transfer operations, so it does not require the empty return data handling that some non-standard ERC20 tokens (like USDT) need.

Applied to files:

test/mainnet-fork/Uniswapv4.t.sol

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)

GitHub Check: coverage
GitHub Check: test
GitHub Check: build

🔇 Additional comments (22)

.lintstagedrc (1)

1-8: LGTM! Standard pre-commit configuration.

The lint-staged setup correctly maps file patterns to formatting and linting scripts. Solidity files will be processed by both prettier and solhint, which is the expected workflow.

.prettierignore (1)

1-5: LGTM! Appropriate exclusions for formatting.

The ignore patterns correctly exclude third-party code (node_modules and Foundry's lib directory) from Prettier formatting.

.prettierrc (1)

1-24: LGTM! Consistent formatting configuration.

The Prettier setup correctly enables Solidity support and uses sensible formatting defaults. The 120-character print width aligns with the solhint max-line-length rule in .solhint.json.

.solhint.json (3)

10-11: Verify intentional disabling of low-level call guards.

The avoid-call-value and avoid-low-level-calls rules are disabled, removing safety guardrails against potentially unsafe low-level operations. While this is likely intentional for a proxy/controller contract that requires low-level control, ensure that:

Code using .call{value:...}() includes proper reentrancy protection

Low-level calls are limited to specific, well-audited use cases

Return values are checked appropriately

72-73: Verify intentional disabling of assembly and timestamp guards.

The no-inline-assembly and not-rely-on-time rules are disabled. While assembly and timestamp usage may be necessary for optimization and protocol logic respectively, ensure:

Inline assembly is limited, well-documented, and audited

Timestamp dependencies account for miner manipulation risks (typically ±15 second variance)

1-90: Overall configuration is comprehensive.

The solhint configuration extends recommended rules and adds appropriate constraints for Solidity development. The disabled security rules align with the needs of a low-level controller contract but warrant careful review during code audits.

lib/uniswap-v4-core (1)

1-1: No action required—submodule commit is verified.

The pinned commit 59d3ecf53afa9264a16bba0e38f4c5d2231f80bc corresponds to the official Uniswap v4-core v1.0.2 release, committed by a Uniswap developer and GPG-signed. This is a stable, legitimate dependency pin with no security concerns.

src/interfaces/Common.sol (1)

1-18: Interfaces look correct and match intended external contracts

IERC20Like and IPermit2Like are minimal but sufficient for your use cases, and IPermit2Like.approve matches the current Permit2 signature (no return value). I don’t see issues with these definitions.

src/RateLimitHelpers.sol (1)

6-20: Rate‑limit key helpers are simple and sound

All helpers consistently derive keys as keccak256(abi.encode(key, …)), keeping domain separation via the leading key and avoiding storage layout surprises. No issues from a correctness or security perspective.

src/libraries/UniswapV4Lib.sol (1)

251-303: Rate‑limit decrease computation assumes net outflow on increases and 1:1 normalized value

_increaseLiquidity:

Uses clamped subtraction on normalized balances before/after to derive rateLimitDecrease, which safely handles the (unlikely) case of net token inflow when adding liquidity.

Immediately calls triggerRateLimitDecrease with makeBytes32Key(commonParams.rateLimitId, commonParams.poolId) and then fully revokes approvals via _approvePositionManager(..., 0).

This is correct under the documented assumption that the tokens are treated as economically equal once normalized to 18 decimals (e.g., USDC/USDT), and matches the repository’s design of using normalized (not USD) values for rate limits.

Based on learnings

test/mainnet-fork/Uniswapv4.t.sol (12)

121-128: Rate‑limit configuration in setUp matches library’s rate‑limit keying

setUp configures deposit/withdraw limits using _DEPOSIT_LIMIT_KEY and _WITHDRAW_LIMIT_KEY, which are keccak256(abi.encode(limitId, _POOL_ID)). This aligns with the library’s use of RateLimitHelpers.makeBytes32Key(rateLimitId, poolId) and the expectation that both directions share a bytes32 key derived from (rateLimitId, poolId).

The per‑test assertions on rateLimits.getCurrentRateLimit(...) effectively verify that rate‑limit deltas match the normalized balance changes computed inside UniswapV4Lib.

Based on learnings

134-207: mintPosition tests provide good coverage of guards and allowance‑based slippage

The mintPositionUniswapV4_* tests collectively check:

Reentrancy guard via _setControllerEntered() and ReentrancyGuard revert.

Role gating for non‑relayers.

Tick range sanity (lower bound, upper bound, and max range).

“Slippage” via allowance caps: tests deliberately expect generic vm.expectRevert() when actual spend exceeds amount0Max/amount1Max.

This matches the library/controller design where per‑operation slippage is enforced via Permit2 allowances rather than explicit on‑chain ratios. No issues spotted here.

262-281: End‑to‑end mint test correctly asserts ownership, liquidity, allowances, and rate‑limit deltas

test_mintPositionUniswapV4 is a strong integration test:

Confirms the Position Manager owner is almProxy and that getPositionLiquidity equals result.liquidityIncrease.

Asserts both token→Permit2 and Permit2→PositionManager allowances are zero via _assertZeroAllowances, validating the approval reset logic in _approvePositionManager.

Compares the rate‑limit decrease against normalized 18‑dec amounts (_to18From6Decimals) based on actual spend, matching the library’s normalization behavior.

This is an effective regression guard for future changes in approval handling or rate‑limit math.

Based on learnings

287-404: increaseLiquidity tests mirror mint tests and validate ownership + poolId guards

The increaseLiquidityUniswapV4_* tests cover:

Access control for non‑relayers.

Enforcement of "MC/non-proxy-position" when the NFT is transferred away from the proxy.

Enforcement of "MC/tokenId-poolId-mismatch" when the provided poolId doesn’t match the position’s pool.

Allowance‑based bounds on amount0Max/amount1Max, and correct rate‑limit deltas on successful increases, including allowance reset checks.

Together with mint tests, this gives good confidence that the increase flow is safe, correctly keyed to the intended pool, and interacts properly with rate‑limits.

410-511: burnPosition tests validate poolId checks, min‑amount slippage, and withdraw rate‑limits

The burnPositionUniswapV4_* tests:

Enforce relayer role.

Exercise "MC/tokenId-poolId-mismatch" for wrong poolId.

Assert Uniswap’s SlippageCheck.MinimumAmountInsufficient error when amount0Min/amount1Min exceed forecasted outputs.

In the happy path, assert that position liquidity goes to zero and that the withdraw rate‑limit delta matches normalized amounts actually received.

Given the intentional design choice to not enforce ownerOf in burnPosition, these tests correctly focus on poolId matching, slippage, and rate‑limit behavior rather than ownership.

Based on learnings

517-624: decreaseLiquidity tests nicely complement burn tests and verify partial liquidity removal

For decreaseLiquidityUniswapV4_*:

Access control and poolId mismatch cases mirror the burn tests.

Slippage checks for both tokens use half the minted liquidity and assert the correct MinimumAmountInsufficient behavior.

The success test verifies:

NFT remains owned by almProxy.

Liquidity decreases by the requested fraction (half in this case).

Withdraw rate‑limit delta matches normalized amounts received.

This provides good coverage of partial unwind behavior and confirms that rate‑limit accounting for decreases is symmetric with burns.

630-667: Tick‑limit tests ensure admin gating and correct configuration event semantics

The setUniswapV4tickLimits_* tests:

Confirm reentrancy protection.

Enforce that only the admin (DEFAULT_ADMIN_ROLE) can set tick limits.

Validate "MC/invalid-ticks" behavior for bad ranges.

Assert the UniswapV4TickLimitsSet event contents and that the stored limits match expectations.

This is important because these per‑pool tick limits are a core defense against pathological LP ranges and front‑running scenarios in the attack tests.

673-729: Story test is a solid end‑to‑end scenario tying together mint, increase, decrease, burn, swaps, and rate‑limits

test_uniswapV4_story1:

Configures realistic tick limits.

Exercises mint → increase → decrease → burn across time, with intervening swaps that move the pool price.

Checks that both deposit and withdraw rate‑limits track the total normalized tokens moved over the lifecycle.

This serves as a high‑level “does everything still work together?” test and complements the unit‑style tests nicely.

Based on learnings

761-1158: Attack tests give excellent coverage of front‑run/back‑run scenarios across price regimes

The suite of test_uniswapV4_attack_* functions:

Systematically explores baseline vs. frontrun/backrun cases when the true price is mid, above, or below the expected tick range.

Measures PnL for the attacker and LP using concrete amounts, showing how much value can be extracted when the relayer assumes stale prices.

Includes a defended variant (*_defended) demonstrating how tighter amount{0,1}Max settings cause the manipulated mint to revert due to insufficient allowance.

These tests are very valuable as regression checks against subtle changes in controller logic or Uniswap V4 behavior. The hard‑coded expected amounts do make them somewhat brittle to changes in pool state or fork block, but given _getBlock() is overridden to a fixed historical block that’s an acceptable trade‑off for precision here.

1208-1359: Helper flows _mintPosition / _increasePosition / _burnPosition are consistent with PositionManager semantics

The helpers:

Use nextTokenId() before minting and assume the minted tokenId equals that value, which matches the current PositionManager implementation (tokenId = nextTokenId++).

Compute amounts spent/received as simple balance differences, which works given the direction of funds in mint/increase/burn operations.

Reuse Position Manager’s getPoolAndPositionInfo to derive ticks, avoiding re‑encoding pool keys in tests.

No correctness issues seen. If PositionManager semantics change in the future, the tokenId assumption is the main coupling point to revisit.

1405-1455: Liquidity → amount helpers match standard Uniswap formulas and include input validation

_getAmount0ForLiquidity, _getAmount1ForLiquidity, and _getAmountsForLiquidity:

Implement the usual Q64.96 formulas with FullMath.mulDiv, matching Uniswap’s LiquidityAmounts logic.

Enforce sqrtPriceAX96 < sqrtPriceBX96 with explicit requires, which helps catch misconfigured ranges early in tests.

Correctly handle the three regions (price below, within, above range) in _getAmountsForLiquidity.

These helpers provide a trustworthy “oracle” for the forecasted amounts used in slippage and limit calculations in tests.

1586-1633: Swap helper correctly wires Permit2 + Universal Router for test purposes

_swap:

Treats the pool as symmetric USDC/USDT and chooses zeroForOne based on tokenIn.

Uses deal to fund the account, sets a Permit2 allowance, and executes a single V4 swap via the Universal Router with SWAP_EXACT_IN_SINGLE + SETTLE_ALL + TAKE_ALL.

Computes amountOut by simple balance delta.

This looks correct for exercising price movement and attack scenarios in a forked test context. Since this uses live mainnet contracts via forking, any future changes to router or pool behavior would surface as test failures, which is desirable.

src/libraries/UniswapV4Lib.sol

test/mainnet-fork/Uniswapv4.t.sol

coderabbitai

Actionable comments posted: 4

♻️ Duplicate comments (3)

src/MainnetController.sol (1)

675-739: Reentrancy and access control on Uniswap V4 relayer functions now match threat model

All four Uniswap V4 relayer entrypoints (increaseLiquidityUniswapV4, burnPositionUniswapV4, decreaseLiquidityUniswapV4, plus the existing mintPositionUniswapV4) are now external nonReentrant and RELAYER‑gated, closing the hook‑driven reentrancy path flagged in an earlier review. Delegation to UniswapV4Lib centralises poolId/ownership checks and rate-limit accounting, which is consistent with the rest of the controller design.

Given the intentional choice that burnPosition/decreasePosition in the library only enforce poolId (not ownership) so that any approved burn/decrease routes underlying tokens to the proxy, no additional checks are needed here; just ensure this behavior remains clearly documented in the library to prevent future “fixes” that would break the economic model. Based on learnings

src/libraries/UniswapV4Lib.sol (2)

29-101: Position entrypoints and poolId/ownership checks match the intended security model

The four external entrypoints cleanly separate responsibilities:

mintPosition fetches the PoolKey from the Position Manager, builds MINT_POSITION + SETTLE_PAIR calldata, and delegates to _increaseLiquidity, which handles approvals and rate-limit accounting.

increasePosition enforces both ownerOf(tokenId) == proxy and _requirePoolIdMatch(poolId, tokenId), ensuring added liquidity remains owned by the proxy and the tokenId belongs to the expected pool.

burnPosition/decreasePosition only require a poolId match and rely on Position Manager approvals, intentionally allowing any approved burn/decrease for that pool to route tokens to proxy even if the proxy is not the NFT owner. This is consistent with the design decision that non‑owner burns are economically beneficial to the system.

From an attack‑surface perspective, this means a misconfigured approval could allow a relayer to burn someone else’s NFT position, but the proceeds still land on the ALM proxy, not the attacker. Given that trade‑off, it would be helpful to keep a short comment here (or cross‑reference to higher‑level docs) explaining why burn/decrease omit ownership checks while increase enforces them, to prevent future regressions. Based on learnings

Also applies to: 103-167

173-209: Permit2 approval pattern and rate‑limit accounting are careful; consider defensive clamp on decreases

The _approvePositionManager + _increaseLiquidity flow is well thought‑out: best‑effort zeroing of token→Permit2, strict approve with return‑data check, a per‑call Permit2 approval to the Position Manager with expiration = block.timestamp, then post‑call cleanup by resetting approvals. Combined with pre/post balance sampling and normalized clamped subtraction, deposit rate‑limit consumption is tightly bounded per call.

_decreaseLiquidity mirrors the balance‑differential pattern for withdrawals, but uses raw endingBalance - startingBalance when computing normalized deltas. This is correct for the expected case (net inflow on decrease/burn) and will revert if a future hook ever induces net outflow during a “decrease” call. If you prefer to treat such a path as “no rate‑limit credit” instead of a hard revert, you could reuse _clampedSub on normalized sums here as previously suggested; not critical, but slightly more future‑proof.

Also applies to: 211-258, 260-292

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6c26dfc and 65e9092.

📒 Files selected for processing (5)

lib/uniswap-v4-core (1 hunks)
lib/uniswap-v4-periphery (1 hunks)
src/MainnetController.sol (8 hunks)
src/libraries/UniswapV4Lib.sol (1 hunks)
test/mainnet-fork/Uniswapv4.t.sol (1 hunks)

🧰 Additional context used

🧠 Learnings (6)

📓 Common learnings

Learnt from: deluca-mike
Repo: sparkdotfi/spark-alm-controller PR: 0
File: :0-0
Timestamp: 2025-11-11T19:43:16.032Z
Learning: In spark-alm-controller, the burnPosition and decreasePosition functions in UniswapV4Lib.sol intentionally do not validate ownership because burning someone else's position would cause the underlying tokens to flow to the proxy, which benefits the system rather than harming it.

Learnt from: deluca-mike
Repo: sparkdotfi/spark-alm-controller PR: 0
File: :0-0
Timestamp: 2025-11-11T19:43:16.032Z
Learning: In spark-alm-controller, rate limits for Uniswap positions sum both tokens normalized to 18 decimals, which doesn't account for actual USD values. This is a known design decision where normalized balance is used rather than USD-denominated values.

📚 Learning: 2025-11-11T19:43:16.032Z

Learnt from: deluca-mike
Repo: sparkdotfi/spark-alm-controller PR: 0
File: :0-0
Timestamp: 2025-11-11T19:43:16.032Z
Learning: In spark-alm-controller, the burnPosition and decreasePosition functions in UniswapV4Lib.sol intentionally do not validate ownership because burning someone else's position would cause the underlying tokens to flow to the proxy, which benefits the system rather than harming it.

Applied to files:

src/MainnetController.sol
src/libraries/UniswapV4Lib.sol
test/mainnet-fork/Uniswapv4.t.sol

📚 Learning: 2025-11-11T19:43:16.032Z

Learnt from: deluca-mike
Repo: sparkdotfi/spark-alm-controller PR: 0
File: :0-0
Timestamp: 2025-11-11T19:43:16.032Z
Learning: In spark-alm-controller, rate limits for Uniswap positions sum both tokens normalized to 18 decimals, which doesn't account for actual USD values. This is a known design decision where normalized balance is used rather than USD-denominated values.

Applied to files:

src/MainnetController.sol
src/libraries/UniswapV4Lib.sol
test/mainnet-fork/Uniswapv4.t.sol

📚 Learning: 2025-11-04T17:15:21.481Z

Learnt from: deluca-mike
Repo: sparkdotfi/spark-alm-controller PR: 185
File: test/base-fork/Aave.t.sol:59-63
Timestamp: 2025-11-04T17:15:21.481Z
Learning: In the spark-alm-controller repository, test files in test/base-fork/ use ForkTestBase from test/base-fork/ForkTestBase.t.sol where _setControllerEntered() defaults to writing the reentrancy guard state to foreignController, while test files in test/mainnet-fork/ use ForkTestBase from test/mainnet-fork/ForkTestBase.t.sol where _setControllerEntered() defaults to mainnetController.

Applied to files:

test/mainnet-fork/Uniswapv4.t.sol

📚 Learning: 2025-11-04T17:15:39.489Z

Learnt from: deluca-mike
Repo: sparkdotfi/spark-alm-controller PR: 185
File: test/base-fork/Morpho.t.sol:122-126
Timestamp: 2025-11-04T17:15:39.489Z
Learning: In the spark-alm-controller repository, tests in the `test/base-fork/` directory use `ForkTestBase` from `test/base-fork/ForkTestBase.t.sol`, where the `_setControllerEntered()` helper function defaults to setting the reentrancy guard state on `foreignController`. Tests in `test/mainnet-fork/` use their own `ForkTestBase` which defaults to `mainnetController`.

Applied to files:

test/mainnet-fork/Uniswapv4.t.sol

📚 Learning: 2025-10-16T16:47:34.627Z

Learnt from: supercontracts
Repo: sparkdotfi/spark-alm-controller PR: 170
File: src/MainnetController.sol:1153-1181
Timestamp: 2025-10-16T16:47:34.627Z
Learning: In src/MainnetController.sol, the USDS token always returns true on successful transfer operations, so it does not require the empty return data handling that some non-standard ERC20 tokens (like USDT) need.

Applied to files:

test/mainnet-fork/Uniswapv4.t.sol

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)

GitHub Check: coverage
GitHub Check: build
GitHub Check: test

🔇 Additional comments (6)

lib/uniswap-v4-core (1)

1-1: Submodule reference verified as secure.

Uniswap V4 Core was released as an audited, stable protocol on January 31, 2025, after nine independent audits and a large security competition. The submodule URL points to the official Uniswap repository, and the commit is pinned to a specific version, which is best practice for reproducibility and dependency management. No concerns identified.

src/libraries/UniswapV4Lib.sol (1)

18-24: Limit IDs and hard‑coded Uniswap mainnet addresses are consistent but mainnet‑specific

LIMIT_DEPOSIT/LIMIT_WITHDRAW constants and their use as prefixes for makeBytes32Key align with how MainnetController and tests derive per‑pool rate‑limit keys, so the wiring is coherent end‑to‑end.

The _PERMIT2 and _POSITION_MANAGER addresses match Uniswap’s documented Ethereum mainnet deployments, but they hard‑code this library to mainnet. That’s appropriate for MainnetController, but if you ever reuse UniswapV4Lib on other networks, you’ll need a different deployment or a configurable address pattern.

test/mainnet-fork/Uniswapv4.t.sol (3)

102-127: Rate-limit keys in tests correctly mirror library/controller wiring

The test constants _LIMIT_DEPOSIT/_LIMIT_WITHDRAW and derived keys _DEPOSIT_LIMIT_KEY/_WITHDRAW_LIMIT_KEY use keccak256(abi.encode(limitId, _POOL_ID)), matching how UniswapV4Lib constructs keys via RateLimitHelpers.makeBytes32Key. setUp seeds both limits on the shared rateLimits contract, so subsequent assertions on getCurrentRateLimit are exercising the same paths as production.

This alignment between tests and on-chain key derivation is important for catching any future drift in how per‑pool limits are encoded.

134-425: Good coverage of Uniswap V4 relayer guards, tick limits, slippage, and rate-limit deltas

The mintPositionUniswapV4_*, increaseLiquidityUniswapV4_*, burnPositionUniswapV4_*, decreaseLiquidityUniswapV4_*, and setUniswapV4tickLimits_* tests together cover:

Reentrancy protection via _setControllerEntered() and ReentrancyGuardReentrantCall expectations on every V4 entrypoint and the admin setter.

Access control for RELAYER and DEFAULT_ADMIN_ROLE callers.

Tick‑range validation and the UniswapV4TickLimitsSet event plus mapping reads.

PoolId correctness via "MC/tokenId-poolId-mismatch" for mismatched NFT/pool combinations.

Slippage enforcement on min/max amounts using SlippageCheck.MinimumAmountInsufficient and generic “insufficient allowance” reverts when actual usage exceeds amount{0,1}Max.

Rate-limit consumption symmetry by asserting that initialLimit - currentLimit equals the 18‑dec normalized sum of token deltas.

This suite gives strong confidence that the controller’s Uniswap V4 surface behaves correctly under both happy paths and common misconfigurations.

Also applies to: 430-675, 681-722

1263-1390: Helper math and router/Permit2 scaffolding correctly support the tests

The helpers for quoting liquidity (_getAmount0ForLiquidity, _getAmount1ForLiquidity, _getAmountsForLiquidity, _quoteLiquidity) closely follow the standard Uniswap v3/v4 formulas using FullMath and TickMath, and the 6‑→18 decimal conversion via _to18From6Decimals matches the system‑wide normalization assumption.

_swap’s use of Universal Router + Permit2 (per‑call approvals with expiration = block.timestamp) plus balance‑delta checks provides a realistic price‑movement mechanism for the tests, and _getBlock’s fixed return ensures deterministic forking behavior. Together these helpers make the higher‑level tests precise without introducing additional stateful complexity into the controller.

Also applies to: 1392-1688

src/MainnetController.sol (1)

955-957: Interface ordering is correct; no changes needed.

The quoteOFT function in ILayerZero (src/interfaces/ILayerZero.sol:56) returns (OFTLimit memory, OFTFeeDetail[] memory oftFeeDetails, OFTReceipt memory). Your destructuring ( , , OFTReceipt memory receipt ) correctly captures OFTReceipt as the third return value, and using receipt.amountReceivedLD to set sendParams.minAmountLD is the intended behavior.

lib/uniswap-v4-periphery

src/MainnetController.sol

test/mainnet-fork/Uniswapv4.t.sol

coderabbitai

Actionable comments posted: 2

♻️ Duplicate comments (9)

src/libraries/UniswapV4Lib.sol (5)

183-219: Permit2 approval pattern and rate-limit accounting on deposit side look correct and well-contained

_approvePositionManager first best‑effort zeros the token→Permit2 allowance via a low‑level proxy.call, then mandates success for the non‑zero approve, then grants Permit2→PositionManager allowance with expiration = uint48(block.timestamp) so approvals are transient to the current block/tx. _increaseLiquidity snapshots balances before/after, uses _clampedSub on normalized balances to compute the actual deposit (defensively handling any net inflow), triggers the per‑pool LIMIT_DEPOSIT rate‑limit, and then calls _approvePositionManager(proxy, token, 0) to fully revoke both allowances. The flow is tightly scoped and matches the tests’ expectations around zeroed approvals.

Also applies to: 221-268

18-26: Interactive Uniswap V4 flows are structurally sound; clarify burn/decrease ownership model

mintPosition enforces per‑pool TickLimits, derives PoolKey from poolId, and routes through _increaseLiquidity, while increasePosition adds an explicit ownerOf(tokenId) == proxy check plus a poolId match. In contrast, burnPosition/decreasePosition only call _requirePoolIdMatch and then _decreaseLiquidity, intentionally allowing any approved position in the same pool to be burned/decreased so underlying tokens flow to proxy. That asymmetry is by design and economically favorable to the system, but it’s non‑obvious; adding a brief comment near burnPosition/decreasePosition explaining that ownership is intentionally not enforced (poolId + approvals only) would help future reviewers avoid “fixing” it incorrectly.

Based on learnings

Also applies to: 35-177

270-302: Withdraw-side rate-limit accounting assumes net inflow; optional to mirror _clampedSub for extra safety

_decreaseLiquidity computes rateLimitDecrease from endingBalance{0,1} - startingBalance{0,1} and normalizes the deltas. Given this path only ever pulls tokens from the pool into the proxy (no approvals granted to move tokens out), ending >= starting should always hold, so the raw subtraction is safe with Solidity 0.8 checked arithmetic and matches the tests’ expectations exactly. If you want future‑proofing against unforeseen hook behavior, you could mirror the _increaseLiquidity pattern by using _clampedSub on normalized sums instead, turning any pathological net outflow into “no rate-limit credit” rather than a revert.

467-472: Normalized-balance helper assumes tokens with decimals() <= 18 and standard ERC20 behavior

_getNormalizedBalance scales via balance * 1e18 / 10 ** IERC20Like(token).decimals(), which works as intended for the 6‑decimal stables you’re targeting but will undercount tokens with decimals() > 18 and rely on decimals() being implemented and non‑reverting. That’s acceptable given the current scope, but consider either documenting that this library is only meant for tokens with decimals() <= 18 and standard ERC20 semantics, or adding a defensive check if you expect to broaden token support later.

Based on learnings

474-482: PoolId handling correctly mirrors V4 PositionManager internals but is tightly coupled

_getPoolKey reads poolKeys(bytes25(poolId)), and _requirePoolIdMatch compares keccak256(abi.encode(poolKey)) to the external poolId, which matches Uniswap V4’s current PoolId encoding and how the tests derive/verify poolIds. This is intentionally coupled to the deployed PositionManager’s implementation; a short comment noting that poolId is expected to be keccak256(abi.encode(poolKey)) and that the bytes25 cast mirrors the V4 mapping key would make that dependency explicit for future maintainers.

test/mainnet-fork/Uniswapv4.t.sol (3)

747-803: Story and attack scenario tests provide valuable adversarial coverage of Uniswap V4 economic behavior

The story test and test_uniswapV4_attack_* suite run through realistic sequences of mints, swaps via the V4 router, increases, decreases, and burns under different price path assumptions (mid/above/below, front‑run/back‑run combinations), then assert on concrete USDC/USDT flows and rate-limit deltas. They demonstrate that rate-limit accounting tracks actual normalized token movements and quantify MEV-style PnL impact under adverse order flow, which is exactly the kind of adversarial coverage this integration needs.

Also applies to: 835-1232

809-833: Diagnostic price-logging tests and helpers are verbose but safely isolated

The two test_uniswapV4_logPriceAndTicks_* tests are vm.skip(true) and only used for manual diagnostics, and _logCurrentPriceAndTick performs detailed fixed-point formatting of the 18‑dec price and current tick. This is intentionally verbose but confined to skipped tests, so it doesn’t affect CI performance; if you ever unskip these, you might want to reduce iterations/logging volume, but no change is needed now.

Also applies to: 1547-1631

1648-1654: Allowance assertion messages are slightly token-specific but otherwise fine

_assertZeroAllowances correctly verifies both Permit2’s allowance and the token’s allowance to Permit2 are zero, but the error strings mention “usdc” even when token could be something else. This is purely cosmetic; if you touch this helper again, consider swapping the messages for generic ones (e.g. "permit2 allowance non-zero" / "token->permit2 allowance non-zero") to avoid confusion when debugging failures for other tokens.

src/MainnetController.sol (1)

133-133: Tick-limit setter correctly constrains ranges; consider documenting “(0,0,0) disables minting”

setUniswapV4TickLimits enforces tickLowerMin <= tickUpperMax, writes all three fields, and emits UniswapV4TickLimitsSet, which matches how _checkTickLimits is used in UniswapV4Lib. Combined with _checkTickLimits’s tickLimits-not-set guard, (0,0,0) effectively disables new mints while existing positions remain usable. It would be helpful to mention this “zero tuple = pool disabled for minting” semantics in NatSpec/comments so operators don’t misinterpret (0,0,0) as a valid narrow range.

Also applies to: 317-336

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 65e9092 and 0a5bb66.

📒 Files selected for processing (3)

src/MainnetController.sol (7 hunks)
src/libraries/UniswapV4Lib.sol (1 hunks)
test/mainnet-fork/Uniswapv4.t.sol (1 hunks)

🧰 Additional context used

🧠 Learnings (6)

📓 Common learnings

Learnt from: deluca-mike
Repo: sparkdotfi/spark-alm-controller PR: 0
File: :0-0
Timestamp: 2025-11-11T19:43:16.032Z
Learning: In spark-alm-controller, the burnPosition and decreasePosition functions in UniswapV4Lib.sol intentionally do not validate ownership because burning someone else's position would cause the underlying tokens to flow to the proxy, which benefits the system rather than harming it.

Learnt from: deluca-mike
Repo: sparkdotfi/spark-alm-controller PR: 0
File: :0-0
Timestamp: 2025-11-11T19:43:16.032Z
Learning: In spark-alm-controller, rate limits for Uniswap positions sum both tokens normalized to 18 decimals, which doesn't account for actual USD values. This is a known design decision where normalized balance is used rather than USD-denominated values.

📚 Learning: 2025-11-11T19:43:16.032Z

Learnt from: deluca-mike
Repo: sparkdotfi/spark-alm-controller PR: 0
File: :0-0
Timestamp: 2025-11-11T19:43:16.032Z
Learning: In spark-alm-controller, the burnPosition and decreasePosition functions in UniswapV4Lib.sol intentionally do not validate ownership because burning someone else's position would cause the underlying tokens to flow to the proxy, which benefits the system rather than harming it.

Applied to files:

src/MainnetController.sol
src/libraries/UniswapV4Lib.sol
test/mainnet-fork/Uniswapv4.t.sol

📚 Learning: 2025-11-11T19:43:16.032Z

Learnt from: deluca-mike
Repo: sparkdotfi/spark-alm-controller PR: 0
File: :0-0
Timestamp: 2025-11-11T19:43:16.032Z
Learning: In spark-alm-controller, rate limits for Uniswap positions sum both tokens normalized to 18 decimals, which doesn't account for actual USD values. This is a known design decision where normalized balance is used rather than USD-denominated values.

Applied to files:

src/MainnetController.sol
src/libraries/UniswapV4Lib.sol
test/mainnet-fork/Uniswapv4.t.sol

📚 Learning: 2025-11-04T17:15:21.481Z

Learnt from: deluca-mike
Repo: sparkdotfi/spark-alm-controller PR: 185
File: test/base-fork/Aave.t.sol:59-63
Timestamp: 2025-11-04T17:15:21.481Z
Learning: In the spark-alm-controller repository, test files in test/base-fork/ use ForkTestBase from test/base-fork/ForkTestBase.t.sol where _setControllerEntered() defaults to writing the reentrancy guard state to foreignController, while test files in test/mainnet-fork/ use ForkTestBase from test/mainnet-fork/ForkTestBase.t.sol where _setControllerEntered() defaults to mainnetController.

Applied to files:

test/mainnet-fork/Uniswapv4.t.sol

📚 Learning: 2025-11-04T17:15:39.489Z

Learnt from: deluca-mike
Repo: sparkdotfi/spark-alm-controller PR: 185
File: test/base-fork/Morpho.t.sol:122-126
Timestamp: 2025-11-04T17:15:39.489Z
Learning: In the spark-alm-controller repository, tests in the `test/base-fork/` directory use `ForkTestBase` from `test/base-fork/ForkTestBase.t.sol`, where the `_setControllerEntered()` helper function defaults to setting the reentrancy guard state on `foreignController`. Tests in `test/mainnet-fork/` use their own `ForkTestBase` which defaults to `mainnetController`.

Applied to files:

test/mainnet-fork/Uniswapv4.t.sol

📚 Learning: 2025-10-16T16:47:34.627Z

Learnt from: supercontracts
Repo: sparkdotfi/spark-alm-controller PR: 170
File: src/MainnetController.sol:1153-1181
Timestamp: 2025-10-16T16:47:34.627Z
Learning: In src/MainnetController.sol, the USDS token always returns true on successful transfer operations, so it does not require the empty return data handling that some non-standard ERC20 tokens (like USDT) need.

Applied to files:

test/mainnet-fork/Uniswapv4.t.sol

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)

GitHub Check: coverage
GitHub Check: build
GitHub Check: test

🔇 Additional comments (12)

src/MainnetController.sol (4)

27-27: Uniswap V4 wiring (import, limit IDs, tick-limit mapping) looks consistent

Importing UniswapV4Lib, exposing LIMIT_UNISWAP_V4_DEPOSIT/WITHDRAW from the library, and storing per‑pool TickLimits in uniswapV4TickLimits all line up with how UniswapV4Lib expects to be called (bytes32 poolId keys, makeBytes32Key(LIMIT_*, poolId) in the library, and tests setting rate limits with the same scheme). I don’t see type or key‑mismatch risks here.

Also applies to: 158-159, 201-203

629-656: mintPositionUniswapV4 entrypoint is well-gated and delegates safety checks to UniswapV4Lib

This relayer function is external nonReentrant, RELAYER‑gated, and passes the controller’s proxy, rate‑limits address, poolId, tick range, liquidity, max amounts, and the entire uniswapV4TickLimits mapping into UniswapV4Lib.mintPosition. That cleanly centralizes tick validation, token accounting, and rate‑limit updates inside the library while keeping the controller surface minimal. No additional checks appear missing here.

658-720: Increase/burn/decrease Uniswap V4 entrypoints maintain consistent access control and reentrancy defenses

All three functions (increaseLiquidityUniswapV4, burnPositionUniswapV4, decreaseLiquidityUniswapV4) are external nonReentrant, RELAYER‑gated, and simply forward to the corresponding UniswapV4Lib calls with proxy, rateLimits, poolId, tokenId, amounts, and min amounts. Ownership checks and poolId guards are handled in the library (increasePosition enforces ownerOf == proxy, while burn/decrease intentionally rely only on poolId match and approvals, per the established design). From the controller side, the wiring and protections look correct.

Based on learnings

937-938: LayerZero quoteOFT usage now matches OFTReceipt semantics

Destructuring quoteOFT as ( , , OFTReceipt memory receipt ) and setting sendParams.minAmountLD = receipt.amountReceivedLD correctly uses the quoted minimum amount deliverable on the destination chain, instead of hard‑coding zero or reading the wrong tuple element. This closes a slippage hole in the bridge call while keeping the rest of the flow unchanged.

src/libraries/UniswapV4Lib.sol (1)

308-321: Tick-limit checks correctly gate new mints and treat zeroed limits as “not set”

_checkTickLimits requires at least one of tickLowerMin, tickUpperMax, or maxTickWidth to be non‑zero (so (0,0,0) is “not configured”), enforces tickLower >= tickLowerMin, tickUpper <= tickUpperMax, and bounds the width via uint256(int256(tickUpper) - int256(tickLower)) <= maxTickWidth. That gives admins a clean way to disable new mints for a pool by setting (0,0,0) and relies on the width check to reject any inverted tick ranges. Logic looks correct.

test/mainnet-fork/Uniswapv4.t.sol (7)

20-27: Test harness mirrors production wiring and rate-limit keys correctly

The test contract pulls in UniswapV4Lib and MainnetController, hard-codes the same Permit2/PositionManager/Router/StateView addresses and poolId as the library, and defines _DEPOSIT_LIMIT_KEY/_WITHDRAW_LIMIT_KEY as keccak256(abi.encode(_LIMIT_*, _POOL_ID)), which matches RateLimitHelpers.makeBytes32Key usage. setUp seeds generous rate limits for that key pair. This gives good confidence that tests are exercising the same wiring and keys used in production.

Also applies to: 102-117, 121-128

130-305: mintPositionUniswapV4 tests validate reentrancy, role gating, tick limits, slippage, and rate-limit accounting

This block of tests systematically checks: reentrancy guard behavior, RELAYER-only access, unset tick limits, each tick-bound failure mode, amount0/1 max overshoot via Uniswap V4 slippage reverts, and a successful mint that asserts zeroed Permit2/token allowances plus exact LIMIT_DEPOSIT consumption based on normalized token deltas. That gives strong coverage of the mint entrypoint’s safety and accounting.

306-443: increaseLiquidityUniswapV4 tests comprehensively cover ownership, poolId, slippage, and rate-limits

These tests verify reentrancy, RELAYER gating, the MC/non-proxy-position ownership check, MC/tokenId-poolId-mismatch, amount0/1Max overshoots, and a happy path where liquidity, ownership, allowances, and deposit rate-limit deltas all match expectations. They tightly couple to the library’s increasePosition behavior and confirm the asymmetric ownership model (increase requires proxy ownership) is enforced.

445-565: burnPositionUniswapV4 tests confirm poolId guarding, SlippageCheck behavior, and withdraw rate-limits

The burn tests assert reentrancy and RELAYER-only access, require the tokenId to match the supplied poolId, and use SlippageCheck.MinimumAmountInsufficient expectations to validate the underlying Uniswap V4 slippage guard. The happy-path test then checks position liquidity is fully burned and that the LIMIT_WITHDRAW rate-limit is decremented by the normalized sum of tokens actually received, matching _decreaseLiquidity’s accounting.

567-694: decreaseLiquidityUniswapV4 tests mirror burn coverage for partial withdrawals

These tests mirror the burn suite for partial decreases: reentrancy and RELAYER-only checks, poolId mismatch, per-token min slippage failures, and a successful decrease that verifies ownership retained by the proxy, updated liquidity amount, and precise withdraw rate-limit consumption based on received amounts. This gives good confidence in partial withdraw behavior and its interaction with rate limits.

700-741: Admin tick-limit tests validate reentrancy, role, input validation, and event + storage wiring

The setUniswapV4TickLimits tests ensure the function is nonReentrant, restricted to the admin role, rejects tickLowerMin > tickUpperMax, emits the expected UniswapV4TickLimitsSet event with configured values, and that uniswapV4TickLimits storage returns exactly those values. The _assertReentrancyGuardWrittenToTwice assertion also confirms the setter integrates with the same guard as other admin calls.

1282-1477: Helper functions give clear observability into token flows and align with library semantics

The _mintPosition, _increasePosition, _burnPosition, _decreasePosition, and _swap helpers all compute before/after token balances around the controller calls, enforce proper prank contexts (relayer vs SPARK_PROXY vs external accounts), and return structured results used in assertions. _swap uses the same PoolKey lookup and Permit2 pattern that the library relies on. This structure makes it easy to reason about exact token deltas and ticks in tests, and it matches the production integration patterns.

Also applies to: 1664-1707

src/libraries/UniswapV4Lib.sol

src/MainnetController.sol

coderabbitai

Actionable comments posted: 1

♻️ Duplicate comments (7)

test/mainnet-fork/Uniswapv4.t.sol (3)
763-819: Story and attack scenarios provide valuable adversarial coverage

The test_uniswapV4_story1 and the various test_uniswapV4_attack_* cases:

Walk realistic sequences of mints/increases/decreases/burns interleaved with large swaps via the V4 router.

Quantify LP PnL under different front‑run/back‑run paths (mid→above/below, below/above→mid, etc.).

Assert on rate‑limit deltas in normalized 18‑decimals, matching the library’s design decision.

No issues spotted; these are a strong complement to unit‑style tests and double as documentation of economic behavior under adversarial order flow.

Also applies to: 851-1134, 115-1325

825-849: Verbose price/tick logging helpers are fine as skipped diagnostics

test_uniswapV4_logPriceAndTicks_* and _logCurrentPriceAndTick are intentionally diagnostic:

Tests are vm.skip(true) so they don’t impact CI.

_logCurrentPriceAndTick’s formatting ladder is long but logically consistent with _getPrice.

No changes needed; just be aware that un‑skipping these tests would be slow and noisy in CI.

If you ever re-enable these, consider replacing the if ladder with a single formatting helper and limiting iterations for CI‑friendliness.

Also applies to: 1683-1767

1784-1790: Assertion messages in _assertZeroAllowances are token‑specific but helper is generic

The helper accepts an arbitrary token address, but the assertion messages are hard‑coded as "permit2 usdc allowance" and "token usdc allowance", which can be misleading when called with non‑USDC tokens.

If you touch this helper again, consider making the messages token‑agnostic (e.g. "permit2 allowance non-zero" / "token->permit2 allowance non-zero").
-        assertEq(allowance, 0, "permit2 usdc allowance");
-
-        assertEq(IERC20Like(token).allowance(address(almProxy), _PERMIT2), 0, "token usdc allowance");
+        assertEq(allowance, 0, "permit2 allowance non-zero");
+
+        assertEq(
+            IERC20Like(token).allowance(address(almProxy), _PERMIT2),
+            0,
+            "token->permit2 allowance non-zero"
+        );
src/libraries/UniswapV4Lib.sol (4)
183-219: Permit2 approval flow is subtle but appears correct; consider documenting expiration intent

_approvePositionManager:

Best‑effort zeroes the token→Permit2 allowance via a low‑level proxy.call(IALMProxy.doCall(...approve(...,0))), ignoring failure.

For amount != 0, enforces ERC‑20 approve success by checking the return data is empty or decodes to true.

Unconditionally grants Permit2’s allowance to _POSITION_MANAGER with expiration = uint48(block.timestamp), making the Permit2 approval transient to the current block/tx.

Callers later clear approvals again by calling _approvePositionManager(proxy, token, 0).

The pattern looks sound; the only suggestion is to add a brief comment noting that expiration == block.timestamp is deliberate to make Permit2 approvals as short‑lived as possible.

If you haven’t already, you may want to cross‑check the current Permit2 approve signature and expiration semantics against the deployed version you target to ensure no interface drift.

270-302: Use clamped subtraction in _decreaseLiquidity for symmetry and future‑proofing

_decreaseLiquidity currently computes:
rateLimitDecrease =
    _getNormalizedBalance(token0, endingBalance0 - startingBalance0) +
    _getNormalizedBalance(token1, endingBalance1 - startingBalance1);
This assumes endingBalance{0,1} >= startingBalance{0,1} and will revert on any net outflow during a “decrease” (e.g., if future hooks or integrations ever cause tokens to leave the proxy in this path). In contrast, _increaseLiquidity already uses _clampedSub for the symmetric case.

For consistency and extra safety, consider mirroring _increaseLiquidity:
-        rateLimitDecrease =
-            _getNormalizedBalance(token0, endingBalance0 - startingBalance0) +
-            _getNormalizedBalance(token1, endingBalance1 - startingBalance1);
+        rateLimitDecrease = _clampedSub(
+            _getNormalizedBalance(token0, endingBalance0) +
+            _getNormalizedBalance(token1, endingBalance1),
+            _getNormalizedBalance(token0, startingBalance0) +
+            _getNormalizedBalance(token1, startingBalance1)
+        );
That would treat unexpected net outflows as “no rate‑limit credit” instead of reverting.

If you keep the current subtraction, it’s worth documenting the invariant that Uniswap V4 decrease/burn flows must always be net in‑flows to the proxy under your chosen actions/hook configuration.

469-474: Normalization assumes decimals() <= 18 and a standard ERC‑20 decimals implementation

_getNormalizedBalance scales via:
return balance * 1e18 / (10 ** IERC20Like(token).decimals());
This:

Works as intended for 6‑decimals stables (USDC/USDT) and other decimals() <= 18 tokens.

Will truncate towards zero for tokens with decimals() > 18 and can overflow 10 ** decimals if decimals is ever very large.

Assumes decimals() is implemented and non‑reverting.

Given the learnings that this integration is for known stable tokens and uses normalized, not USD‑denominated, limits, behavior is acceptable. If you expect to broaden token support later, consider either constraining to decimals() <= 18 or documenting that requirement.

If you consider supporting tokens beyond USDC/USDT, it’s worth reviewing the exact decimals() of those assets and updating this helper (or adding a guard) accordingly.

476-484: PoolId handling closely follows PositionManager internals; document coupling

_getPoolKey and _requirePoolIdMatch:

Cast the external bytes32 poolId to bytes25 to index poolKeys(bytes25(poolId)).

Compare keccak256(abi.encode(poolKey)) against the caller‑supplied poolId for tokenId validation.

This matches Uniswap V4’s current pattern (PoolId = keccak256(abi.encode(poolKey)) and poolKeys keyed by a truncated representation), but tightly couples this library to that internal encoding.

A short comment here explaining that:

poolId is expected to be keccak256(abi.encode(poolKey)) for the deployed _POSITION_MANAGER, and

the bytes25 cast mirrors its internal mapping key,

would help future maintainers avoid “simplifying” this in a way that breaks compatibility.

If you ever upgrade PositionManager or target another chain, double‑check that its PoolId/poolKeys encoding still matches these assumptions.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0a5bb66 and 3b7619b.

📒 Files selected for processing (2)

src/libraries/UniswapV4Lib.sol (1 hunks)
test/mainnet-fork/Uniswapv4.t.sol (1 hunks)

🧰 Additional context used

🧠 Learnings (6)

📓 Common learnings

Learnt from: deluca-mike
Repo: sparkdotfi/spark-alm-controller PR: 0
File: :0-0
Timestamp: 2025-11-11T19:43:16.032Z
Learning: In spark-alm-controller, the burnPosition and decreasePosition functions in UniswapV4Lib.sol intentionally do not validate ownership because burning someone else's position would cause the underlying tokens to flow to the proxy, which benefits the system rather than harming it.

Learnt from: deluca-mike
Repo: sparkdotfi/spark-alm-controller PR: 0
File: :0-0
Timestamp: 2025-11-11T19:43:16.032Z
Learning: In spark-alm-controller, rate limits for Uniswap positions sum both tokens normalized to 18 decimals, which doesn't account for actual USD values. This is a known design decision where normalized balance is used rather than USD-denominated values.

📚 Learning: 2025-11-04T17:15:21.481Z

Learnt from: deluca-mike
Repo: sparkdotfi/spark-alm-controller PR: 185
File: test/base-fork/Aave.t.sol:59-63
Timestamp: 2025-11-04T17:15:21.481Z
Learning: In the spark-alm-controller repository, test files in test/base-fork/ use ForkTestBase from test/base-fork/ForkTestBase.t.sol where _setControllerEntered() defaults to writing the reentrancy guard state to foreignController, while test files in test/mainnet-fork/ use ForkTestBase from test/mainnet-fork/ForkTestBase.t.sol where _setControllerEntered() defaults to mainnetController.

Applied to files:

test/mainnet-fork/Uniswapv4.t.sol

📚 Learning: 2025-11-04T17:15:39.489Z

Learnt from: deluca-mike
Repo: sparkdotfi/spark-alm-controller PR: 185
File: test/base-fork/Morpho.t.sol:122-126
Timestamp: 2025-11-04T17:15:39.489Z
Learning: In the spark-alm-controller repository, tests in the `test/base-fork/` directory use `ForkTestBase` from `test/base-fork/ForkTestBase.t.sol`, where the `_setControllerEntered()` helper function defaults to setting the reentrancy guard state on `foreignController`. Tests in `test/mainnet-fork/` use their own `ForkTestBase` which defaults to `mainnetController`.

Applied to files:

test/mainnet-fork/Uniswapv4.t.sol

📚 Learning: 2025-11-11T19:43:16.032Z

Learnt from: deluca-mike
Repo: sparkdotfi/spark-alm-controller PR: 0
File: :0-0
Timestamp: 2025-11-11T19:43:16.032Z
Learning: In spark-alm-controller, the burnPosition and decreasePosition functions in UniswapV4Lib.sol intentionally do not validate ownership because burning someone else's position would cause the underlying tokens to flow to the proxy, which benefits the system rather than harming it.

Applied to files:

test/mainnet-fork/Uniswapv4.t.sol
src/libraries/UniswapV4Lib.sol

📚 Learning: 2025-11-11T19:43:16.032Z

Learnt from: deluca-mike
Repo: sparkdotfi/spark-alm-controller PR: 0
File: :0-0
Timestamp: 2025-11-11T19:43:16.032Z
Learning: In spark-alm-controller, rate limits for Uniswap positions sum both tokens normalized to 18 decimals, which doesn't account for actual USD values. This is a known design decision where normalized balance is used rather than USD-denominated values.

Applied to files:

test/mainnet-fork/Uniswapv4.t.sol
src/libraries/UniswapV4Lib.sol

📚 Learning: 2025-10-16T16:47:34.627Z

Learnt from: supercontracts
Repo: sparkdotfi/spark-alm-controller PR: 170
File: src/MainnetController.sol:1153-1181
Timestamp: 2025-10-16T16:47:34.627Z
Learning: In src/MainnetController.sol, the USDS token always returns true on successful transfer operations, so it does not require the empty return data handling that some non-standard ERC20 tokens (like USDT) need.

Applied to files:

test/mainnet-fork/Uniswapv4.t.sol

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)

GitHub Check: build
GitHub Check: coverage
GitHub Check: test

🔇 Additional comments (4)

test/mainnet-fork/Uniswapv4.t.sol (2)

102-128: End‑to‑end coverage of Uniswap V4 controller entrypoints looks strong

These tests exercise all four relayer entrypoints (mint/increase/burn/decrease), including:

Reentrancy guard and role checks.

Tick‑limit configuration and enforcement.

Amount‑bound behavior via _getIncreasePositionMaxAmounts / _getDecreasePositionMinAmounts, consistent with the “efficiency‑ratio” slippage semantics.

Rate‑limit accounting through _DEPOSIT_LIMIT_KEY / _WITHDRAW_LIMIT_KEY keyed off (_LIMIT_* , _POOL_ID).

This gives good confidence that the MainnetController↔UniswapV4Lib↔PositionManager integration behaves as intended on a mainnet fork for the target pool.

If you want an extra sanity check, verify on the fork that RateLimitHelpers.makeBytes32Key matches the test’s _DEPOSIT_LIMIT_KEY / _WITHDRAW_LIMIT_KEY derivation for this pool so limits are aligned across contracts.

Also applies to: 297-320, 434-459

169-261: Reentrancy and access‑control tests correctly guard all new controller entrypoints

The reentrancy tests (_setControllerEntered + ReentrancyGuardReentrantCall) and the role checks for relayer/admin roles around all Uniswap V4 entrypoints give good coverage of the intended protections at the MainnetController layer.

These tests should catch regressions if reentrancy guards or AccessControl roles are accidentally relaxed in the future.

It’s worth double‑checking that every new Uniswap V4 relayer method in MainnetController.sol is decorated with the same non‑reentrant + role modifiers that these tests expect.

Also applies to: 388-459, 561-581, 587-710, 716-757

src/libraries/UniswapV4Lib.sol (2)

35-177: Uniswap V4 interaction pattern and guards look coherent; main assumptions are explicit

The four public helpers:

Mint: enforces tick‑limits via _checkTickLimits, derives PoolKey from poolId, and routes through _increaseLiquidity with Permit2 approvals and rate‑limit accounting keyed by (LIMIT_DEPOSIT, poolId).

Increase: requires ownerOf(tokenId) == proxy and _requirePoolIdMatch, then reuses _increaseLiquidity.

Burn / decrease: rely on _requirePoolIdMatch plus Position Manager approvals, intentionally omitting ownership checks so any approved position for the same pool can be burned/decreased and tokens routed to proxy (as per project learnings).

All flows normalize token balances to 1e18 and treat USDC/USDT as equal‑valued for rate limiting (also per learnings).

I don’t see a functional bug here; the main security model hinges on:

Correct poolId wiring (must be the canonical keccak256(abi.encode(poolKey)) for the target PositionManager).

Only trusted callers having the relayer/controller privileges that can invoke these helpers.

Please confirm that:

The poolId passed from MainnetController is exactly keccak256(abi.encode(poolKey)) for _POSITION_MANAGER on this network.

Only the intended relayer/controller can reach these functions (e.g. via onlyRole(RELAYER)).

308-323: Tick‑limit checks are strict and make (0,0,0) a sentinel; ensure that’s desired

_checkTickLimits:

Treats (0,0,0) as “not set” and reverts "MC/tickLimits-not-set".

Requires tickLower < tickUpper, enforces tickLower >= tickLowerMin, tickUpper <= tickUpperMax, and caps width via maxTickWidth.

This means:

You can’t deliberately set a zero‑width range or disable width limits (since maxTickWidth must be ≥ the actual width).

Setting (0,0,0) after initial provisioning fully disables the pool for new mints in this helper, which matches how tests use it.

All good as long as the sentinel behavior is intentional; consider mentioning this in controller‑level docs so operators understand that (0,0,0) means “no Uniswap V4 activity allowed for this pool”.

test/mainnet-fork/Uniswapv4.t.sol

lucas-manuel · 2025-11-20T16:19:31Z

src/libraries/UniswapV4Lib.sol

+        );
+
+        require(tickLower >= limits.tickLowerMin, "MC/tickLower-too-low");
+        require(tickUpper <= limits.tickUpperMax, "MC/tickUpper-too-high");


require lower <= upper

src/libraries/UniswapV4Lib.sol

src/interfaces/UniswapV4.sol

src/libraries/UniswapV4Lib.sol

test/mainnet-fork/Uniswapv4.t.sol

lucas-manuel · 2025-11-24T18:57:29Z

test/mainnet-fork/Uniswapv4.t.sol

+        deal(address(usdt), address(almProxy), usdt.balanceOf(address(almProxy)) + amount1Forecasted + 1);
+
+        vm.prank(relayer);
+        vm.expectRevert(); // Insufficient allowance.


Should include stdError here if thats whats failing

It's token specific (i.e. based on how that version of USDT and/or USDC was written).

coderabbitai

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3b7619b and 05787ae.

📒 Files selected for processing (4)

src/MainnetController.sol (7 hunks)
src/interfaces/UniswapV4.sol (1 hunks)
src/libraries/UniswapV4Lib.sol (1 hunks)
test/mainnet-fork/Uniswapv4.t.sol (1 hunks)

🧰 Additional context used

🧠 Learnings (5)

📓 Common learnings

Learnt from: deluca-mike
Repo: sparkdotfi/spark-alm-controller PR: 0
File: :0-0
Timestamp: 2025-11-11T19:43:16.032Z
Learning: In spark-alm-controller, the burnPosition and decreasePosition functions in UniswapV4Lib.sol intentionally do not validate ownership because burning someone else's position would cause the underlying tokens to flow to the proxy, which benefits the system rather than harming it.

Learnt from: deluca-mike
Repo: sparkdotfi/spark-alm-controller PR: 0
File: :0-0
Timestamp: 2025-11-11T19:43:16.032Z
Learning: In spark-alm-controller, rate limits for Uniswap positions sum both tokens normalized to 18 decimals, which doesn't account for actual USD values. This is a known design decision where normalized balance is used rather than USD-denominated values.

📚 Learning: 2025-11-11T19:43:16.032Z

Learnt from: deluca-mike
Repo: sparkdotfi/spark-alm-controller PR: 0
File: :0-0
Timestamp: 2025-11-11T19:43:16.032Z
Learning: In spark-alm-controller, the burnPosition and decreasePosition functions in UniswapV4Lib.sol intentionally do not validate ownership because burning someone else's position would cause the underlying tokens to flow to the proxy, which benefits the system rather than harming it.

Applied to files:

src/interfaces/UniswapV4.sol
src/MainnetController.sol
test/mainnet-fork/Uniswapv4.t.sol
src/libraries/UniswapV4Lib.sol

📚 Learning: 2025-11-11T19:43:16.032Z

Learnt from: deluca-mike
Repo: sparkdotfi/spark-alm-controller PR: 0
File: :0-0
Timestamp: 2025-11-11T19:43:16.032Z
Learning: In spark-alm-controller, rate limits for Uniswap positions sum both tokens normalized to 18 decimals, which doesn't account for actual USD values. This is a known design decision where normalized balance is used rather than USD-denominated values.

Applied to files:

src/MainnetController.sol
test/mainnet-fork/Uniswapv4.t.sol
src/libraries/UniswapV4Lib.sol

📚 Learning: 2025-11-04T17:15:21.481Z

Learnt from: deluca-mike
Repo: sparkdotfi/spark-alm-controller PR: 185
File: test/base-fork/Aave.t.sol:59-63
Timestamp: 2025-11-04T17:15:21.481Z
Learning: In the spark-alm-controller repository, test files in test/base-fork/ use ForkTestBase from test/base-fork/ForkTestBase.t.sol where _setControllerEntered() defaults to writing the reentrancy guard state to foreignController, while test files in test/mainnet-fork/ use ForkTestBase from test/mainnet-fork/ForkTestBase.t.sol where _setControllerEntered() defaults to mainnetController.

Applied to files:

test/mainnet-fork/Uniswapv4.t.sol

📚 Learning: 2025-11-04T17:15:39.489Z

Learnt from: deluca-mike
Repo: sparkdotfi/spark-alm-controller PR: 185
File: test/base-fork/Morpho.t.sol:122-126
Timestamp: 2025-11-04T17:15:39.489Z
Learning: In the spark-alm-controller repository, tests in the `test/base-fork/` directory use `ForkTestBase` from `test/base-fork/ForkTestBase.t.sol`, where the `_setControllerEntered()` helper function defaults to setting the reentrancy guard state on `foreignController`. Tests in `test/mainnet-fork/` use their own `ForkTestBase` which defaults to `mainnetController`.

Applied to files:

test/mainnet-fork/Uniswapv4.t.sol

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)

GitHub Check: coverage
GitHub Check: test
GitHub Check: build

🔇 Additional comments (7)

src/interfaces/UniswapV4.sol (1)

4-28: Interfaces match library and test usage; no changes needed

The IPositionManagerLike and IUniversalRouterLike signatures line up with how UniswapV4Lib and the fork tests call them (getPoolAndPositionInfo, poolKeys(bytes25), modifyLiquidities, execute). I don’t see any inconsistencies or missing methods for the current integration surface.

src/MainnetController.sol (3)

133-137: Tick‑limits and Uniswap V4 limit IDs are wired coherently

LIMIT_UNISWAP_V4_{DEPOSIT,WITHDRAW,SWAP} correctly mirror UniswapV4Lib’s LIMIT_* constants, so RateLimits keys are consistent between controller, library, and tests.

uniswapV4TickLimits is admin‑set via setUniswapV4TickLimits (DEFAULT_ADMIN_ROLE, nonReentrant) and enforced in the library’s _checkTickLimits, meaning per‑pool mint ranges are centrally governed while increase/decrease remain unrestricted by ticks, as intended.

Setting (0,0,0) for a pool effectively disables new mints via the library’s “tickLimits-not-set” guard, matching how tests use this as a “no new mints” switch.

Looks sound from a control‑surface and configuration perspective.

Also applies to: 158-160, 202-203, 318-337

634-701: Uniswap V4 mint/increase/decrease entrypoints respect roles, reentrancy, and library invariants

All three functions are RELAYER‑only and nonReentrant, which is important given V4 hooks can execute arbitrary code and attempt to re‑enter.

They delegate all Uniswap‑specific logic (Permit2 approvals, PositionManager.modifyLiquidities, poolId checks, tick‑limit enforcement, and rate‑limit accounting) to UniswapV4Lib, keeping this contract’s surface small and easier to audit.

increaseLiquidityUniswapV4 and decreaseLiquidityUniswapV4 rely on the library’s poolId/owner checks (owner enforced only on increase, intentionally omitted on decrease per design) and per‑pool rate‑limits; tests exercise both the happy paths and the “non‑proxy position” / “tokenId‑poolId‑mismatch” failure modes.

Given the current design goals, the controller side of the V4 liquidity flows looks correct and appropriately constrained.

703-721: Swap wiring to Uniswap V4 and slippage/rate‑limit controls look correct

swapUniswapV4 is RELAYER‑only, nonReentrant, and delegates to UniswapV4Lib.swap, which performs:

A maxSlippage != 0 guard (so swaps are disabled unless configured),

A semantic check that amountOutMin is consistent with the configured maxSlippage,

A V4 Universal Router call using Permit2, and

A per‑pool rate‑limit decrease keyed on (LIMIT_SWAP, poolId).

The controller’s maxSlippage lookup via maxSlippages[address(uint160(uint256(poolId)))] matches how tests set the slippage for this pool, and the library enforces the “max-slippage-not-set” / “amount-out-min-too-low” reverts which the tests assert on.

From the controller’s side, the swap integration with Uniswap V4 plus slippage and rate‑limit checks behaves as intended.

test/mainnet-fork/Uniswapv4.t.sol (1)

149-1574: Tests exercise Uniswap V4 integration, hooks, rate‑limits, and attack scenarios thoroughly

Reentrancy tests (test_*_reentrancy) use _setControllerEntered() to assert that all Uniswap V4 entrypoints respect the controller’s nonReentrant guard, which is key against malicious V4 hooks.

Role‑based tests (revertsForNonRelayer, revertsWhenTickLimitsNotSet, revertsWhenTokenIsNotForPool, etc.) validate that only RELAYER and admin roles can touch the new flows and that poolId/tokenId/tick invariants are enforced.

Rate‑limit tests around _DEPOSIT_LIMIT_KEY, _WITHDRAW_LIMIT_KEY, and _SWAP_LIMIT_KEY confirm that the library’s normalized‑balance accounting for deposit/withdraw/swap matches the external RateLimits contract behavior.

The story/attack tests (test_uniswapV4_story1 and test_uniswapV4_attack_*) exercise realistic front‑run/back‑run price moves on the real USDC/USDT V4 pool, demonstrating the economic impact while still validating that rate‑limits and tick‑limits behave deterministically.

Overall this suite gives strong end‑to‑end assurance that the new Uniswap V4 integration behaves correctly and that the intended defenses (ticks, slippage, rate‑limits, reentrancy guards) actually hold under adversarial flows.

src/libraries/UniswapV4Lib.sol (2)

40-150: Uniswap V4 liquidity helpers correctly wrap PositionManager with tick/rate‑limit and owner safeguards

mintPosition enforces per‑pool TickLimits via _checkTickLimits before building MINT_POSITION + SETTLE_PAIR calldata, then uses _increaseLiquidity to:

Approve token0/token1 to Permit2 and PositionManager,

Measure pre/post balances at the proxy,

Compute a normalized deposit delta (with _clampedSub to ignore pathological “net in” cases), and

Trigger a rate‑limit decrease keyed on (LIMIT_DEPOSIT, poolId), followed by clearing approvals.

increasePosition adds an explicit ownerOf(tokenId) == proxy check plus _requirePoolIdMatch, so added liquidity always accrues to the proxy’s NFT and only for the intended pool.

decreasePosition relies on _requirePoolIdMatch and PositionManager’s own approvals, then routes all proceeds back to the proxy and credits (LIMIT_WITHDRAW, poolId) based on normalized balance deltas, matching how the tests compute expected rate‑limit usage.

This structure gives a clear, well‑bounded integration surface to Uniswap V4: ticks, ownership, and per‑pool limits are checked here, while MainnetController only handles roles and dispatch.

219-304: Permit2 approval pattern is subtle but implemented correctly for PositionManager flows

_approveWithPermit2:

Best‑effort zeros the token→Permit2 allowance via a low‑level proxy.call(IALMProxy.doCall(...approve(...,0))), explicitly ignoring failure.

When amount != 0, performs a mandatory IERC20Like.approve(_PERMIT2, amount) through IALMProxy.doCall, requiring either empty or true return data to handle both ERC20 variants.

Grants a Permit2 allowance for (user=proxy, token, spender, amount, expiration=block.timestamp), intentionally limiting spender use to the current transaction.

_increaseLiquidity then:

Calls _approveWithPermit2 for token0 and token1 with positive amounts before modifyLiquidities,

Measures normalized pre/post balances to drive (LIMIT_DEPOSIT, poolId) rate‑limits, and

Calls _approveWithPermit2(..., 0) afterward to clear both token→Permit2 and Permit2→PositionManager allowances.

Tests validate that allowances are returned to zero after mint/increase flows. Given the interaction with external ERC20s and Permit2, this pattern looks correct and appropriately defensive.

src/libraries/UniswapV4Lib.sol

test/mainnet-fork/Uniswapv4.t.sol

src/libraries/UniswapV4Lib.sol

test/mainnet-fork/Uniswapv4.t.sol

lucas-manuel · 2025-11-24T22:55:29Z

test/mainnet-fork/Uniswapv4.t.sol

+
+        mainnetController.swapUniswapV4(_POOL_ID, address(usdc), 1_000_000e6, 999_281e6);
+    }
+


We can change this to 6 hours to be more realistic, same for all warps

lucas-manuel · 2025-11-24T22:56:41Z

test/mainnet-fork/Uniswapv4.t.sol

+            tickLower  : -30,
+            tickUpper  : -10,
+            liquidity  : 1_000_000_000e6,
+            amount0Max : amount0Max,
+            amount1Max : amount1Max
+        });
+    }
+
+    function test_uniswapV4_attack_baseline_priceAbove_wideTicks() public {
+        // Setup the pool and the controller.
+        vm.startPrank(SPARK_PROXY);
+        mainnetController.setUniswapV4TickLimits(_POOL_ID, -200, 200, 200); // Allow wider tick range.
+        rateLimits.setRateLimitData(_DEPOSIT_LIMIT_KEY,  20_000_000e18, uint256(20_000_000e18) / 1 days);
+        rateLimits.setRateLimitData(_WITHDRAW_LIMIT_KEY, 20_000_000e18, uint256(20_000_000e18) / 1 days);
+        vm.stopPrank();
+
+        /******************************************************************************************/
+        /*** Add Liquidity (Current price is expected to be above the range)                    ***/
+        /******************************************************************************************/
+
+        assertEq(_getCurrentTick(), -7);
+
+        IncreasePositionResult memory increaseResult = _mintPosition(-200, -10, 1_000_000_000e6, type(uint160).max, type(uint160).max);
+
+        assertEq(increaseResult.amount0Spent, 0);
+        assertEq(increaseResult.amount1Spent, 9_449_821.223798e6);
+        assertEq(increaseResult.amount1Spent + increaseResult.amount0Spent, 9_449_821.223798e6);
+
+        /******************************************************************************************/
+        /*** Remove Liquidity                                                                   ***/
+        /******************************************************************************************/
+
+        assertEq(_getCurrentTick(), -7);
+
+        DecreasePositionResult memory decreaseResult = _decreasePosition(increaseResult.tokenId, 1_000_000_000e6, 0, 0);
+
+        assertEq(decreaseResult.amount0Received, 0);
+        assertEq(decreaseResult.amount1Received, 9_449_821.223797e6);
+        assertEq(decreaseResult.amount0Received + decreaseResult.amount1Received, 9_449_821.223797e6); // Lost 0 USD.
+    }
+
+    function test_uniswapV4_attack_priceAboveToBelow_wideTicks() public {
+        // Setup the pool and the controller.
+        vm.startPrank(SPARK_PROXY);
+        mainnetController.setUniswapV4TickLimits(_POOL_ID, -200, 200, 200); // Allow wider tick range.
+        rateLimits.setRateLimitData(_DEPOSIT_LIMIT_KEY,  20_000_000e18, uint256(20_000_000e18) / 1 days);
+        rateLimits.setRateLimitData(_WITHDRAW_LIMIT_KEY, 20_000_000e18, uint256(20_000_000e18) / 1 days);
+        vm.stopPrank();
+
+        /******************************************************************************************/
+        /*** Frontrun                                                                           ***/
+        /******************************************************************************************/
+
+        assertEq(_getCurrentTick(), -7);
+
+        uint256 amountOut1 = _externalSwap(_alice, address(usdc), 3_020_000e6);
+
+        /******************************************************************************************/
+        /*** Add Liquidity (Current price was expected to be above the range, but is below)     ***/
+        /******************************************************************************************/
+
+        assertEq(_getCurrentTick(), -501);
+
+        IncreasePositionResult memory increaseResult = _mintPosition(-200, -10, 1_000_000_000e6, type(uint160).max, type(uint160).max);
+
+        assertEq(increaseResult.amount0Spent, 9_549_562.082877e6); // Expected 0
+        assertEq(increaseResult.amount1Spent, 0); // Expected 9_449_821.223798e6
+        assertEq(increaseResult.amount1Spent + increaseResult.amount0Spent, 9_549_562.082877e6); // Expected 9_449_821.223798e6
+
+        /******************************************************************************************/
+        /*** Backrun                                                                            ***/
+        /******************************************************************************************/
+
+        assertEq(_getCurrentTick(), -501);
+
+        uint256 amountOut2 = _externalSwap(_alice, address(usdt), uint128(amountOut1));
+
+        assertEq(amountOut2, 3_067_685.526025e6);


These should be moved into a dedicated section for user actions where assertions are also performed

Other functions should be consolidated in here as well

coderabbitai

Actionable comments posted: 2

♻️ Duplicate comments (1)

src/libraries/UniswapV4Lib.sol (1)
348-358: Consider defensive subtraction for consistency.

Unlike _increaseLiquidity which uses _clampedSub to handle theoretical edge cases, _decreaseLiquidity performs raw subtraction (endingBalance - startingBalance). While the DECREASE_LIQUIDITY + TAKE_PAIR flow should always increase the proxy's balance, using consistent defensive patterns across similar operations would prevent potential reverts from unexpected edge cases (e.g., exotic hooks or fee-on-transfer interactions).
-        uint256 rateLimitDecrease =
-            _getNormalizedBalance(token0, endingBalance0 - startingBalance0) +
-            _getNormalizedBalance(token1, endingBalance1 - startingBalance1);
+        // Account for the theoretical possibility of spending tokens when removing liquidity
+        // (e.g., due to hooks or other edge cases) by using clamped subtraction.
+        uint256 rateLimitDecrease =
+            _getNormalizedBalance(token0, _clampedSub(endingBalance0, startingBalance0)) +
+            _getNormalizedBalance(token1, _clampedSub(endingBalance1, startingBalance1));

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 949f62f and 9ca1d11.

📒 Files selected for processing (11)

.gitmodules (1 hunks)
foundry.toml (1 hunks)
lib/uniswap-v4-core (1 hunks)
lib/uniswap-v4-periphery (1 hunks)
src/MainnetController.sol (7 hunks)
src/RateLimitHelpers.sol (1 hunks)
src/interfaces/Common.sol (1 hunks)
src/interfaces/UniswapV4.sol (1 hunks)
src/libraries/UniswapV4Lib.sol (1 hunks)
test/mainnet-fork/Curve.t.sol (0 hunks)
test/mainnet-fork/Uniswapv4.t.sol (1 hunks)

💤 Files with no reviewable changes (1)

test/mainnet-fork/Curve.t.sol

🧰 Additional context used

🧠 Learnings (6)

📓 Common learnings

Learnt from: deluca-mike
Repo: sparkdotfi/spark-alm-controller PR: 0
File: :0-0
Timestamp: 2025-11-11T19:43:16.032Z
Learning: In spark-alm-controller, the burnPosition and decreasePosition functions in UniswapV4Lib.sol intentionally do not validate ownership because burning someone else's position would cause the underlying tokens to flow to the proxy, which benefits the system rather than harming it.

Learnt from: deluca-mike
Repo: sparkdotfi/spark-alm-controller PR: 0
File: :0-0
Timestamp: 2025-11-11T19:43:16.032Z
Learning: In spark-alm-controller, rate limits for Uniswap positions sum both tokens normalized to 18 decimals, which doesn't account for actual USD values. This is a known design decision where normalized balance is used rather than USD-denominated values.

📚 Learning: 2025-11-11T19:43:16.032Z

Learnt from: deluca-mike
Repo: sparkdotfi/spark-alm-controller PR: 0
File: :0-0
Timestamp: 2025-11-11T19:43:16.032Z
Learning: In spark-alm-controller, the burnPosition and decreasePosition functions in UniswapV4Lib.sol intentionally do not validate ownership because burning someone else's position would cause the underlying tokens to flow to the proxy, which benefits the system rather than harming it.

Applied to files:

src/libraries/UniswapV4Lib.sol
src/interfaces/UniswapV4.sol
test/mainnet-fork/Uniswapv4.t.sol
src/MainnetController.sol

📚 Learning: 2025-11-11T19:43:16.032Z

Learnt from: deluca-mike
Repo: sparkdotfi/spark-alm-controller PR: 0
File: :0-0
Timestamp: 2025-11-11T19:43:16.032Z
Learning: In spark-alm-controller, rate limits for Uniswap positions sum both tokens normalized to 18 decimals, which doesn't account for actual USD values. This is a known design decision where normalized balance is used rather than USD-denominated values.

Applied to files:

src/libraries/UniswapV4Lib.sol
test/mainnet-fork/Uniswapv4.t.sol
src/MainnetController.sol

📚 Learning: 2025-11-04T17:15:21.481Z

Learnt from: deluca-mike
Repo: sparkdotfi/spark-alm-controller PR: 185
File: test/base-fork/Aave.t.sol:59-63
Timestamp: 2025-11-04T17:15:21.481Z
Learning: In the spark-alm-controller repository, test files in test/base-fork/ use ForkTestBase from test/base-fork/ForkTestBase.t.sol where _setControllerEntered() defaults to writing the reentrancy guard state to foreignController, while test files in test/mainnet-fork/ use ForkTestBase from test/mainnet-fork/ForkTestBase.t.sol where _setControllerEntered() defaults to mainnetController.

Applied to files:

test/mainnet-fork/Uniswapv4.t.sol

📚 Learning: 2025-11-04T17:15:39.489Z

Learnt from: deluca-mike
Repo: sparkdotfi/spark-alm-controller PR: 185
File: test/base-fork/Morpho.t.sol:122-126
Timestamp: 2025-11-04T17:15:39.489Z
Learning: In the spark-alm-controller repository, tests in the `test/base-fork/` directory use `ForkTestBase` from `test/base-fork/ForkTestBase.t.sol`, where the `_setControllerEntered()` helper function defaults to setting the reentrancy guard state on `foreignController`. Tests in `test/mainnet-fork/` use their own `ForkTestBase` which defaults to `mainnetController`.

Applied to files:

test/mainnet-fork/Uniswapv4.t.sol

📚 Learning: 2025-10-16T16:47:34.627Z

Learnt from: supercontracts
Repo: sparkdotfi/spark-alm-controller PR: 170
File: src/MainnetController.sol:1153-1181
Timestamp: 2025-10-16T16:47:34.627Z
Learning: In src/MainnetController.sol, the USDS token always returns true on successful transfer operations, so it does not require the empty return data handling that some non-standard ERC20 tokens (like USDT) need.

Applied to files:

test/mainnet-fork/Uniswapv4.t.sol

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)

GitHub Check: coverage
GitHub Check: build
GitHub Check: test

🔇 Additional comments (39)

lib/uniswap-v4-periphery (1)

1-1: Original review comment is based on incorrect assumption—v4.0.0 tag does not exist in Uniswap v4-periphery.

The prior review suggested pinning v4-periphery to v4.0.0 to match v4-core, but v4-periphery has no published releases or tags. Using the HEAD commit is likely the correct approach for this repository, as it does not maintain formal release tags like v4-core does. The version mismatch concern is invalid given the different release management practices between the two projects.

Likely an incorrect or invalid review comment.

foundry.toml (2)

13-22: Remapping syntax is correct and consistent.

The new @uniswap/v4-core remapping follows the established pattern, is syntactically correct, and does not conflict with existing remappings.

21-21: The review comment based on an unfounded assumption—no v4-periphery imports or directory exist in the codebase.

Verification shows:

No imports from @uniswap/v4-periphery found in any Solidity files

The lib/uniswap-v4-periphery directory does not exist in the repository

The @uniswap/v4-core remapping at line 21 is correctly added and sufficient for the actual codebase needs

The original concern assumed v4-periphery would be used directly, but the code either uses only custom interfaces or imports only from v4-core. The remapping addition is correct as-is.

Likely an incorrect or invalid review comment.

lib/uniswap-v4-core (2)

1-1: Acknowledge intentional Uniswap V4 design decisions per prior review.

Per the PR objectives and retrieved learnings, the prior automated review identified several design-level considerations—including missing ownership checks in burnPosition/decreasePosition, poolId→address casting, and bytes32→bytes25 truncation. The PR author has documented that these are intentional alignments with Uniswap V4 Position Manager design and do not represent security concerns in the context of this system's risk controls (tick-range limits, slippage checks, and rate limiting).

Ensure that the submodule commit does not introduce unexpected changes that would affect these already-reviewed and documented design decisions.

Based on learnings, the ownership omission in burnPosition and decreasePosition is an intentional design decision where burning someone else's position routes tokens to the proxy and is considered beneficial. Similarly, rate limits normalize token amounts to 18 decimals rather than USD values—also intentional.

1-1: Submodule configuration verified and correct.

The commit hash e50237c43811bd9b526eff40f26772152a42daba is valid, reachable (tagged as v4.0.0 in the Uniswap V4 Core repository), and the submodule is properly registered with the correct Foundry remapping. No issues identified.

.gitmodules (1)

50-55: LGTM!

The submodule additions for Uniswap V4 core and periphery are correctly configured with official Uniswap repository URLs. Submodule commit SHAs will pin the exact versions used.

src/interfaces/Common.sol (1)

1-18: LGTM!

Clean interface definitions that expose only the required ERC20 and Permit2 functions needed for the Uniswap V4 integration. The named return values improve readability.

src/RateLimitHelpers.sol (1)

6-20: LGTM!

The refactored helpers with generic parameter names (a, b) and the new makeBytes32Key function provide a cleaner, more consistent API for key construction. The new makeBytes32Key directly supports the poolId-based rate limiting in UniswapV4Lib.

src/interfaces/UniswapV4.sol (1)

9-28: LGTM!

The interface definitions correctly expose the required Uniswap V4 PositionManager and UniversalRouter functions. The function ordering (state-mutating modifyLiquidities first, then view functions) follows a reasonable convention.

src/libraries/UniswapV4Lib.sol (11)

17-34: LGTM!

The struct and constants are well-defined. The rate limit identifiers use unique keccak256 hashes to prevent collisions. The mainnet addresses are documented with their source.

40-78: LGTM!

The mintPosition function correctly validates tick limits before building calldata and delegating to _increaseLiquidity. The proxy is set as the NFT recipient in _getMintCalldata, ensuring ownership.

80-119: LGTM!

The increasePosition function correctly enforces ownership (ownerOf(tokenId) == proxy) before allowing liquidity increase. This ensures the proxy retains ownership of the increased liquidity.

121-158: LGTM!

The intentional omission of ownership validation is well-documented in the inline comment (lines 132-134). Based on learnings, this design allows decreasing any position (for which the proxy has approval) with tokens flowing to the proxy, which benefits the system.

160-223: LGTM!

The swap function correctly implements:

Slippage validation using the project's inverted semantics (minimum efficiency ratio)

Protocol-level validation of tokenIn via Uniswap V4's CurrencyNotSettled revert

Permit2 approval flow for the router

Rate limiting based on normalized input amount

The swap rate limit tracks only tokenIn amount, which is a reasonable simplification for stablecoin pools.

229-272: LGTM!

The Permit2 approval pattern is well-implemented:

Best-effort zeroing via low-level call handles non-standard tokens (e.g., USDT)

Mandatory success path validates return data for both bool-returning and void ERC20s

expiration = uint48(block.timestamp) creates transient Permit2 approvals limited to the current block/transaction

274-323: LGTM!

The _increaseLiquidity implementation:

Uses _clampedSub to defensively handle edge cases where tokens might be received

Documents the known limitation of equal-value token assumption (line 302-303)

Properly cleans up Permit2 approvals after the operation

365-377: LGTM!

The _checkTickLimits function correctly validates:

Configuration is set (maxTickSpacing != 0)

Tick ordering (tickLower < tickUpper - strict inequality ensures non-zero range)

Bounds (tickLower >= tickLowerMin, tickUpper <= tickUpperMax)

Width constraint with proper casting to handle negative tick values

379-385: LGTM!

Clean utility functions. _clampedSub provides safe subtraction to prevent underflow.

387-496: LGTM!

The calldata construction functions correctly encode:

Appropriate action sequences for each operation (MINT/INCREASE/DECREASE with settlement)

The proxy as recipient for mint and decrease operations

block.timestamp as deadline for immediate execution

498-515: LGTM!

These utility functions correctly implement:

Balance normalization to 18 decimals (acceptable for the known 6-decimal stablecoins in use)

bytes25 cast matching Uniswap V4's internal poolKeys mapping key format

poolId verification using consistent keccak256(abi.encode(poolKey)) encoding

src/MainnetController.sol (7)

133-138: LGTM!

The event declaration is well-structured with poolId indexed for efficient filtering.

163-165: LGTM!

Exposing the library constants as public state variables maintains consistency with other rate limit identifiers and provides external visibility.

207-209: LGTM!

Per-pool tick limits storage is appropriately keyed by bytes32 poolId.

323-346: LGTM!

The validation logic correctly handles both the "disabled" state (all zeros) and valid configurations. The requirement that tickLowerMin < tickUpperMax (strict inequality) properly rejects zero-width ranges.

643-666: LGTM!

Proper access control, reentrancy protection, and clean delegation to UniswapV4Lib.mintPosition. The parameter ordering has been corrected with tickLower before tickUpper.

668-710: LGTM!

Both functions have proper RELAYER access control and nonReentrant protection. The reentrancy guard issue from previous review has been addressed. Based on learnings, the intentional omission of ownership validation in decreaseLiquidityUniswapV4 is a design decision—decreasing someone else's position would route tokens to the proxy, which benefits the system.

948-948: LGTM!

Minor syntactic cleanup to the destructuring assignment; no functional change.

test/mainnet-fork/Uniswapv4.t.sol (12)

26-35: LGTM!

The IERC20Like interface intentionally omits the return type on approve to handle non-conformant tokens (as noted in the comment). This is appropriate for test compatibility.

129-137: LGTM!

The limit key constants correctly mirror the derivation pattern used in UniswapV4Lib, ensuring test assertions match production behavior.

201-241: LGTM!

Good boundary testing for tick ordering: verifies revert when tickLower > tickUpper, revert when tickLower == tickUpper, and success when tickLower < tickUpper.

480-495: LGTM!

This test correctly verifies that increaseLiquidityUniswapV4 rejects positions not owned by the proxy, which is the expected security control for increasing liquidity on existing positions.

780-808: LGTM!

Both partial and full decrease tests properly verify balance changes, rate limit consumption, and position liquidity updates. The tests correctly use _getDecreasePositionMinAmounts for slippage protection.

839-853: LGTM!

Tests correctly verify two layers of slippage protection:

Controller-level check ("MC/amountOutMin-too-low") ensures amountOutMin >= amountIn * maxSlippage

Router-level check (V4TooLittleReceived) ensures actual output meets minimum

Both token swap directions are also covered.

Also applies to: 866-889

1015-1071: LGTM!

test_uniswapV4_story1 provides excellent end-to-end coverage: a complete round-trip of mint → increase → decrease → close with external swaps interspersed, verifying rate limit accounting at each step.

1103-1187: LGTM!

The attack scenario tests provide valuable documentation of MEV exposure under various price manipulation conditions. The "defended" tests (test_uniswapV4_attack_priceAboveToBelow_defended, test_uniswapV4_attack_priceAboveToBelow_defended_wideTicks) demonstrate that the amount0Max/amount1Max parameters and tick spacing limits effectively mitigate sandwich attacks.

Also applies to: 1520-1564

1722-1772: LGTM!

The _mintPosition helper is well-structured with comprehensive assertions: verifies amounts spent are within max, rate limit is correctly consumed, position ownership, position liquidity, and allowances are zeroed. This consolidates assertions for cleaner tests.

2123-2129: LGTM!

The assertion messages have been updated to be token-agnostic ("permit2 allowance not 0", "allowance to permit2 not 0"), addressing the previous review feedback.

2135-2137: Future block number for fork testing.

The test uses block 23470490 (September 29, 2025), which is a future block. This appears intentional to test against a specific pool state. Consider adding a comment explaining why this specific block was chosen if there are particular state requirements.

2139-2182: LGTM!

The _externalSwap helper correctly implements the Uniswap V4 swap flow via Universal Router: token dealing, Permit2 approval chain, command encoding with SWAP_EXACT_IN_SINGLE + SETTLE_ALL + TAKE_ALL actions, and proper balance delta tracking.

coderabbitai · 2025-11-25T05:02:54Z

src/MainnetController.sol

+    function swapUniswapV4(
+        bytes32 poolId,
+        address tokenIn,
+        uint128 amountIn,
+        uint128 amountOutMin
+    )
+        external nonReentrant
+    {
+        _checkRole(RELAYER);
+
+        UniswapV4Lib.swap({
+            proxy        : address(proxy),
+            rateLimits   : address(rateLimits),
+            poolId       : poolId,
+            tokenIn      : tokenIn,
+            amountIn     : amountIn,
+            amountOutMin : amountOutMin,
+            maxSlippage  : maxSlippages[address(uint160(uint256(poolId)))]
+        });
+    }


🧹 Nitpick | 🔵 Trivial

Consider adding inline documentation for the poolId to address casting.

The maxSlippages lookup on line 729 uses address(uint160(uint256(poolId))), which truncates the bytes32 pool ID to 160 bits. Per PR discussion, this aligns with Uniswap V4's design, but consider adding a brief inline comment explaining this casting rationale for future maintainers.

UniswapV4Lib.swap({ proxy : address(proxy), rateLimits : address(rateLimits), poolId : poolId, tokenIn : tokenIn, amountIn : amountIn, amountOutMin : amountOutMin, + // NOTE: poolId is cast to address (lower 160 bits) to align with Uniswap V4's design maxSlippage : maxSlippages[address(uint160(uint256(poolId)))] });

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

function swapUniswapV4(

bytes32 poolId,

address tokenIn,

uint128 amountIn,

uint128 amountOutMin

)

external nonReentrant

{

_checkRole(RELAYER);

UniswapV4Lib.swap({

proxy : address(proxy),

rateLimits : address(rateLimits),

poolId : poolId,

tokenIn : tokenIn,

amountIn : amountIn,

amountOutMin : amountOutMin,

maxSlippage : maxSlippages[address(uint160(uint256(poolId)))]

});

}

function swapUniswapV4(

bytes32 poolId,

address tokenIn,

uint128 amountIn,

uint128 amountOutMin

)

external nonReentrant

{

_checkRole(RELAYER);

UniswapV4Lib.swap({

proxy : address(proxy),

rateLimits : address(rateLimits),

poolId : poolId,

tokenIn : tokenIn,

amountIn : amountIn,

amountOutMin : amountOutMin,

// NOTE: poolId is cast to address (lower 160 bits) to align with Uniswap V4's design

maxSlippage : maxSlippages[address(uint160(uint256(poolId)))]

});

}

🤖 Prompt for AI Agents

In src/MainnetController.sol around lines 712 to 731, the lookup maxSlippages[address(uint160(uint256(poolId)))] truncates the bytes32 poolId to a 160-bit address without explanation; add a concise inline comment immediately above that expression (or assign the cast to a well-named local variable like address poolAddr = address(uint160(uint256(poolId)));) stating that Uniswap V4 encodes the pool's address in the low 160 bits of poolId and that this cast intentionally extracts that address for the maxSlippages mapping, so future maintainers understand the rationale and avoid accidental refactors.

coderabbitai · 2025-11-25T05:02:55Z

test/mainnet-fork/Uniswapv4.t.sol

+    function test_setUniswapV4tickLimits_revertsForNonRelayer() external {
+        vm.expectRevert(
+            abi.encodeWithSelector(
+                IAccessControl.AccessControlUnauthorizedAccount.selector,
+                _unauthorized,
+                mainnetController.DEFAULT_ADMIN_ROLE()
+            )
+        );
+
+        vm.prank(_unauthorized);
+        mainnetController.setUniswapV4TickLimits(bytes32(0), 0, 0, 0);
+    }


⚠️ Potential issue | 🟡 Minor

Test name is misleading: function checks DEFAULT_ADMIN_ROLE, not RELAYER.

The test is named test_setUniswapV4tickLimits_revertsForNonRelayer but verifies DEFAULT_ADMIN_ROLE access control. Consider renaming to test_setUniswapV4tickLimits_revertsForNonAdmin for clarity.

- function test_setUniswapV4tickLimits_revertsForNonRelayer() external { + function test_setUniswapV4tickLimits_revertsForNonAdmin() external {

🤖 Prompt for AI Agents

In test/mainnet-fork/Uniswapv4.t.sol around lines 956 to 967, the test currently asserts that calling setUniswapV4TickLimits reverts due to missing DEFAULT_ADMIN_ROLE but the function name says "NonRelayer"; rename the test to reflect ADMIN role (e.g., test_setUniswapV4tickLimits_revertsForNonAdmin) and update any references or test runner expectations accordingly so the name matches the asserted access control check.

github-actions · 2025-11-25T05:19:31Z

Coverage after merging sc-1234-uniswap-v4-support into dev will be

99.61%

Coverage Report

File	Stmts	Branches	Funcs	Lines	Uncovered Lines
deploy
ControllerDeploy.sol	100%	100%	100%	100%
ForeignControllerInit.sol	100%	100%	100%	100%
MainnetControllerInit.sol	97.37%	93.33%	100%	100%	152, 90
src
ALMProxy.sol	100%	100%	100%	100%
ALMProxyFreezable.sol	100%	100%	100%	100%
ForeignController.sol	98.24%	91.67%	100%	99.16%	312–313, 613
MainnetController.sol	99.16%	100%	98.25%	99.22%	537–538
OTCBuffer.sol	100%	100%	100%	100%
RateLimitHelpers.sol	100%	100%	100%	100%
RateLimits.sol	100%	100%	100%	100%
src/libraries
AaveLib.sol	100%	100%	100%	100%
ApproveLib.sol	100%	100%	100%	100%
CCTPLib.sol	100%	100%	100%	100%
CurveLib.sol	100%	100%	100%	100%
ERC4626Lib.sol	96%	75%	100%	100%	108
PSMLib.sol	100%	100%	100%	100%
UniswapV4Lib.sol	99.14%	94.12%	100%	100%	188

coderabbitai

Actionable comments posted: 3

♻️ Duplicate comments (1)

test/mainnet-fork/Uniswapv4.t.sol (1)

796-807: Test name is misleading.

The test is named test_setUniswapV4tickLimits_revertsForNonRelayer but verifies DEFAULT_ADMIN_ROLE access control (Line 801). Consider renaming to test_setUniswapV4tickLimits_revertsForNonAdmin for clarity.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9ca1d11 and 6c06cbb.

📒 Files selected for processing (11)

.gitmodules (1 hunks)
foundry.toml (1 hunks)
lib/uniswap-v4-core (1 hunks)
lib/uniswap-v4-periphery (1 hunks)
src/MainnetController.sol (7 hunks)
src/RateLimitHelpers.sol (1 hunks)
src/interfaces/Common.sol (1 hunks)
src/interfaces/UniswapV4.sol (1 hunks)
src/libraries/UniswapV4Lib.sol (1 hunks)
test/mainnet-fork/Curve.t.sol (0 hunks)
test/mainnet-fork/Uniswapv4.t.sol (1 hunks)

💤 Files with no reviewable changes (1)

test/mainnet-fork/Curve.t.sol

🧰 Additional context used

🧠 Learnings (6)

📓 Common learnings

Learnt from: deluca-mike
Repo: sparkdotfi/spark-alm-controller PR: 0
File: :0-0
Timestamp: 2025-11-11T19:43:16.032Z
Learning: In spark-alm-controller, the burnPosition and decreasePosition functions in UniswapV4Lib.sol intentionally do not validate ownership because burning someone else's position would cause the underlying tokens to flow to the proxy, which benefits the system rather than harming it.

Learnt from: deluca-mike
Repo: sparkdotfi/spark-alm-controller PR: 0
File: :0-0
Timestamp: 2025-11-11T19:43:16.032Z
Learning: In spark-alm-controller, rate limits for Uniswap positions sum both tokens normalized to 18 decimals, which doesn't account for actual USD values. This is a known design decision where normalized balance is used rather than USD-denominated values.

📚 Learning: 2025-11-11T19:43:16.032Z

Learnt from: deluca-mike
Repo: sparkdotfi/spark-alm-controller PR: 0
File: :0-0
Timestamp: 2025-11-11T19:43:16.032Z
Learning: In spark-alm-controller, the burnPosition and decreasePosition functions in UniswapV4Lib.sol intentionally do not validate ownership because burning someone else's position would cause the underlying tokens to flow to the proxy, which benefits the system rather than harming it.

Applied to files:

src/interfaces/UniswapV4.sol
src/libraries/UniswapV4Lib.sol
test/mainnet-fork/Uniswapv4.t.sol
src/MainnetController.sol

📚 Learning: 2025-11-11T19:43:16.032Z

Learnt from: deluca-mike
Repo: sparkdotfi/spark-alm-controller PR: 0
File: :0-0
Timestamp: 2025-11-11T19:43:16.032Z
Learning: In spark-alm-controller, rate limits for Uniswap positions sum both tokens normalized to 18 decimals, which doesn't account for actual USD values. This is a known design decision where normalized balance is used rather than USD-denominated values.

Applied to files:

src/libraries/UniswapV4Lib.sol
test/mainnet-fork/Uniswapv4.t.sol
src/MainnetController.sol

📚 Learning: 2025-11-04T17:15:21.481Z

Learnt from: deluca-mike
Repo: sparkdotfi/spark-alm-controller PR: 185
File: test/base-fork/Aave.t.sol:59-63
Timestamp: 2025-11-04T17:15:21.481Z
Learning: In the spark-alm-controller repository, test files in test/base-fork/ use ForkTestBase from test/base-fork/ForkTestBase.t.sol where _setControllerEntered() defaults to writing the reentrancy guard state to foreignController, while test files in test/mainnet-fork/ use ForkTestBase from test/mainnet-fork/ForkTestBase.t.sol where _setControllerEntered() defaults to mainnetController.

Applied to files:

test/mainnet-fork/Uniswapv4.t.sol

📚 Learning: 2025-11-04T17:15:39.489Z

Learnt from: deluca-mike
Repo: sparkdotfi/spark-alm-controller PR: 185
File: test/base-fork/Morpho.t.sol:122-126
Timestamp: 2025-11-04T17:15:39.489Z
Learning: In the spark-alm-controller repository, tests in the `test/base-fork/` directory use `ForkTestBase` from `test/base-fork/ForkTestBase.t.sol`, where the `_setControllerEntered()` helper function defaults to setting the reentrancy guard state on `foreignController`. Tests in `test/mainnet-fork/` use their own `ForkTestBase` which defaults to `mainnetController`.

Applied to files:

test/mainnet-fork/Uniswapv4.t.sol

📚 Learning: 2025-10-16T16:47:34.627Z

Learnt from: supercontracts
Repo: sparkdotfi/spark-alm-controller PR: 170
File: src/MainnetController.sol:1153-1181
Timestamp: 2025-10-16T16:47:34.627Z
Learning: In src/MainnetController.sol, the USDS token always returns true on successful transfer operations, so it does not require the empty return data handling that some non-standard ERC20 tokens (like USDT) need.

Applied to files:

test/mainnet-fork/Uniswapv4.t.sol

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)

GitHub Check: build
GitHub Check: coverage
GitHub Check: test

🔇 Additional comments (32)

src/RateLimitHelpers.sol (1)

6-20: LGTM! Generic parameter names and new bytes32 helper align with reusable rate-limit key construction.

The refactoring to generic parameter names (a, b instead of asset, destination, domain) increases reusability while maintaining type safety through abi.encode. The new makeBytes32Key helper supports Uniswap V4 poolId (bytes32) integration and follows the same collision-resistant pattern as the existing helpers.

lib/uniswap-v4-periphery (1)

1-1: I need to check what release tags actually exist on v4-periphery:

I need to verify if the current pinned commit is the latest HEAD on v4-periphery:

The original review comment's major issue is invalid. The v4-periphery contracts in this repository are still in development—no v4.0.0 release tag exists because the project has not published formal releases, unlike v4-core. Pinning to a valid commit (3779387e5d296f39df543d23524b050f89a62917) is the correct approach for active development code. No changes are required.

Likely an incorrect or invalid review comment.

.gitmodules (1)

50-55: LGTM! Standard submodule additions.

The two new Uniswap V4 submodules are correctly configured with official repository URLs and follow the same format as existing submodules in the project.

src/interfaces/UniswapV4.sol (1)

9-21: Interface looks complete for the use case.

The IPositionManagerLike interface provides the necessary functions for Uniswap V4 position management:

modifyLiquidities for updating positions

getPoolAndPositionInfo for querying position details

poolKeys for pool lookup

ownerOf for ownership validation

lib/uniswap-v4-core (1)

1-1: Pinned Uniswap v4 core via submodule commit; verify provenance and review upstream diff

This file only pins the uniswap-v4-core submodule to commit e50237c4…. Locally this is fine, but security/behavior now depends entirely on that upstream commit. Please ensure:

The commit is from the expected official Uniswap repository and tag/branch.

You’ve reviewed the upstream diff between the prior pinned commit (if any) and e50237c4… for breaking changes or security-relevant behavior.

src/interfaces/Common.sol (1)

1-18: LGTM!

The interfaces are minimal, focused, and correctly defined for their intended use in UniswapV4Lib.sol. The function signatures align with standard ERC20 and Permit2 patterns.

src/libraries/UniswapV4Lib.sol (10)

16-30: LGTM!

The library structure, TickLimits struct, and constants are well-defined. The hard-coded mainnet addresses align with Uniswap V4 official deployments per the documentation comment.

35-73: LGTM!

The mintPosition function correctly validates tick limits before minting, constructs appropriate calldata, and delegates to _increaseLiquidity for execution and rate-limit accounting. The proxy is set as the NFT recipient in _getMintCalldata.

75-114: LGTM!

The increasePosition function correctly enforces ownership validation (Lines 87-90) to ensure the proxy retains ownership of the increased liquidity. The poolId match check (Line 92) prevents cross-pool misconfigurations.

116-153: LGTM!

The decreasePosition function intentionally omits ownership checks as documented in the comment (Lines 127-129). Per the design decision, decreasing another account's position routes underlying tokens to the proxy, which benefits the system. The poolId validation remains as the primary guard.

Based on learnings.

159-202: LGTM!

The _approveWithPermit2 function implements a robust approval pattern:

Best-effort zeroing of existing allowance (Lines 167-178) with documented rationale for ignoring failures.

Mandatory approval with return-value handling for both standard and non-compliant ERC20s (Lines 180-191).

Transient Permit2 approval via expiration = uint48(block.timestamp) limiting the permit to the current block (Line 199).

204-253: LGTM!

The _increaseLiquidity function correctly:

Approves tokens before operation and cleans up after (Lines 216-217, 251-252).

Uses balance deltas for accurate accounting (Lines 220-228).

Applies clamped subtraction for edge cases (Lines 234-239).

Documents the token valuation assumption (Lines 232-233).

The rate-limit accounting sums normalized balances rather than USD values, which is a known design decision per learnings.

255-289: LGTM!

The _decreaseLiquidity function correctly handles the withdrawal flow:

No Permit2 approval needed since tokens flow inward.

Balance delta accounting captures actual token increases.

Direct subtraction (Lines 281-282) is safe under the standard V4 flow where endingBalance >= startingBalance.

The token valuation assumption is documented (Lines 278-279), consistent with the deposit side.

295-311: LGTM!

The _checkTickLimits function correctly validates:

Configuration is set (maxTickSpacing != 0).

Ticks are ordered (tickLower < tickUpper - strict inequality is correct for Uniswap V4).

Ticks within admin-defined bounds.

Position width within configured maximum.

The _clampedSub helper is a clean implementation for safe subtraction.

317-432: LGTM!

The calldata builder functions correctly encode Uniswap V4 action sequences:

_getMintCalldata: MINT_POSITION + SETTLE_PAIR

_getIncreaseLiquidityCallData: INCREASE_LIQUIDITY + CLOSE_CURRENCY × 2

_getDecreaseLiquidityCallData: DECREASE_LIQUIDITY + TAKE_PAIR

The _getNormalizedBalance function correctly handles 6-decimal tokens (USDC/USDT). The decimals <= 18 assumption is appropriate for the intended stable pairs.

434-446: LGTM!

The _getPoolKey and _requirePoolIdMatch functions correctly interface with Uniswap V4's internal encoding:

bytes25 cast mirrors the Position Manager's internal mapping key format.

keccak256(abi.encode(poolKey)) comparison validates pool membership correctly.

src/MainnetController.sol (7)

133-138: LGTM!

The UniswapV4TickLimitsSet event is well-structured with poolId indexed for efficient event filtering. The event parameters capture all relevant tick limit configuration.

163-164: LGTM!

The rate-limit identifiers are correctly bound to UniswapV4Lib constants, ensuring consistency between the controller and library. The uniswapV4TickLimits mapping provides per-pool tick configuration.

Also applies to: 206-208

322-345: LGTM!

The setUniswapV4TickLimits function correctly:

Enforces admin-only access with nonReentrant.

Validates either all-zero (disabled) or valid tick configuration (maxTickSpacing > 0 and tickLowerMin < tickUpperMax).

Setting (0, 0, 0) effectively disables new mints while allowing existing positions to be managed.

642-665: LGTM!

The mintPositionUniswapV4 function correctly gates access with RELAYER role and nonReentrant, then delegates to UniswapV4Lib.mintPosition which handles tick validation, Permit2 approvals, and rate-limit accounting.

667-687: LGTM!

The increaseLiquidityUniswapV4 function correctly enforces RELAYER role and nonReentrant modifier. The library function handles ownership validation and rate-limit accounting.

689-709: LGTM!

The decreaseLiquidityUniswapV4 function correctly enforces RELAYER role and nonReentrant modifier. The intentional omission of ownership validation in the library is documented and aligns with the design decision.

926-927: LGTM!

The destructuring adjustment correctly extracts only the needed OFTReceipt from the quoteOFT return values.

test/mainnet-fork/Uniswapv4.t.sol (9)

26-35: LGTM!

The local IERC20Like interface intentionally omits the bool return on approve (as documented in the comment at Line 28) to accommodate non-conformant tokens during testing. This is a pragmatic choice for test flexibility.

131-419: LGTM!

Comprehensive test coverage for mintPositionUniswapV4:

Access control and reentrancy tests.

Boundary condition tests for tick limits (misordered, too low, too high, too wide) with both failure and success assertions.

Slippage protection tests for both tokens.

Rate-limit boundary testing.

Happy path with reentrancy guard state verification.

421-597: LGTM!

Thorough test coverage for increaseLiquidityUniswapV4:

Ownership validation test (Line 456-471) correctly verifies proxy must own the position.

PoolId mismatch test verifies cross-pool protection.

Slippage and rate-limit boundary tests included.

599-784: LGTM!

Complete test coverage for decreaseLiquidityUniswapV4:

Correctly omits ownership test (intentional design per learnings).

Min amount slippage tests verify SlippageCheck.MinimumAmountInsufficient revert.

Both partial (50%) and full decrease scenarios are tested with rate-limit verification.

825-841: LGTM!

The setUniswapV4TickLimits happy path test correctly verifies event emission and state updates.

847-910: LGTM!

The test_uniswapV4_story1 provides excellent end-to-end coverage:

Full lifecycle: mint → increase → decrease → close.

Time warps (90 days) and external swaps simulate realistic market conditions.

Rate-limit accounting verified at each step.

Clear NatSpec documentation of the scenario.

912-1523: LGTM!

Comprehensive adversarial test coverage:

Baseline scenarios establish expected behavior at different price positions.

Attack scenarios (front-run/back-run patterns) quantify MEV/sandwich attack impact.

Defense tests (*_defended) demonstrate tick-width limits and slippage caps effectively blocking attack vectors.

Wide-tick scenarios illustrate the increased risk of broader ranges.

These tests serve as both validation and documentation of the economic security model.

1561-1611: LGTM!

The _mintPosition helper includes comprehensive assertions:

Token amounts within limits (Lines 1600-1601).

Rate-limit accounting verification (Line 1603).

Position ownership and liquidity checks (Lines 1605-1607).

Zero-allowance verification for security (Lines 1609-1610).

This pattern is consistently applied across all position operation helpers.

1926-1969: LGTM!

The _externalSwap helper correctly implements external swap functionality via the Universal Router:

Proper action encoding (SWAP_EXACT_IN_SINGLE, SETTLE_ALL, TAKE_ALL).

Correct Permit2 approval flow.

Returns actual amount out from balance delta.

coderabbitai · 2025-11-25T05:21:05Z

foundry.toml

    '@layerzerolabs/lz-evm-messagelib-v2/=lib/layerzero-v2/packages/layerzero-v2/evm/messagelib/',
    'solidity-bytes-utils/=lib/solidity-bytes-utils/',
    'forge-std/=lib/forge-std/src/',
+    '@uniswap/v4-core/=lib/uniswap-v4-core/'


🛠️ Refactor suggestion | 🟠 Major

Add v4-periphery remapping for consistency.

Only the v4-core remapping is added, but the codebase also imports from v4-periphery (see src/interfaces/UniswapV4.sol line 7). To support the suggested remapped import pattern and maintain consistency, add a remapping for v4-periphery as well.

Apply this diff to add the missing remapping:

'@uniswap/v4-core/=lib/uniswap-v4-core/' + '@uniswap/v4-periphery/=lib/uniswap-v4-periphery/' ]

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

'@uniswap/v4-core/=lib/uniswap-v4-core/'

'@uniswap/v4-core/=lib/uniswap-v4-core/'

'@uniswap/v4-periphery/=lib/uniswap-v4-periphery/'

]

🤖 Prompt for AI Agents

In foundry.toml around line 21, the file only contains a remapping for '@uniswap/v4-core/=lib/uniswap-v4-core/' while imports also reference v4-periphery; add a parallel remapping entry for '@uniswap/v4-periphery/=lib/uniswap-v4-periphery/' so the periphery imports resolve correctly, placing it alongside the existing remapping and ensuring whitespace/format matches existing remapping style.

coderabbitai · 2025-11-25T05:21:05Z

src/interfaces/UniswapV4.sol

+import { PoolId }  from "../../lib/uniswap-v4-core/src/types/PoolId.sol";
+import { PoolKey } from "../../lib/uniswap-v4-core/src/types/PoolKey.sol";
+
+import { PositionInfo } from "../../lib/uniswap-v4-periphery/src/libraries/PositionInfoLibrary.sol";


⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

Missing v4-periphery remapping in foundry.toml.

The past review comment suggests using remapped imports including @uniswap/v4-periphery/, but foundry.toml only adds the @uniswap/v4-core/ remapping. To enable the suggested refactor, add the v4-periphery remapping to foundry.toml.

Add this remapping to foundry.toml (in the remappings array):

'@uniswap/v4-periphery/=lib/uniswap-v4-periphery/'

--- 🏁 Script executed: ```shell #!/bin/bash # Find and examine foundry.toml to check remappings find . -name "foundry.toml" -type f -exec cat {} \;

Length of output: 1445

🏁 Script executed:

#!/bin/bash # Search for v4-periphery imports in the codebase rg "v4-periphery" -n -B2 -A2

Length of output: 1988

🏁 Script executed:

#!/bin/bash # Specifically check the imports in src/interfaces/UniswapV4.sol cat src/interfaces/UniswapV4.sol

Length of output: 754

Add missing v4-periphery remapping to foundry.toml for import consistency.

The codebase uses v4-periphery imports in src/interfaces/UniswapV4.sol, src/libraries/UniswapV4Lib.sol, and test files, but all use relative paths (../../lib/uniswap-v4-periphery/...) while v4-core already has a remapping. Add this to the remappings array in foundry.toml:

'@uniswap/v4-periphery/=lib/uniswap-v4-periphery/'

This enables converting the three files' v4-periphery imports to use the remapped path pattern for consistency.

🤖 Prompt for AI Agents

In src/interfaces/UniswapV4.sol lines 4-7, the imports use a relative path to v4-periphery while v4-core already uses a remapping; add the remapping entry '@uniswap/v4-periphery/=lib/uniswap-v4-periphery/' to the remappings array in foundry.toml and then update the v4-periphery imports in this file (and the other occurrences in src/libraries/UniswapV4Lib.sol and relevant test files) to use the remapped import pattern @uniswap/v4-periphery/... instead of ../../lib/uniswap-v4-periphery/ to ensure consistent import resolution.

coderabbitai · 2025-11-25T05:21:05Z

src/RateLimitHelpers.sol

 library RateLimitHelpers {

-    function makeAddressKey(bytes32 key, address asset) internal pure returns (bytes32) {
-        return keccak256(abi.encode(key, asset));
+    function makeAddressKey(bytes32 key, address a) internal pure returns (bytes32) {
+        return keccak256(abi.encode(key, a));
    }

-    function makeAddressAddressKey(bytes32 key, address asset, address destination) internal pure returns (bytes32) {
-        return keccak256(abi.encode(key, asset, destination));
+    function makeAddressAddressKey(bytes32 key, address a, address b) internal pure returns (bytes32) {
+        return keccak256(abi.encode(key, a, b));
    }

-    function makeUint32Key(bytes32 key, uint32 domain) internal pure returns (bytes32) {
-        return keccak256(abi.encode(key, domain));
+    function makeBytes32Key(bytes32 key, bytes32 a) internal pure returns (bytes32) {
+        return keccak256(abi.encode(key, a));
+    }
+
+    function makeUint32Key(bytes32 key, uint32 a) internal pure returns (bytes32) {
+        return keccak256(abi.encode(key, a));
    }

 }


🧹 Nitpick | 🔵 Trivial

Consider adding library-level documentation.

While the implementations are straightforward, a brief comment block explaining that these helpers generate collision-resistant rate-limit keys via keccak256(abi.encode(...)) would improve maintainability.

Example:

/// @notice Internal helpers for building collision-resistant rate-limit keys /// @dev Each helper encodes a base key with typed parameters using abi.encode /// and hashes the result to produce a unique 32-byte identifier library RateLimitHelpers {

🤖 Prompt for AI Agents

In src/RateLimitHelpers.sol around lines 4 to 22, add a short library-level NatSpec comment block above the library declaration that explains these are internal helpers that build collision-resistant rate-limit keys by encoding inputs with abi.encode and hashing with keccak256; include @notice and @dev tags mentioning that each helper combines a base key with typed parameters and returns a 32-byte identifier to improve maintainability and intent clarity.

deluca-mike requested review from lucas-manuel and supercontracts November 10, 2025 15:25

deluca-mike self-assigned this Nov 10, 2025

deluca-mike added Priority: Medium Status: Ready for Review labels Nov 10, 2025

deluca-mike force-pushed the sc-1233-erc4626-lib branch from 7a87195 to 0b598fe Compare November 10, 2025 17:39

deluca-mike force-pushed the sc-1234-uniswap-v4-support branch from abe98f0 to 48e1b57 Compare November 10, 2025 17:40

Base automatically changed from sc-1233-erc4626-lib to dev November 10, 2025 18:12

deluca-mike force-pushed the sc-1234-uniswap-v4-support branch from 48e1b57 to 0122e2c Compare November 11, 2025 19:43

coderabbitai bot requested changes Nov 11, 2025

View reviewed changes

lib/uniswap-v4-periphery Outdated Show resolved Hide resolved

src/MainnetController.sol Show resolved Hide resolved

lucas-manuel force-pushed the dev branch from 1754ec8 to 2aed93a Compare November 18, 2025 12:47

deluca-mike force-pushed the sc-1234-uniswap-v4-support branch from 0122e2c to 6c26dfc Compare November 20, 2025 06:30

coderabbitai bot requested changes Nov 20, 2025

View reviewed changes

lib/uniswap-v4-periphery Show resolved Hide resolved

src/MainnetController.sol Outdated Show resolved Hide resolved

src/MainnetController.sol Show resolved Hide resolved

test/mainnet-fork/Uniswapv4.t.sol Show resolved Hide resolved

coderabbitai bot requested changes Nov 20, 2025

View reviewed changes

src/libraries/UniswapV4Lib.sol Show resolved Hide resolved

src/MainnetController.sol Show resolved Hide resolved

coderabbitai bot requested changes Nov 20, 2025

View reviewed changes

test/mainnet-fork/Uniswapv4.t.sol Show resolved Hide resolved

supercontracts reviewed Nov 24, 2025

View reviewed changes

test/mainnet-fork/Uniswapv4.t.sol Show resolved Hide resolved

lucas-manuel requested changes Nov 24, 2025

View reviewed changes

coderabbitai bot requested changes Nov 24, 2025

View reviewed changes

src/libraries/UniswapV4Lib.sol Outdated Show resolved Hide resolved

src/libraries/UniswapV4Lib.sol Show resolved Hide resolved

lucas-manuel requested changes Nov 24, 2025

View reviewed changes

deluca-mike changed the title ~~feat: UniswapV4 support~~ feat: UniswapV4 mint/increase/decrease liquidity support Nov 25, 2025

deluca-mike force-pushed the sc-1234-uniswap-v4-support branch from 949f62f to 9ca1d11 Compare November 25, 2025 04:58

coderabbitai bot requested changes Nov 25, 2025

View reviewed changes

feat: UniswapV4 mint/increase/decrease liquidity support (SC-1234)

6c06cbb

deluca-mike force-pushed the sc-1234-uniswap-v4-support branch from 9ca1d11 to 6c06cbb Compare November 25, 2025 05:16

deluca-mike requested review from lucas-manuel and supercontracts November 25, 2025 05:17

coderabbitai bot requested changes Nov 25, 2025

View reviewed changes


		mainnetController.swapUniswapV4(_POOL_ID, address(usdc), 1_000_000e6, 999_281e6);
		}

feat: UniswapV4 mint/increase/decrease liquidity support #189

Are you sure you want to change the base?

feat: UniswapV4 mint/increase/decrease liquidity support #189

Conversation

deluca-mike commented Nov 10, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Nov 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

Suggested reviewers

Poem

Pre-merge checks and finishing touches

Uh oh!

lucas-manuel commented Nov 11, 2025

Uh oh!

coderabbitai bot commented Nov 11, 2025

🔴 CRITICAL SECURITY ISSUES

1. Missing Ownership Validation in burnPosition and decreasePosition

2. PoolId to Address Casting Creates Collision Risk

3. bytes32 to bytes25 Casting Truncates Data

🟠 HIGH SEVERITY ISSUES

4. TODO Comments in Production Code

5. Unconventional Slippage Calculation

🟡 MEDIUM SEVERITY ISSUES

6. Permit2 Approval Pattern Uses Low-Level Call

7. Rate Limit Based on Normalized Balance

How This Interacts with UniswapV4

Recommendations Priority

Uh oh!

deluca-mike commented Nov 11, 2025

Uh oh!

coderabbitai bot commented Nov 11, 2025

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lucas-manuel Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

deluca-mike commented Nov 10, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Nov 10, 2025 •

edited

Loading

1. Missing Ownership Validation in `burnPosition` and `decreasePosition`