fix: Add an explicit bounds check to prevent silent truncation in _approveWithPermit2

[supercontracts]

Summary by CodeRabbit

• Bug Fixes

  • Added a runtime guard to block approval requests that exceed the permitted maximum; oversized permit attempts now fail early with a clear rejection ("MC/amount-too-large-for-permit2"), improving safety and reliability.

• Tests

  • Updated and renamed tests to reflect the new maximum-amount boundary semantics and added explicit revert expectations validating the guard.

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

[octane-security-app]

Summary by Octane

New Contracts

No new contracts were added.

Updated Contracts

• UniswapV4Lib.sol: The smart contract now ensures pool ID matches and limits permit2 amount to uint160 max.

---

🔗 Commit Hash: 4a1a221

[coderabbitai]

Warning

Rate limit exceeded

@deluca-mike has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 34 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between a94ce31 and 1c9ed33.

📒 Files selected for processing (2)

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

Walkthrough

Added a runtime guard in UniswapV4Lib::_approveWithPermit2 to revert with "MC/amount-too-large-for-permit2" when the approved amount exceeds type(uint160).max; updated tests to use fixed inputs, expect this revert, and renamed several test functions to reflect the new boundary semantics.

Changes

Cohort / File(s)	Summary
Permit2 amount guard src/libraries/UniswapV4Lib.sol	Add runtime check in internal _approveWithPermit2 enforcing amount <= type(uint160).max; revert with "MC/amount-too-large-for-permit2" if exceeded. No public API changes.
Tests: boundary semantics & expectations test/mainnet-fork/Uniswapv4.t.sol	Rename multiple test functions to reflect "MaxAmountsTooLargeForPermit2"/boundary names; replace forecasted amounts with fixed inputs; add explicit revert expectations for "MC/amount-too-large-for-permit2" and adjust max-amount parameters across several test cases.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• Review focus:
  • src/libraries/UniswapV4Lib.sol — verify correct uint160 threshold and exact error string.
  • test/mainnet-fork/Uniswapv4.t.sol — ensure renamed test signatures integrate with test runner and that revert assertions target the new guard.
  • Check any callers that might rely on larger Permit2 amounts.

Possibly related PRs

• feat: UniswapV4 mint/increase/decrease liquidity support #189 — Touches same Permit2 approval path; likely introduced related Permit2 usage patterns.
• feat: UniswapV4 swap support (SC-1249) #194 — Integrates _approveWithPermit2 into swap flow; this guard could affect that flow's behavior.
• fix: Minor comments  #206 — Also modifies _approveWithPermit2; may overlap on wording or checks.

Suggested reviewers

• lucas-manuel
• supercontracts

Poem

🐇 I nibbled through bytes and paused at a gate,
A number too tall — I said, "Not today."
A tiny guard set, neat and polite,
Tests updated, all tucked in tight.
Hop, hop — safe code, and carrots in sight. 🥕

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 starts with a valid prefix ('fix:') and clearly describes the main change: adding an explicit bounds check to prevent silent truncation in the _approveWithPermit2 function, which aligns with the code changes.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

---

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

[octane-security-app]

Overview

Vulnerabilities found:	10

Warnings found:	13

---

🔗 Commit Hash: 4a1a221
🛡️ Octane Dashboard: All vulnerabilities

[coderabbitai]

Actionable comments posted: 1

📜 Review details

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between caba61b and ec7bb87.

📒 Files selected for processing (2)

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

🧰 Additional context used

🧠 Learnings (4)

📚 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

📚 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

⏰ 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 (2)

src/libraries/UniswapV4Lib.sol (1)

258-259: LGTM! Excellent defensive programming.

The bounds check correctly prevents silent truncation when casting amount to uint160 at line 292. Without this guard, amounts exceeding type(uint160).max would have been silently truncated, potentially approving far less than intended and causing transaction failures or allowing operations with insufficient approvals.

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

1020-1060: Well-structured boundary tests.

The permit2 boundary test correctly validates:

1. amount0Max exceeding type(uint160).max reverts with "MC/amount-too-large-for-permit2"
2. amount1Max exceeding type(uint160).max reverts with "MC/amount-too-large-for-permit2"
3. Valid amounts within bounds succeed

Using fixed deal amounts (1_000_000e6) is appropriate here since we're testing the boundary check, not the liquidity provision logic.

[github-actions]

Coverage after merging fix/cantina-13-_approveWithPermit2 into dev will be

99.36%

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	94.90%	84.62%	95.65%	97.22%	128–129, 129, 129, 316–317, 573
MainnetController.sol	99.18%	100%	98.28%	99.24%	538–539
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.33%	96%	100%	100%	284