feat: Add v1.9.0 release #214
Conversation
* feat: AAVE external lib and Approve internal lib - cuts down on much needed contract size for future integrations * fix: PR Review
* feat: UniswapV4 mint/increase/decrease liquidity support (SC-1234) * fix: PR review * fix: literal amount assertions * fix: PR Review
* feat: UniswapV4 swap support (SC-1249) * fix: PR review * fix: PR Review 2 * fix: build workflow
* feat: AAVE external lib and Approve internal lib (SC-1182) (#162) * feat: AAVE external lib and Approve internal lib - cuts down on much needed contract size for future integrations * fix: PR Review * feat: Add ERC4626 lib (SC-1233) (#188) * add * fix: Use relative paths (#191) --------- Co-authored-by: Michael De Luca <michael.deluca@circle-free.com> Co-authored-by: Lucas Manuel <lucasmanuel.tech@gmail.com>
* UniswapV4TestBase * use UniswapV4TestBase * refactor for generics * USDT/USDS pool tests
* test: UniswapV4 fuzz tests (SC-1250) * format and usdt-usds fuzz tests * fix: fuzz runs instructions --------- Co-authored-by: Michael De Luca <michael.deluca@circle-free.com>
* feat: Apply Tick Checks on `increaseLiquidityUniswapV4` (SC-1262) * fix: PR Review * fix: PR Review * fix: PR Review * fix: remove extra line
* feat: UniswapV4 swap invalid tokenIn clear error (SC-1259) * fix: PR review
…C-1277) (#200) * feat: rm warning and add check * fix: explicit casting * fix: update maxAmount test * fix: new lines
* save * add: modifiers
Co-authored-by: Lucas Manuel <lucasmanuel.tech@gmail.com>
…Position (#204) Co-authored-by: Lucas Manuel <lucasmanuel.tech@gmail.com>
Co-authored-by: Lucas Manuel <lucasmanuel.tech@gmail.com>
Co-authored-by: supercontracts <bonjour.tabish@gmail.com>
…pproveWithPermit2 ` (#207) * fix: check amount for permit2 C13 * fix: Pr Review * fix --------- Co-authored-by: Michael De Luca <michael.deluca@circle-free.com>
* feat: add additional requirements/assumptions * fix: typo --------- Co-authored-by: Tabish Shaikh <bonjour.tabish@gmail.com>
WalkthroughThis PR integrates Uniswap V4 support via submodules, introduces new library abstractions (ApproveLib, ERC4626Lib, AaveLib, UniswapV4Lib), refactors approval and rate-limiting patterns, removes Morpho allocation functions, updates deployment configs, and adds recipient validation for cross-chain transfers. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MainnetController
participant UniswapV4Lib
participant RateLimitHelpers
participant ALMProxy
participant PositionManager
User->>MainnetController: mintPositionUniswapV4(poolId, tickLower, tickUpper, liquidity, ...)
MainnetController->>UniswapV4Lib: mintPosition(proxy, rateLimits, poolId, ...)
rect rgb(200, 220, 255)
Note over UniswapV4Lib: Validation Phase
UniswapV4Lib->>UniswapV4Lib: _checkTickLimits(tickLower, tickUpper, ...)
UniswapV4Lib->>UniswapV4Lib: _getPoolKeyFromPoolId(poolId)
end
rect rgb(200, 255, 220)
Note over UniswapV4Lib: Rate Limit & Approval Phase
UniswapV4Lib->>RateLimitHelpers: makeAddressKey(LIMIT_DEPOSIT, token)
RateLimitHelpers-->>UniswapV4Lib: rateLimitKey
UniswapV4Lib->>RateLimitHelpers: decreaseRateLimit(rateLimitKey, amount)
UniswapV4Lib->>UniswapV4Lib: _approveWithPermit2(token0, token1, proxy)
end
rect rgb(255, 220, 200)
Note over UniswapV4Lib,ALMProxy: Execution Phase
UniswapV4Lib->>UniswapV4Lib: _getMintCalldata(poolKey, tickLower, tickUpper, liquidity, ...)
UniswapV4Lib->>ALMProxy: doCall(PositionManager, mintCalldata)
ALMProxy->>PositionManager: modifyLiquidities(unlockData, deadline)
PositionManager-->>ALMProxy: success
ALMProxy-->>UniswapV4Lib: encoded tokenId
end
UniswapV4Lib-->>MainnetController: tokenId
MainnetController-->>User: tokenId
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
|
Coverage after merging dev into master will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI Agents
In @README.md:
- Line 150: Add programmatic validation in setUniswapV4TickLimits to enforce the
README's 1:1 asset requirement: before accepting tick limits, verify token0 and
token1 represent equal-valued assets (e.g., compare their canonical asset IDs or
use the same oracle/price feed to ensure identical USD value per normalized
unit) and revert with a clear error if they differ; update any callers/tests
accordingly and remove the reliance on the comment in _increaseLiquidity by
making this guarantee upfront in setUniswapV4TickLimits.
In @script/input/1/arbitrum_one-production.json:
- Around line 9-11: Reorder the JSON object fields so they match the ordering
used in other production configs: place "rateLimits" before "almProxy" (keeping
"susds" as-is); update the entry that currently lists "almProxy" then
"rateLimits" to instead list "rateLimits" then "almProxy" for consistency across
configs.
In @src/libraries/CCTPLib.sol:
- Line 4: The import in CCTPLib.sol uses a relative path to OpenZeppelin instead
of the project remapping; update the import line that references IERC20 to use
the remapped path "@openzeppelin/contracts/token/ERC20/IERC20.sol" (replace the
current "../../lib/openzeppelin-contracts/..." import) so the IERC20 import uses
the configured foundry remapping for consistency and maintainability.
In @src/libraries/PSMLib.sol:
- Line 4: Replace the relative OpenZeppelin import with the remapped path:
update the import in PSMLib.sol that currently references
../../lib/openzeppelin-contracts/... to use
@openzeppelin/contracts/token/ERC20/IERC20.sol so the IERC20 symbol is imported
via the configured Foundry remapping for consistency and future-proofing.
In @src/libraries/UniswapV4Lib.sol:
- Around line 253-298: In _approveWithPermit2, replace the unchecked
proxy.call(...) reset with an explicit (bool success, bytes memory data) =
proxy.call(...) and, if success is false, emit a non-reverting event (e.g.,
Permit2ResetFailed(address proxy, address token, bytes data)) so failures (like
out-of-gas or unexpected reverts) are recorded for debugging while preserving
the original non-fatal behavior; add the event declaration and emit it when
success==false, but do not change the current non-revert semantics that the
comment intends.
- Around line 30-35: The three addresses _PERMIT2, _POSITION_MANAGER, and
_ROUTER are hardcoded to mainnet, which prevents multi-chain deployment; replace
these internal constant addresses with configurable values (e.g.,
constructor/initializer-set immutable variables or function parameters) or route
lookups through a registry contract so the library can be reused across
networks, and if you keep them constant add a clear comment that UniswapV4Lib is
mainnet-only. Update all usages of _PERMIT2, _POSITION_MANAGER, and _ROUTER to
read the new configurable variables or registry lookups.
- Around line 349-382: In _decreaseLiquidity, avoid underflow when computing
endingBalance0 - startingBalance0 and endingBalance1 - startingBalance1 by first
computing deltas safely: for each token compute delta as ending >= starting ?
ending - starting : 0 (or revert with a clear require if you prefer
failing-fast); then pass those non-negative deltas to _getNormalizedBalance and
use the sum for rateLimitDecrease before calling
IRateLimits.triggerRateLimitDecrease on the RateLimit key. Ensure the fix is
applied to the function _decreaseLiquidity and update the inline comment to
reflect the defensive check.
📜 Review details
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
audits/v190-cantina-audit.pdfis excluded by!**/*.pdfaudits/v190-certora-audit.pdfis excluded by!**/*.pdf
📒 Files selected for processing (29)
.github/workflows/ci.yml.gitmodulesREADME.mdfoundry.tomllib/uniswap-v4-corelib/uniswap-v4-peripheryscript/input/1/arbitrum_one-production.jsonscript/input/1/avalanche-production.jsonscript/input/1/optimism-production.jsonscript/input/1/unichain-production.jsonsrc/ForeignController.solsrc/MainnetController.solsrc/RateLimitHelpers.solsrc/interfaces/Common.solsrc/interfaces/UniswapV4.solsrc/libraries/AaveLib.solsrc/libraries/ApproveLib.solsrc/libraries/CCTPLib.solsrc/libraries/CurveLib.solsrc/libraries/ERC4626Lib.solsrc/libraries/PSMLib.solsrc/libraries/UniswapV4Lib.soltest/base-fork/MorphoAllocations.t.soltest/mainnet-fork/Approve.t.soltest/mainnet-fork/Curve.t.soltest/mainnet-fork/LayerZero.t.soltest/mainnet-fork/OTCSwaps.t.soltest/mainnet-fork/Uniswapv4.t.soltest/unit/controllers/Admin.t.sol
💤 Files with no reviewable changes (3)
- test/mainnet-fork/Curve.t.sol
- test/mainnet-fork/OTCSwaps.t.sol
- test/base-fork/MorphoAllocations.t.sol
🧰 Additional context used
🧠 Learnings (6)
📚 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/Approve.t.soltest/mainnet-fork/LayerZero.t.soltest/unit/controllers/Admin.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/Approve.t.soltest/mainnet-fork/LayerZero.t.soltest/unit/controllers/Admin.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/Approve.t.solsrc/libraries/ApproveLib.solsrc/interfaces/UniswapV4.solsrc/libraries/UniswapV4Lib.solsrc/libraries/CurveLib.solsrc/MainnetController.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/Approve.t.soltest/mainnet-fork/LayerZero.t.solsrc/MainnetController.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/LayerZero.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:
test/mainnet-fork/LayerZero.t.solREADME.mdsrc/libraries/ERC4626Lib.solsrc/libraries/UniswapV4Lib.solsrc/MainnetController.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 (64)
.github/workflows/ci.yml (1)
25-25: Build command change is correct.The modification from
--skip testto explicitly specifying./srcis cleaner and more maintainable. All production contracts reside exclusively in thesrc/directory as configured infoundry.toml, and no source files exist outside this directory that need to be built. Deployment and script files are properly separated into their own directories. Submodule dependencies are correctly configured via remappings and will resolve as needed when referenced fromsrc/contracts.script/input/1/arbitrum_one-production.json (1)
9-11: Verify the production addresses for Arbitrum One before merging.The new
almProxy(0x92afd6F2385a90e44da3a8B60fe36f6cBe1D8709) andrateLimits(0x19D08879851FB54C2dCc4bb32b5a1EA5E9Ad6838) addresses must be validated against the Arbitrum One network and cross-referenced within the project to ensure correctness.script/input/1/unichain-production.json (1)
9-11: Verify the production addresses for Unichain.The new
rateLimitsandalmProxyaddresses are being added to the production configuration. Given the "Priority: High" label, ensure these addresses are correct for the Unichain network before merging.script/input/1/optimism-production.json (1)
9-11: Verify these new Optimism production addresses against official sources.The
rateLimits(0x6B34A6B84444dC3Fc692821D5d077a1e4927342d) andalmProxy(0x876664f0c9Ff24D1aa355Ce9f1680AE1A5bf36fB) addresses are new to the codebase with no internal references to cross-check. Confirm these addresses are correct for the Optimism network by validating them against official Optimism contract documentation before merging.script/input/1/avalanche-production.json (1)
7-9: Verify the production addresses for Avalanche with the deployment team.The new
rateLimitsandalmProxyaddresses are being added to the production configuration. While both contract names are documented in the README as legitimate components of the ALM architecture, the actual addresses (0xb79972e8B21f0dE911E65AC342ac85ad38C9A77a for rateLimits and 0xecE6B0E8a54c2f44e066fBb9234e7157B15b7FeC for almProxy) must be confirmed with the team that deployed these contracts on Avalanche before merging.lib/uniswap-v4-periphery (1)
1-1: The submodule commit is valid and properly configured. Verification confirms commit3779387e5d296f39df543d23524b050f89a62917exists in the Uniswap/v4-periphery repository, is reachable from the main branch, and has legitimate authorship (Daniel Gretzke, "Add per hop slippage checks (#497)"). No compatibility or security concerns identified.lib/uniswap-v4-core (1)
1-1: The submodule commite50237c43811bd9b526eff40f26772152a42dabais correctly pinned to thev4.0.0tagged release of the Uniswap V4-Core repository, ensuring proper reproducibility and stability for this production release.foundry.toml (1)
21-21: LGTM!The Uniswap V4 core remapping is correctly formatted and consistent with other remappings in the configuration. This properly resolves the @uniswap/v4-core import prefix to the submodule location.
README.md (1)
143-143: Verify that Ethena's external validation safeguards are sufficient.This trust assumption introduces a dependency on Ethena's external Order Validity Checks to prevent a compromised RELAYER from setting a malicious delegated signer. Ensure that:
- Ethena's validation mechanisms are well understood and documented
- The risk of relying on external validation is acceptable to governance
- There are fallback mechanisms if Ethena's checks fail or are bypassed
Do you want to add additional smart contract-level constraints to limit the risk, or document the specific Ethena validation rules that are relied upon?
src/interfaces/Common.sol (1)
1-18: LGTM! Clean minimal interface definitions.These interfaces provide focused abstractions for ERC20-like and Permit2-like interactions. The intentionally minimal function sets (e.g., IERC20Like with only
approve,balanceOf, anddecimals) are appropriate for the specific use cases in the codebase, avoiding unnecessary dependencies on full interface definitions. The "Like" suffix naming convention clearly indicates these are simplified interfaces.src/libraries/AaveLib.sol (1)
22-57: LGTM! Deposit logic is sound.The function correctly:
- Rate-limits before the deposit operation
- Validates slippage parameters
- Tracks aToken balance changes to measure actual tokens received
- Enforces slippage bounds using the standard maxSlippage multiplier pattern (where 1e18 = 100%)
.gitmodules (1)
50-55: LGTM! Uniswap V4 submodule additions.The submodule configuration for
uniswap-v4-peripheryanduniswap-v4-coreis correct and references the official Uniswap repositories.test/mainnet-fork/Approve.t.sol (1)
9-10: LGTM! Test refactoring to use ApproveLib.The test file correctly adopts
ApproveLib.approvein place of the previous internal_approvecalls, and the expected revert message now matches ApproveLib's"MC/approve-failed"error string.Also applies to: 64-69, 269-269
src/libraries/ApproveLib.sol (1)
11-35: LGTM! USDT-compatible approval implementation.The function correctly handles non-standard ERC20 tokens (like USDT) that require resetting allowance to zero before setting a new value:
- Lines 15-16: Uses low-level
callto attempt the approval without reverting on failure- Lines 20-26: Checks if the approve succeeded (empty return data or
true)- Lines 29-31: If the first attempt failed, resets to zero and retries using typed interface calls that revert on failure
- Line 34: Final validation ensures the approval succeeded
The mixing of low-level and typed calls is intentional—the first call must not revert to handle tokens that fail on non-zero allowances, while subsequent calls should revert to catch genuine errors.
src/libraries/CurveLib.sol (1)
4-4: LGTM! Refactoring to use ApproveLib.The file correctly adopts
ApproveLib.approvefor token approvals in bothswapandaddLiquidityfunctions. The import path update for IERC20 and the addition of theaddress(params.proxy)cast are appropriate for the new library-based approval pattern.Also applies to: 9-10, 118-123, 159-164
src/RateLimitHelpers.sol (4)
6-8: LGTM!The generic parameter name
aimproves reusability across different use cases beyond just assets.
10-12: LGTM!The generic parameter names improve reusability for various address-pair key combinations.
14-16: LGTM!This new helper provides support for bytes32-based key generation, maintaining consistency with the existing pattern.
18-20: LGTM!The generic parameter name improves reusability beyond just domain identifiers.
src/interfaces/UniswapV4.sol (3)
4-7: LGTM!The imports correctly reference Uniswap V4 core and periphery types from the submodules.
9-21: LGTM!The interface correctly defines the Uniswap V4 Position Manager functions needed for liquidity management operations.
23-27: LGTM!The interface correctly defines the Universal Router execute function for command-based operations.
test/mainnet-fork/LayerZero.t.sol (4)
63-86: LGTM!The addition of recipient setup (lines 76-79) ensures this test properly focuses on the zero-maxAmount error without hitting the new recipient-not-set validation.
141-182: LGTM!This test correctly validates the new recipient-not-set safety check, ensuring LayerZero transfers cannot proceed without a configured recipient.
427-450: LGTM!The addition of recipient setup (lines 440-443) ensures this test properly focuses on the zero-maxAmount error, consistent with the mainnet controller test.
508-549: LGTM!This test correctly validates the new recipient-not-set safety check for the foreign controller, mirroring the mainnet controller test.
test/unit/controllers/Admin.t.sol (6)
4-4: LGTM!The IAccessControl import is correctly added to support access control error checking in the new tests.
444-446: LGTM!The test constants are properly defined for the Uniswap V4 tick limits test scenarios.
448-452: LGTM!The reentrancy test correctly validates that the function is protected against reentrancy attacks.
454-465: LGTM!The access control test correctly validates that only admins can set Uniswap V4 tick limits.
467-481: LGTM!The test correctly validates tick parameter constraints: lower must be less than upper, and maxTickSpacing can only be zero if all parameters are zero.
483-499: LGTM!The success test correctly validates event emission, reentrancy guard behavior, and state updates for setting Uniswap V4 tick limits.
src/libraries/ERC4626Lib.sol (5)
15-15: LGTM!The exchange rate precision constant is appropriately set for high-precision calculations.
17-43: LGTM!The deposit function correctly implements rate-limited ERC4626 deposits with exchange rate validation. The flow properly approves the underlying asset, executes the deposit via the proxy, and enforces the maximum exchange rate constraint.
45-72: LGTM!The withdraw function correctly implements bidirectional rate limiting: it decreases the withdraw limit (consuming capacity) and increases the deposit limit (freeing capacity). The ERC4626 withdraw call is properly structured.
74-101: LGTM!The redeem function correctly implements bidirectional rate limiting based on the actual assets received. The rate limit adjustments properly consume withdraw capacity and free deposit capacity.
103-111: No overflow protection needed—Solidity 0.8.21 automatically reverts on arithmetic overflow by default. The calculation(EXCHANGE_RATE_PRECISION * assets) / sharesis already protected by the language and requires no additional safeguards.Likely an incorrect or invalid review comment.
src/ForeignController.sol (3)
163-172: LGTM!The
setMaxExchangeRatefunction properly uses theonlyRole(DEFAULT_ADMIN_ROLE)modifier, which is consistent with other admin functions in this contract likesetMaxSlippage,setMintRecipient, andsetLayerZeroRecipient. The zero-address check and event emission pattern are correct.
187-205: LGTM!The
transferAssetfunction refactor using therateLimitedmodifier is cleaner than an inline call. The rate limit key construction usingRateLimitHelpers.makeAddressAddressKeycorrectly combines asset and destination for granular control.
295-343: Good security improvement with recipient validation.The addition of explicit recipient lookup and validation (lines 309-311) before use in
SendParamis a solid security enhancement. This prevents transfers to unconfigured destinations that would otherwise result in lost funds.src/MainnetController.sol (16)
91-97: LGTM!Moving the
OTCstruct declaration inside the contract is a reasonable organizational change that keeps the data structure close to its usage.
133-138: LGTM!The
UniswapV4TickLimitsSetevent properly captures all relevant parameters for tracking tick limit configuration changes, withpoolIdcorrectly indexed for efficient filtering.
163-165: LGTM!Using constants from
UniswapV4Libensures consistency between the controller and library. TheuniswapV4TickLimitsmapping provides per-pool tick configuration as expected.Also applies to: 207-209
310-321: LGTM!Delegating exchange rate calculation to
ERC4626Lib.getExchangeRatecentralizes this logic and ensures consistency across the codebase.
323-346: Verify the tick limit validation allows intended edge cases.The validation logic on lines 333-337 allows either:
- All zeros (to clear/disable limits), OR
maxTickSpacing > 0ANDtickLowerMin < tickUpperMaxThis means
tickLowerMin == tickUpperMaxwithmaxTickSpacing > 0is rejected, which is correct since no valid range could exist. However, confirm that negative tick values are intentionally supported (e.g.,tickLowerMin = -100, tickUpperMax = 100), which is valid for Uniswap V4 pools centered around tick 0.
493-506: LGTM!The refactor to
ERC4626Lib.depositproperly passes all required parameters includingmaxExchangeRatefor slippage protection. Named parameters improve readability.
508-536: LGTM!Both
withdrawERC4626andredeemERC4626correctly delegate to their respective library functions with appropriate rate limit IDs for both withdraw and deposit (for the refill mechanism).
538-540: LGTM!The
EXCHANGE_RATE_PRECISIONfunction provides backward compatibility for external callers that may have relied on the previously-removed constant.
546-572: LGTM!The Aave deposit/withdraw functions are cleanly refactored to use
AaveLib, maintaining the same functionality while improving code organization.
643-666: LGTM!The
mintPositionUniswapV4function correctly passes theuniswapV4TickLimitsmapping storage reference to the library for tick validation. All parameters are properly forwarded.
668-689: LGTM!The
increaseLiquidityUniswapV4function properly includes tick limits validation through the library, which is important since positions could have been minted under different constraints or transferred to the proxy.
691-711: LGTM!The
decreaseLiquidityUniswapV4function correctly omits tick limits since decreasing liquidity doesn't require position bounds validation. Based on learnings, ownership validation is intentionally omitted as decreasing another account's position benefits the system.
760-761: LGTM!Using
ApproveLib.approvefor Ethena mint/burn preparation is consistent with the library-based approval pattern adopted throughout this PR.Also applies to: 766-767
929-931: Good security improvement with recipient validation.This mirrors the same improvement made in
ForeignController.sol. Validating the recipient is configured before proceeding prevents transfers to zero addresses.
953-953: LGTM!The
quoteOFTdestructuring with explicit spacing( , , OFTReceipt memory receipt )is consistent with the updated return shape and matches the pattern inForeignController.sol.
713-732: The casting pattern is confirmed to be intentional and correctly used.The
maxSlippagesmapping is keyed byaddress, and Uniswap V4 poolIds are designed such that their lower 160 bits represent the pool identifier. The castaddress(uint160(uint256(poolId)))is the standard way to derive the pool address for slippage lookups. This pattern is consistently used throughout the test suite (see test/mainnet-fork/Uniswapv4.t.sol) when configuring maxSlippage for V4 pools.src/libraries/UniswapV4Lib.sol (8)
41-80: LGTM!The
mintPositionfunction properly validates tick limits before minting, constructs the pool key from poolId, verifies the match, and delegates to_increaseLiquidityfor the actual operation and rate limiting.
82-127: LGTM!The
increasePositionfunction correctly:
- Validates proxy ownership of the position (line 95-98)
- Re-validates tick limits against current constraints (important for positions minted under old limits or transferred in)
- Verifies poolId matches the position's actual pool
129-164: LGTM!The
decreasePositionfunction intentionally omits ownership validation. Based on learnings, this is by design: burning/decreasing another account's position would send tokens to the proxy, which benefits the system rather than harming it.
166-247: LGTM with minor observation.The
swapfunction correctly:
- Validates maxSlippage is configured
- Validates tokenIn is one of the pool's currencies
- Applies rate limit based on normalized input amount
- Validates amountOutMin meets slippage requirements
- Resets Permit2 approval after swap
The rate limit decrease uses amountIn rather than actual amount taken (noted in comment line 190), which is a conservative approach.
300-347: LGTM!The
_increaseLiquidityfunction correctly:
- Sets up Permit2 approvals for both tokens
- Measures token balance changes to compute actual amounts used
- Uses
_clampedSubto handle edge cases where tokens might be received- Resets approvals after operation (good security practice)
The normalized balance approach for rate limiting is a known design decision per learnings.
388-400: LGTM!The
_checkTickLimitsfunction provides comprehensive validation:
- Ensures tick limits are configured (
maxTickSpacing != 0)- Validates tick ordering
- Validates ticks are within bounds
- Validates tick range doesn't exceed maximum spacing
522-526: Potential precision loss for tokens with >18 decimals.The normalization
balance * 1e18 / (10 ** decimals)will lose precision for tokens with more than 18 decimals (e.g., if decimals = 24, dividing by 10^24 then multiplying by 1e18 loses 6 decimal places of precision).Per learnings, this is a known design decision where normalized balance is used rather than USD-denominated values. Document this limitation if not already done.
544-546: Implementation is correct. The_requirePoolIdMatchfunction correctly computes poolId askeccak256(abi.encode(poolKey)), which matches Uniswap V4's standard. The PoolKey structure is imported from the official Uniswap V4 core library, ensuring proper field ordering and encoding.
| - All ERC-4626 vaults that are onboarded MUST have an initial burned shares amount that prevents rounding-based frontrunning attacks. These shares have to be unrecoverable so that they cannot be removed at a later date. | ||
| - All ERC-20 tokens are to be non-rebasing with sufficiently high decimal precision. | ||
| - Rate limits must be configured for specific ERC-4626 vaults and AAVE aTokens (vaults without rate limits set will revert). Unlimited rate limits can be used as an onboarding tool. | ||
| - All Uniswap V4 pool onboardings are to be done with 1:1 assets. |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, understand the repo structure
find . -type f -name "*.sol" | head -20Repository: sparkdotfi/spark-alm-controller
Length of output: 701
🏁 Script executed:
#!/bin/bash
# Search for Uniswap V4 onboarding/configuration functions
rg -nP -C5 '(setUniswapV4|addUniswapV4|configureUniswapV4|onboard.*[Uu]niswap.*[Vv]4)' --type=sol
# Also search more broadly for Uniswap V4 related functions
rg -nP 'UniswapV4|uniswapv4' --type=sol -iRepository: sparkdotfi/spark-alm-controller
Length of output: 130
🏁 Script executed:
#!/bin/bash
# Search for any validation logic checking asset parity, 1:1 ratios, or equal assets
rg -nP -C3 '(require.*1:1|require.*equal.*asset|require.*parity|asset.*ratio|token.*token|decimal)' --type=sol | head -100Repository: sparkdotfi/spark-alm-controller
Length of output: 102
🏁 Script executed:
#!/bin/bash
# Search for Uniswap V4 related code in all files
rg -nP '(UniswapV4|uniswapv4)' -iRepository: sparkdotfi/spark-alm-controller
Length of output: 33697
🏁 Script executed:
#!/bin/bash
# Search for 1:1 asset or equal token validation
rg -nP -C3 '(1:1|equal.*token|token.*equal|require.*asset)' | head -150Repository: sparkdotfi/spark-alm-controller
Length of output: 11041
🏁 Script executed:
#!/bin/bash
# Look at the README to see what it says about the 1:1 constraint
rg -nP -C5 '1:1.*asset|Uniswap.*V4.*onboard' -iRepository: sparkdotfi/spark-alm-controller
Length of output: 1006
🏁 Script executed:
#!/bin/bash
# Read UniswapV4Lib.sol to understand the implementation
wc -l src/libraries/UniswapV4Lib.sol
head -100 src/libraries/UniswapV4Lib.solRepository: sparkdotfi/spark-alm-controller
Length of output: 4025
🏁 Script executed:
#!/bin/bash
# Read the setUniswapV4TickLimits function
sed -n '323,350p' src/MainnetController.solRepository: sparkdotfi/spark-alm-controller
Length of output: 1129
🏁 Script executed:
#!/bin/bash
# Search for any validation that checks if assets are equal or 1:1 in Uniswap V4
rg -n 'token0|token1|asset' src/libraries/UniswapV4Lib.sol -A 3 -B 1 | head -80Repository: sparkdotfi/spark-alm-controller
Length of output: 3283
🏁 Script executed:
#!/bin/bash
# Look for _getPoolKeyFromPoolId and _requirePoolIdMatch to see if there's validation
grep -n '_getPoolKeyFromPoolId\|_requirePoolIdMatch' src/libraries/UniswapV4Lib.sol -A 5Repository: sparkdotfi/spark-alm-controller
Length of output: 1894
🏁 Script executed:
#!/bin/bash
# Search for any function that validates or registers Uniswap V4 pools
grep -n 'require.*currency\|require.*token\|pool.*register\|pool.*onboard' src/libraries/UniswapV4Lib.sol -iRepository: sparkdotfi/spark-alm-controller
Length of output: 167
🏁 Script executed:
#!/bin/bash
# Check the full implementation of critical validation functions
sed -n '450,548p' src/libraries/UniswapV4Lib.solRepository: sparkdotfi/spark-alm-controller
Length of output: 3408
🏁 Script executed:
#!/bin/bash
# Search for any pool whitelist or registration mechanism
rg -n 'whitelist.*pool\|pool.*register\|pool.*onboard' src/ -i -A 2 -B 2Repository: sparkdotfi/spark-alm-controller
Length of output: 57
🏁 Script executed:
#!/bin/bash
# Check if there are any restrictions on what pools can be used
grep -n 'poolId\|PoolKey' src/MainnetController.sol | head -30Repository: sparkdotfi/spark-alm-controller
Length of output: 752
🏁 Script executed:
#!/bin/bash
# Look for any other setups or constraints in the test files about 1:1 assets
rg -n '1:1|equal.*asset|token.*token|currency.*currency' test/mainnet-fork/Uniswapv4.t.sol | head -20Repository: sparkdotfi/spark-alm-controller
Length of output: 1212
🏁 Script executed:
#!/bin/bash
# Let's check if there's any validation when setting tick limits or creating pools
# Look at the full setUniswapV4TickLimits function and surrounding context
sed -n '320,350p' src/MainnetController.solRepository: sparkdotfi/spark-alm-controller
Length of output: 1147
🏁 Script executed:
#!/bin/bash
# Check if poolId can be set arbitrarily or if there's a whitelist
rg -n 'poolId.*require\|whitelist.*poolId\|validate.*pool' src/ --type=sol -iRepository: sparkdotfi/spark-alm-controller
Length of output: 102
🏁 Script executed:
#!/bin/bash
# Look at _getNormalizedBalance and understand the rate limit calculation
sed -n '500,510p' src/libraries/UniswapV4Lib.solRepository: sparkdotfi/spark-alm-controller
Length of output: 463
🏁 Script executed:
#!/bin/bash
# Let's search more carefully for any asset validation or 1:1 checks
rg -n 'currency0.*currency1\|token0.*token1' src/libraries/UniswapV4Lib.sol -B 2 -A 2Repository: sparkdotfi/spark-alm-controller
Length of output: 57
🏁 Script executed:
#!/bin/bash
# Check if there's any admin function that validates pools before they're used
rg -n 'onboard\|register\|add.*pool\|setup.*pool' src/ -i -A 3Repository: sparkdotfi/spark-alm-controller
Length of output: 57
🏁 Script executed:
#!/bin/bash
# Let's look at the full _increaseLiquidity function to understand rate limit calculation
sed -n '298,350p' src/libraries/UniswapV4Lib.solRepository: sparkdotfi/spark-alm-controller
Length of output: 2166
🏁 Script executed:
#!/bin/bash
# Let's look at the swap function to see how rate limits work there
sed -n '170,200p' src/libraries/UniswapV4Lib.solRepository: sparkdotfi/spark-alm-controller
Length of output: 1081
🏁 Script executed:
#!/bin/bash
# Check if there's any validation when a pool is first configured
# Look for any function that takes poolId and validates assets
grep -n 'function.*poolId\|function.*pool' src/MainnetController.sol | head -20Repository: sparkdotfi/spark-alm-controller
Length of output: 268
🏁 Script executed:
#!/bin/bash
# Check if there's any place where pools are registered or validated
sed -n '1,50p' src/MainnetController.solRepository: sparkdotfi/spark-alm-controller
Length of output: 2408
🏁 Script executed:
#!/bin/bash
# Look at how tests set up pools to see if they validate 1:1 assets
head -150 test/mainnet-fork/Uniswapv4.t.sol | tail -50Repository: sparkdotfi/spark-alm-controller
Length of output: 2440
🏁 Script executed:
#!/bin/bash
# Check the test setup for pool creation
sed -n '100,150p' test/mainnet-fork/Uniswapv4.t.solRepository: sparkdotfi/spark-alm-controller
Length of output: 2441
🏁 Script executed:
#!/bin/bash
# Let's check if there are any pool validation functions we might have missed
grep -n 'function.*V4\|function.*Uniswap' src/MainnetController.sol | head -20Repository: sparkdotfi/spark-alm-controller
Length of output: 277
🏁 Script executed:
#!/bin/bash
# One final check - look for any require statements that check token decimals or prices
grep -n 'decimals\|price\|exchange.*rate' src/libraries/UniswapV4Lib.solRepository: sparkdotfi/spark-alm-controller
Length of output: 270
Add validation to enforce the 1:1 asset constraint for Uniswap V4 pools.
The 1:1 asset requirement is documented in the README but not enforced in code. The setUniswapV4TickLimits function only validates tick parameters without checking that token0 and token1 are equal-valued assets. While the _increaseLiquidity function includes a comment acknowledging the assumption that tokens are valued equally, no programmatic validation prevents configuring pools with mismatched asset values.
This is critical because the rate limit calculation normalizes both tokens to 18 decimals and sums them without accounting for actual USD values (per the learned design decision about normalized balances). An admin could inadvertently—or maliciously—configure a non-1:1 pool and create a rate limit bypass via price discrepancies.
Consider adding validation in setUniswapV4TickLimits to verify token0 and token1 are equal-valued assets before allowing pool configuration.
🤖 Prompt for AI Agents
In @README.md at line 150, Add programmatic validation in setUniswapV4TickLimits
to enforce the README's 1:1 asset requirement: before accepting tick limits,
verify token0 and token1 represent equal-valued assets (e.g., compare their
canonical asset IDs or use the same oracle/price feed to ensure identical USD
value per normalized unit) and revert with a clear error if they differ; update
any callers/tests accordingly and remove the reliance on the comment in
_increaseLiquidity by making this guarantee upfront in setUniswapV4TickLimits.
| "susds": "0xdDb46999F8891663a8F2828d25298f70416d7610", | ||
| "almProxy": "0x92afd6F2385a90e44da3a8B60fe36f6cBe1D8709", | ||
| "rateLimits": "0x19D08879851FB54C2dCc4bb32b5a1EA5E9Ad6838" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Inconsistent field ordering.
The field order is almProxy then rateLimits, but the other production configs (unichain, avalanche, optimism) use rateLimits then almProxy. While JSON objects are unordered, maintaining consistency across similar configuration files improves maintainability.
🔎 Suggested reordering for consistency
"susds": "0xdDb46999F8891663a8F2828d25298f70416d7610",
- "almProxy": "0x92afd6F2385a90e44da3a8B60fe36f6cBe1D8709",
- "rateLimits": "0x19D08879851FB54C2dCc4bb32b5a1EA5E9Ad6838"
+ "rateLimits": "0x19D08879851FB54C2dCc4bb32b5a1EA5E9Ad6838",
+ "almProxy": "0x92afd6F2385a90e44da3a8B60fe36f6cBe1D8709"📝 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.
| "susds": "0xdDb46999F8891663a8F2828d25298f70416d7610", | |
| "almProxy": "0x92afd6F2385a90e44da3a8B60fe36f6cBe1D8709", | |
| "rateLimits": "0x19D08879851FB54C2dCc4bb32b5a1EA5E9Ad6838" | |
| "susds": "0xdDb46999F8891663a8F2828d25298f70416d7610", | |
| "rateLimits": "0x19D08879851FB54C2dCc4bb32b5a1EA5E9Ad6838", | |
| "almProxy": "0x92afd6F2385a90e44da3a8B60fe36f6cBe1D8709" |
🤖 Prompt for AI Agents
In @script/input/1/arbitrum_one-production.json around lines 9 - 11, Reorder the
JSON object fields so they match the ordering used in other production configs:
place "rateLimits" before "almProxy" (keeping "susds" as-is); update the entry
that currently lists "almProxy" then "rateLimits" to instead list "rateLimits"
then "almProxy" for consistency across configs.
| function withdraw( | ||
| address proxy, | ||
| address aToken, | ||
| uint256 amount, | ||
| address rateLimits, | ||
| bytes32 rateLimitWithdrawId, | ||
| bytes32 rateLimitDepositId | ||
| ) external returns (uint256 amountWithdrawn) { | ||
| address pool = IATokenWithPool(aToken).POOL(); | ||
|
|
||
| // Withdraw underlying from Aave pool, decode resulting amount withdrawn. | ||
| // Assumes proxy has adequate aTokens. | ||
| amountWithdrawn = abi.decode( | ||
| IALMProxy(proxy).doCall( | ||
| pool, | ||
| abi.encodeCall( | ||
| IPool(pool).withdraw, | ||
| (IATokenWithPool(aToken).UNDERLYING_ASSET_ADDRESS(), amount, proxy) | ||
| ) | ||
| ), | ||
| (uint256) | ||
| ); | ||
|
|
||
| IRateLimits(rateLimits).triggerRateLimitDecrease( | ||
| RateLimitHelpers.makeAddressKey(rateLimitWithdrawId, aToken), | ||
| amountWithdrawn | ||
| ); | ||
|
|
||
| IRateLimits(rateLimits).triggerRateLimitIncrease( | ||
| RateLimitHelpers.makeAddressKey(rateLimitDepositId, aToken), | ||
| amountWithdrawn | ||
| ); | ||
| } |
There was a problem hiding this comment.
Add slippage validation for withdrawals.
The withdraw function lacks slippage protection. Unlike deposit (which validates newATokens >= amount * maxSlippage / 1e18), this function does not verify that amountWithdrawn meets minimum expectations. Aave withdrawals can experience rounding or precision loss, potentially returning less than the requested amount.
🔎 Recommended fix: Add slippage validation
function withdraw(
address proxy,
address aToken,
uint256 amount,
+ uint256 maxSlippage,
address rateLimits,
bytes32 rateLimitWithdrawId,
bytes32 rateLimitDepositId
) external returns (uint256 amountWithdrawn) {
+ require(maxSlippage != 0, "MC/max-slippage-not-set");
+
address pool = IATokenWithPool(aToken).POOL();
// Withdraw underlying from Aave pool, decode resulting amount withdrawn.
// Assumes proxy has adequate aTokens.
amountWithdrawn = abi.decode(
IALMProxy(proxy).doCall(
pool,
abi.encodeCall(
IPool(pool).withdraw,
(IATokenWithPool(aToken).UNDERLYING_ASSET_ADDRESS(), amount, proxy)
)
),
(uint256)
);
+ require(
+ amountWithdrawn >= amount * maxSlippage / 1e18,
+ "MC/slippage-too-high"
+ );
+
IRateLimits(rateLimits).triggerRateLimitDecrease(
RateLimitHelpers.makeAddressKey(rateLimitWithdrawId, aToken),
amountWithdrawn
);
IRateLimits(rateLimits).triggerRateLimitIncrease(
RateLimitHelpers.makeAddressKey(rateLimitDepositId, aToken),
amountWithdrawn
);
}| pragma solidity ^0.8.21; | ||
|
|
||
| import { IERC20 } from "openzeppelin-contracts/contracts/interfaces/IERC20.sol"; | ||
| import { IERC20 } from "../../lib/openzeppelin-contracts/contracts/token/ERC20/IERC20.sol"; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Prefer using the configured remapping for consistency.
The import uses a relative path ../../lib/openzeppelin-contracts/contracts/token/ERC20/IERC20.sol instead of the remapping @openzeppelin/contracts/token/ERC20/IERC20.sol configured in foundry.toml (line 17). Consistent use of remappings improves maintainability.
🔎 Proposed fix
-import { IERC20 } from "../../lib/openzeppelin-contracts/contracts/token/ERC20/IERC20.sol";
+import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol";📝 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.
| import { IERC20 } from "../../lib/openzeppelin-contracts/contracts/token/ERC20/IERC20.sol"; | |
| import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol"; |
🤖 Prompt for AI Agents
In @src/libraries/CCTPLib.sol at line 4, The import in CCTPLib.sol uses a
relative path to OpenZeppelin instead of the project remapping; update the
import line that references IERC20 to use the remapped path
"@openzeppelin/contracts/token/ERC20/IERC20.sol" (replace the current
"../../lib/openzeppelin-contracts/..." import) so the IERC20 import uses the
configured foundry remapping for consistency and maintainability.
| pragma solidity ^0.8.21; | ||
|
|
||
| import { IERC20 } from "openzeppelin-contracts/contracts/interfaces/IERC20.sol"; | ||
| import { IERC20 } from "../../lib/openzeppelin-contracts/contracts/token/ERC20/IERC20.sol"; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Prefer using the configured remapping for consistency.
The import uses a relative path ../../lib/openzeppelin-contracts/contracts/token/ERC20/IERC20.sol instead of the remapping @openzeppelin/contracts/token/ERC20/IERC20.sol configured in foundry.toml (line 17). While both work, using remappings consistently across the codebase improves maintainability and resilience to directory structure changes.
🔎 Proposed fix
-import { IERC20 } from "../../lib/openzeppelin-contracts/contracts/token/ERC20/IERC20.sol";
+import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol";📝 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.
| import { IERC20 } from "../../lib/openzeppelin-contracts/contracts/token/ERC20/IERC20.sol"; | |
| import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol"; |
🤖 Prompt for AI Agents
In @src/libraries/PSMLib.sol at line 4, Replace the relative OpenZeppelin import
with the remapped path: update the import in PSMLib.sol that currently
references ../../lib/openzeppelin-contracts/... to use
@openzeppelin/contracts/token/ERC20/IERC20.sol so the IERC20 symbol is imported
via the configured Foundry remapping for consistency and future-proofing.
| uint256 internal constant _V4_SWAP = 0x10; | ||
|
|
||
| // NOTE: From https://docs.uniswap.org/contracts/v4/deployments (Ethereum Mainnet). | ||
| address internal constant _PERMIT2 = 0x000000000022D473030F116dDEE9F6B43aC78BA3; | ||
| address internal constant _POSITION_MANAGER = 0xbD216513d74C8cf14cf4747E6AaA6420FF64ee9e; | ||
| address internal constant _ROUTER = 0x66a9893cC07D91D95644AEDD05D03f95e1dBA8Af; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Hardcoded mainnet addresses limit deployability.
These constants (_PERMIT2, _POSITION_MANAGER, _ROUTER) are Ethereum mainnet-specific. If this library needs to support other networks (testnets, L2s), these addresses will differ.
Consider passing these addresses as parameters or using a registry pattern if multi-chain deployment is anticipated. For now, document that this library is mainnet-only.
🤖 Prompt for AI Agents
In @src/libraries/UniswapV4Lib.sol around lines 30 - 35, The three addresses
_PERMIT2, _POSITION_MANAGER, and _ROUTER are hardcoded to mainnet, which
prevents multi-chain deployment; replace these internal constant addresses with
configurable values (e.g., constructor/initializer-set immutable variables or
function parameters) or route lookups through a registry contract so the library
can be reused across networks, and if you keep them constant add a clear comment
that UniswapV4Lib is mainnet-only. Update all usages of _PERMIT2,
_POSITION_MANAGER, and _ROUTER to read the new configurable variables or
registry lookups.
| function _approveWithPermit2( | ||
| address proxy, | ||
| address token, | ||
| address spender, | ||
| uint256 amount | ||
| ) | ||
| internal | ||
| { | ||
| require(amount <= type(uint160).max, "MC/amount-too-large-for-permit2"); | ||
|
|
||
| // Approve the Permit2 contract to spend none of the token (success is optional). | ||
| // NOTE: We don't care about the success of this call, since the only outcomes are: | ||
| // - the allowance is 0 (it was reset or was already 0) | ||
| // - the allowance is not 0, in which case the success of the overall set of | ||
| // operations is dependent on the success of the subsequent calls. | ||
| // In other words, this is a convenience call that may not even be needed for success. | ||
| proxy.call( | ||
| abi.encodeCall( | ||
| IALMProxy.doCall, | ||
| (token, abi.encodeCall(IERC20Like.approve, (_PERMIT2, 0))) | ||
| ) | ||
| ); | ||
|
|
||
| if (amount != 0) { | ||
| // Approve the Permit2 contract to spend the amount of token (success is mandatory). | ||
| bytes memory approveResult = IALMProxy(proxy).doCall( | ||
| token, | ||
| abi.encodeCall(IERC20Like.approve, (_PERMIT2, amount)) | ||
| ); | ||
|
|
||
| // Revert if approve returns anything, and that anything is not `true`. | ||
| require( | ||
| approveResult.length == 0 || abi.decode(approveResult, (bool)), | ||
| "MC/permit2-approve-failed" | ||
| ); | ||
| } | ||
|
|
||
| // Finally, approve the spender to spend the token via Permit2. | ||
| IALMProxy(proxy).doCall( | ||
| _PERMIT2, | ||
| abi.encodeCall( | ||
| IPermit2Like.approve, | ||
| (token, spender, uint160(amount), uint48(block.timestamp)) | ||
| ) | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Unchecked low-level call result may mask failures.
Line 269-274 uses proxy.call(...) without checking the return value. While the comment explains this is intentional (the call is a "convenience" reset), if the proxy itself reverts for an unexpected reason (e.g., out of gas), this would be silently ignored.
The current approach is acceptable given the documented rationale, but consider logging or emitting an event if the reset fails for debugging purposes in production.
🤖 Prompt for AI Agents
In @src/libraries/UniswapV4Lib.sol around lines 253 - 298, In
_approveWithPermit2, replace the unchecked proxy.call(...) reset with an
explicit (bool success, bytes memory data) = proxy.call(...) and, if success is
false, emit a non-reverting event (e.g., Permit2ResetFailed(address proxy,
address token, bytes data)) so failures (like out-of-gas or unexpected reverts)
are recorded for debugging while preserving the original non-fatal behavior; add
the event declaration and emit it when success==false, but do not change the
current non-revert semantics that the comment intends.
| function _decreaseLiquidity( | ||
| address proxy, | ||
| address rateLimits, | ||
| bytes32 poolId, | ||
| address token0, | ||
| address token1, | ||
| bytes memory callData | ||
| ) | ||
| internal | ||
| { | ||
| // Get token balances before liquidity decrease. | ||
| uint256 startingBalance0 = _getBalance(token0, proxy); | ||
| uint256 startingBalance1 = _getBalance(token1, proxy); | ||
|
|
||
| // Perform action. | ||
| IALMProxy(proxy).doCall(_POSITION_MANAGER, callData); | ||
|
|
||
| // Get token balances after liquidity decrease. | ||
| uint256 endingBalance0 = _getBalance(token0, proxy); | ||
| uint256 endingBalance1 = _getBalance(token1, proxy); | ||
|
|
||
| // NOTE: The limitation of this integration is the assumption that the tokens are valued | ||
| // equally (i.e. 1.000000 USDC = 1.000000000000000000 USDS). | ||
| uint256 rateLimitDecrease = | ||
| _getNormalizedBalance(token0, endingBalance0 - startingBalance0) + | ||
| _getNormalizedBalance(token1, endingBalance1 - startingBalance1); | ||
|
|
||
| // Perform rate limit decrease. | ||
| // NOTE: Rate limit decrease includes any token0 or token1 received due to fees. | ||
| IRateLimits(rateLimits).triggerRateLimitDecrease( | ||
| RateLimitHelpers.makeBytes32Key(LIMIT_WITHDRAW, poolId), | ||
| rateLimitDecrease | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Potential underflow if balances decrease unexpectedly.
Lines 373-374 compute endingBalance0 - startingBalance0 without checking that ending >= starting. If the position manager has a bug or unexpected behavior that reduces balances, this would underflow.
🔎 Suggested defensive check
+ require(endingBalance0 >= startingBalance0, "MC/unexpected-balance0-decrease");
+ require(endingBalance1 >= startingBalance1, "MC/unexpected-balance1-decrease");
+
uint256 rateLimitDecrease =
_getNormalizedBalance(token0, endingBalance0 - startingBalance0) +
_getNormalizedBalance(token1, endingBalance1 - startingBalance1);🤖 Prompt for AI Agents
In @src/libraries/UniswapV4Lib.sol around lines 349 - 382, In
_decreaseLiquidity, avoid underflow when computing endingBalance0 -
startingBalance0 and endingBalance1 - startingBalance1 by first computing deltas
safely: for each token compute delta as ending >= starting ? ending - starting :
0 (or revert with a clear require if you prefer failing-fast); then pass those
non-negative deltas to _getNormalizedBalance and use the sum for
rateLimitDecrease before calling IRateLimits.triggerRateLimitDecrease on the
RateLimit key. Ensure the fix is applied to the function _decreaseLiquidity and
update the inline comment to reflect the defensive check.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.