feat: ERC4626 Exchange Rate Threshold (SC-1229) #186
Conversation
deluca-mike
requested review from
hacker-DOM,
hivtushok,
lucas-manuel and
supercontracts
WalkthroughReplaces per-pool slippage gating with per-token exchange-rate ceilings: adds Changes
Sequence Diagram(s)sequenceDiagram
participant Relayer
participant Controller
participant ERC4626
participant Storage as "maxExchangeRates"
Relayer->>Controller: depositERC4626(token, amount)
Controller->>ERC4626: proxy convertToShares(amount) -> shares
ERC4626-->>Controller: shares
Controller->>Storage: read maxExchangeRates[token]
Storage-->>Controller: maxRate
rect rgb(240,255,240)
alt _getExchangeRate(shares, amount) <= maxRate
Controller->>ERC4626: approve IERC4626(token).asset()
Controller->>ERC4626: finalize deposit / hold
Controller-->>Relayer: return shares
else _getExchangeRate > maxRate
Controller-->>Relayer: revert "exchange-rate-too-high"
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (11)
src/ForeignController.sol(4 hunks)src/MainnetController.sol(4 hunks)test/base-fork/Morpho.t.sol(2 hunks)test/base-fork/MorphoAllocations.t.sol(1 hunks)test/base-fork/SparkVault.t.sol(2 hunks)test/mainnet-fork/4626Calls.t.sol(5 hunks)test/mainnet-fork/ERC4626DonationAttack.t.sol(2 hunks)test/mainnet-fork/Ethena.t.sol(1 hunks)test/mainnet-fork/Maple.t.sol(2 hunks)test/mainnet-fork/SparkVault.t.sol(1 hunks)test/unit/controllers/Admin.t.sol(17 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/SparkVault.t.soltest/base-fork/Morpho.t.soltest/mainnet-fork/ERC4626DonationAttack.t.soltest/base-fork/MorphoAllocations.t.soltest/mainnet-fork/Maple.t.soltest/unit/controllers/Admin.t.soltest/mainnet-fork/4626Calls.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/SparkVault.t.soltest/base-fork/Morpho.t.soltest/mainnet-fork/ERC4626DonationAttack.t.soltest/base-fork/MorphoAllocations.t.soltest/mainnet-fork/Maple.t.soltest/unit/controllers/Admin.t.soltest/mainnet-fork/4626Calls.t.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/base-fork/SparkVault.t.soltest/base-fork/Morpho.t.soltest/mainnet-fork/ERC4626DonationAttack.t.soltest/unit/controllers/Admin.t.soltest/mainnet-fork/4626Calls.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/4626Calls.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 (19)
src/ForeignController.sol (3)
53-53: LGTM! Event declaration follows existing patterns.The event declaration is consistent with the existing
MaxSlippageSetevent and matches the implementation inMainnetController.sol.
88-89: LGTM! State variable properly declared.The
maxExchangeRatesmapping is correctly structured for per-token exchange rate ceilings.
345-363: LGTM! Exchange rate gating logic is correctly implemented.The changes successfully replace slippage-based validation with exchange rate ceiling checks:
- The check
IERC4626(token).convertToAssets(1e18) <= maxExchangeRates[token]correctly validates that the current exchange rate doesn't exceed the configured maximum- The refactoring to use a plain
address assetvariable improves readability- The direct return eliminates an unnecessary local variable
test/base-fork/SparkVault.t.sol (2)
4-4: LGTM! Import added for IERC4626 interface usage.The import is necessary for the exchange rate configuration in the test setup.
292-294: LGTM! Test configuration updated for exchange rate gating.The setup correctly migrates from slippage-based validation to exchange rate ceiling. The
1.2e18value allows up to 20% premium on the vault's exchange rate, which is a reasonable upper bound for testing purposes.test/mainnet-fork/Maple.t.sol (2)
59-59: LGTM! Test setup uses current exchange rate as ceiling.Setting the maximum exchange rate to the current value (
syrup.convertToAssets(1e18)) ensures the initial setup passes validation while allowing tests to adjust this ceiling for boundary testing.
128-145: LGTM! Comprehensive boundary test for exchange rate validation.The test correctly validates the exchange rate boundary:
- Setting the ceiling just below the current rate causes rejection
- Setting it to the exact current rate allows deposits
- This ensures the validation logic works as intended
test/mainnet-fork/Ethena.t.sol (1)
664-664: LGTM! High exchange rate ceiling appropriate for sUSDe testing.The
10.0e18ceiling is appropriate for sUSDe (a yield-bearing staked USDe token) as it allows for significant appreciation over time in E2E test scenarios without blocking deposits.test/mainnet-fork/SparkVault.t.sol (1)
288-288: LGTM! Test configuration consistent with base-fork tests.The exchange rate ceiling of
1.2e18matches the configuration intest/base-fork/SparkVault.t.sol, maintaining consistency across test suites.test/base-fork/MorphoAllocations.t.sol (1)
222-222: LGTM! Exchange rate configuration uses current vault state.Setting the ceiling to the current exchange rate ensures the test setup is valid while allowing reallocation tests to proceed normally.
test/mainnet-fork/ERC4626DonationAttack.t.sol (2)
90-97: LGTM! Test correctly validates exchange rate protection against donation attacks.The test demonstrates that setting an appropriate exchange rate ceiling (1.2e18) prevents the donation attack from succeeding by rejecting deposits when the inflated exchange rate exceeds the configured maximum.
102-105: LGTM! Test demonstrates vulnerability when protection is disabled.Setting the exchange rate ceiling to
type(uint256).maxeffectively disables the protection, correctly demonstrating that proper configuration is essential to prevent donation attacks.src/MainnetController.sol (3)
102-102: LGTM! Event declaration consistent across controllers.The event matches the implementation in
ForeignController.soland follows existing event patterns in the contract.
192-193: LGTM! State variable mirrors ForeignController implementation.The
maxExchangeRatesmapping is correctly structured and maintains consistency withForeignController.sol.
454-472: LGTM! Exchange rate validation correctly implemented.The changes successfully migrate from post-deposit slippage checks to pre-deposit exchange rate validation:
- The exchange rate ceiling check is performed before any state changes
- The refactoring improves code clarity
- Consistent with
ForeignController.solimplementationtest/base-fork/Morpho.t.sol (1)
143-160: Great exchange-rate boundary coverageNice job hardening the test by dialing the cap below and back to the live
convertToAssets(1e18)before asserting both revert and success. This gives strong confidence the new gating won’t regress silently.test/mainnet-fork/4626Calls.t.sol (2)
24-32: Nice reuse of cached rate-limit keysCaching
depositKey/withdrawKeyinsetUptrims repetition and keeps the later assertions focused on behavior instead of plumbing. Clean refactor.
76-106: Exchange-rate boundary test hits the key failure modeThe test walks through minting, forcing the cap below the live price, and verifying both the revert string and the success path once the cap is restored—exactly what we need to guard against admin misconfiguration.
test/unit/controllers/Admin.t.sol (1)
610-644: Admin coverage for the new setter looks solidThe ForeignController suite mirrors the mainnet tests with reentrancy, auth, zero-address, and happy-path checks—great to see parity across domains.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
src/ForeignController.sol (1)
163-174: Consider validating non-zero exchange rate.The function allows setting
maxExchangeRateto zero (ifmaxExpectedAssetsis 0), which would block all deposits for that token. A past review flagged this. Consider whether zero is an intentional way to pause deposits or if validation is needed.src/MainnetController.sol (1)
297-308: Consider validating non-zero exchange rate.Similar to ForeignController, this function allows setting
maxExchangeRateto zero, which would block all deposits for that token. Refer to the past review comment for details.test/unit/controllers/Admin.t.sol (1)
385-439: Comprehensive test coverage for setMaxExchangeRate.The test contract provides thorough coverage including reentrancy, authorization, validation, and functionality tests. The exchange rate calculations in event expectations are correct.
Note: A past review suggested storing
makeAddr("token")once for consistency (see past review comment), but this is a minor readability improvement.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (12)
src/ForeignController.sol(6 hunks)src/MainnetController.sol(7 hunks)test/base-fork/ForkTestBase.t.sol(1 hunks)test/base-fork/Morpho.t.sol(2 hunks)test/base-fork/MorphoAllocations.t.sol(2 hunks)test/base-fork/SparkVault.t.sol(2 hunks)test/mainnet-fork/4626Calls.t.sol(5 hunks)test/mainnet-fork/ERC4626DonationAttack.t.sol(1 hunks)test/mainnet-fork/Ethena.t.sol(1 hunks)test/mainnet-fork/Maple.t.sol(2 hunks)test/mainnet-fork/SparkVault.t.sol(1 hunks)test/unit/controllers/Admin.t.sol(18 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/ERC4626DonationAttack.t.soltest/mainnet-fork/SparkVault.t.soltest/base-fork/MorphoAllocations.t.soltest/base-fork/Morpho.t.soltest/mainnet-fork/4626Calls.t.soltest/base-fork/ForkTestBase.t.soltest/unit/controllers/Admin.t.soltest/mainnet-fork/Maple.t.soltest/base-fork/SparkVault.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/ERC4626DonationAttack.t.soltest/mainnet-fork/SparkVault.t.soltest/base-fork/MorphoAllocations.t.soltest/base-fork/Morpho.t.soltest/mainnet-fork/4626Calls.t.soltest/base-fork/ForkTestBase.t.soltest/unit/controllers/Admin.t.soltest/mainnet-fork/Maple.t.soltest/base-fork/SparkVault.t.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/ERC4626DonationAttack.t.soltest/base-fork/Morpho.t.soltest/mainnet-fork/4626Calls.t.soltest/base-fork/ForkTestBase.t.soltest/unit/controllers/Admin.t.soltest/base-fork/SparkVault.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/4626Calls.t.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). (2)
- GitHub Check: coverage
- GitHub Check: test
🔇 Additional comments (22)
test/base-fork/SparkVault.t.sol (2)
4-4: LGTM!The IERC4626 import addition is appropriate for the test context.
292-294: LGTM!The migration from
setMaxSlippagetosetMaxExchangeRatecorrectly uses the new API with appropriate parameters (1e18 shares, 1.2e18 max assets).src/ForeignController.sol (2)
53-53: LGTM!The event, constant, and mapping declarations for the exchange rate mechanism are well-defined. The 1e36 precision allows for accurate exchange rate calculations across tokens with varying decimal places.
Also applies to: 61-61, 90-91
345-368: LGTM!The deposit flow correctly transitions from slippage-based validation to exchange-rate-based validation. The check
_getExchangeRate(shares, amount) <= maxExchangeRates[token]ensures deposits are rejected if the exchange rate exceeds the configured ceiling.src/MainnetController.sol (2)
102-102: LGTM!The event, constant, and mapping declarations mirror those in ForeignController, ensuring consistent behavior across both controllers.
Also applies to: 135-135, 194-195
455-477: LGTM!The deposit flow correctly implements exchange-rate-based validation, consistent with ForeignController.
test/mainnet-fork/4626Calls.t.sol (3)
18-32: LGTM!The setUp updates correctly initialize the test environment with the new exchange-rate-based approach. Moving
depositKeyandwithdrawKeyto state variables improves code reusability across test functions.
76-95: LGTM!The test correctly validates exchange-rate boundary behavior: deposits fail at the exact ceiling but succeed when the ceiling is raised slightly. The prank structure is properly organized.
97-106: LGTM!The test updates properly separate admin actions (under SPARK_PROXY prank) from relayer operations, maintaining clear test boundaries and correct authorization contexts.
Also applies to: 173-187, 289-309
test/unit/controllers/Admin.t.sol (2)
4-5: LGTM!The imports are appropriate for the new test coverage, and using fully qualified event names (e.g.,
MainnetController.MintRecipientSet) improves test clarity and prevents ambiguity.Also applies to: 83-83, 90-90, 99-99
628-678: LGTM!The ForeignController tests mirror the MainnetController tests, ensuring consistent coverage and validation across both controllers.
test/base-fork/ForkTestBase.t.sol (1)
6-7: LGTM! Import additions support ERC4626 functionality.The IERC4626 import enables tests to reference ERC4626 interfaces for the new exchange-rate validation logic introduced in this PR.
test/mainnet-fork/SparkVault.t.sol (1)
288-288: Verify the 20% exchange-rate ceiling is intentional.The previous slippage configuration allowed only 0.01% deviation (
1e18 - 1e4), while the new exchange-rate ceiling permits up to 20% (1.2e18for1e18shares). This is a significant relaxation that may be intentional for broader test coverage, but please confirm this aligns with the expected risk tolerance for the Morpho DAI vault in production scenarios.test/mainnet-fork/ERC4626DonationAttack.t.sol (2)
88-96: LGTM! Donation attack test correctly adapted to exchange-rate validation.The test properly validates that the exchange-rate ceiling prevents donation attacks by setting a max exchange rate and expecting the appropriate revert message when exceeded.
100-103: LGTM! Success case correctly uses permissive exchange-rate ceiling.The success test appropriately sets an extremely high exchange-rate ceiling (
1e24) to demonstrate that the donation attack can succeed when exchange-rate validation is too permissive, confirming the importance of proper configuration.test/mainnet-fork/Maple.t.sol (2)
59-59: LGTM! Setup correctly migrated to exchange-rate API.The configuration properly sets the max exchange rate using the Syrup vault's current conversion rate as the baseline.
128-145: LGTM! Exchange-rate boundary test is well-structured.The test correctly validates the exchange-rate ceiling by:
- Setting the max to exactly the current conversion rate (causing failure)
- Incrementing by one unit to allow success
This properly tests the boundary condition of the exchange-rate validation.
test/base-fork/MorphoAllocations.t.sol (2)
212-212: Clarify the_xxxsuffix in test name.The test name
test_reallocateMorpho_xxxincludes an unusual_xxxsuffix. If this is a placeholder or temporary naming, please update it to a descriptive name. If it's intentional, consider documenting the naming convention.
222-222: LGTM! Exchange-rate configuration correctly implemented.The migration from
setMaxSlippagetosetMaxExchangeRateproperly configures the exchange-rate ceiling using the vault's conversion rate, allowing up to 2x the base rate for 1M USDC deposits.test/base-fork/Morpho.t.sol (3)
105-106: LGTM! Setup correctly migrated to exchange-rate API.Both USDS and USDC vault configurations properly use the new
setMaxExchangeRateAPI with the current conversion rates as ceilings.
143-160: LGTM! Exchange-rate boundary test correctly validates limits.The test properly validates the exchange-rate ceiling by:
- Setting max to
convertToAssets(1e18) - 1(just below current, causing failure)- Resetting to exact
convertToAssets(1e18)(allowing success at the boundary)This correctly tests the >= comparison in the exchange-rate validation logic.
162-182: LGTM! Rate-limit boundary tests correctly restructured.The prank positioning adjustments properly separate the failure and success cases while maintaining correct rate-limit validation for both USDS and USDC vaults.
deluca-mike
force-pushed
the
sc1229-erc4626-exchange-rate-threshold
branch
from
a90750c to
98c8905
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (22)
src/ForeignController.sol(6 hunks)src/MainnetController.sol(7 hunks)test/base-fork/Aave.t.sol(2 hunks)test/base-fork/Morpho.t.sol(4 hunks)test/base-fork/MorphoAllocations.t.sol(1 hunks)test/base-fork/PsmCalls.t.sol(5 hunks)test/base-fork/TransferAsset.t.sol(1 hunks)test/mainnet-fork/4626Calls.t.sol(5 hunks)test/mainnet-fork/Aave.t.sol(4 hunks)test/mainnet-fork/Buidl.t.sol(1 hunks)test/mainnet-fork/CCTPCalls.t.sol(8 hunks)test/mainnet-fork/Curve.t.sol(13 hunks)test/mainnet-fork/Ethena.t.sol(1 hunks)test/mainnet-fork/Farm.t.sol(2 hunks)test/mainnet-fork/LayerZero.t.sol(6 hunks)test/mainnet-fork/Maple.t.sol(5 hunks)test/mainnet-fork/OTCSwaps.t.sol(1 hunks)test/mainnet-fork/PsmCalls.t.sol(3 hunks)test/mainnet-fork/Superstate.t.sol(1 hunks)test/mainnet-fork/TransferAsset.t.sol(1 hunks)test/mainnet-fork/VaultCalls.t.sol(3 hunks)test/mainnet-fork/wstETH.t.sol(2 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/TransferAsset.t.soltest/mainnet-fork/CCTPCalls.t.soltest/base-fork/MorphoAllocations.t.soltest/mainnet-fork/Aave.t.soltest/mainnet-fork/OTCSwaps.t.soltest/mainnet-fork/Superstate.t.soltest/mainnet-fork/Farm.t.soltest/mainnet-fork/VaultCalls.t.soltest/mainnet-fork/wstETH.t.soltest/mainnet-fork/LayerZero.t.soltest/mainnet-fork/Maple.t.soltest/mainnet-fork/PsmCalls.t.soltest/base-fork/TransferAsset.t.soltest/mainnet-fork/4626Calls.t.soltest/base-fork/Morpho.t.soltest/base-fork/Aave.t.soltest/mainnet-fork/Buidl.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/TransferAsset.t.soltest/mainnet-fork/CCTPCalls.t.soltest/base-fork/MorphoAllocations.t.soltest/mainnet-fork/Aave.t.soltest/mainnet-fork/OTCSwaps.t.soltest/mainnet-fork/Superstate.t.soltest/mainnet-fork/Farm.t.soltest/mainnet-fork/VaultCalls.t.soltest/base-fork/PsmCalls.t.soltest/mainnet-fork/wstETH.t.soltest/mainnet-fork/LayerZero.t.soltest/mainnet-fork/Maple.t.soltest/mainnet-fork/PsmCalls.t.solsrc/MainnetController.soltest/base-fork/TransferAsset.t.soltest/mainnet-fork/4626Calls.t.soltest/base-fork/Morpho.t.soltest/base-fork/Aave.t.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/Maple.t.soltest/mainnet-fork/4626Calls.t.soltest/base-fork/Morpho.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/PsmCalls.t.solsrc/MainnetController.soltest/mainnet-fork/4626Calls.t.solsrc/ForeignController.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). (4)
- GitHub Check: coverage
- GitHub Check: test
- GitHub Check: test
- GitHub Check: build
🔇 Additional comments (54)
test/mainnet-fork/Superstate.t.sol (1)
69-74: LGTM! Clean refactoring to per-call pranking.The change from a persistent
vm.startPrank(relayer)context to explicitvm.prank(relayer)calls before each operation is a best practice for Foundry tests. It makes the caller context explicit for each call and eliminates the risk of forgetting to stop the prank. The test logic is preserved correctly.test/base-fork/TransferAsset.t.sol (1)
62-67: LGTM! Improved test clarity with per-call pranks.Replacing extended prank blocks with explicit per-call
vm.prank(relayer)makes the test more explicit about the caller for each operation and prevents accidental state leakage. This change aligns with the pattern used consistently throughout this test file.test/mainnet-fork/PsmCalls.t.sol (3)
105-105: LGTM: Formatting improvement.The empty line appropriately separates the key declaration from the subsequent test execution logic, improving readability.
197-202: LGTM: Improved test clarity with per-call prank pattern.The shift from multi-call prank blocks to individual
vm.prank(relayer)calls before each operation improves test readability and maintainability. This pattern makes it explicit which calls execute as the relayer and is especially clear when one call is expected to revert (line 199) while another succeeds (line 202).
475-475: LGTM: Formatting improvement.The empty line appropriately separates the key declaration from the subsequent test execution logic, consistent with the formatting at line 105.
test/mainnet-fork/Farm.t.sol (1)
81-86: LGTM: Per-call pranks improve test clarity.The switch from
vm.startPrank(relayer)to individualvm.prank(relayer)calls makes the relayer permission scope explicit for each operation and prevents context leakage between test steps.test/base-fork/PsmCalls.t.sol (1)
68-79: LGTM! Improved prank management pattern.The refactoring to use
vm.startPrank(relayer)/vm.stopPrank()pairs correctly scopes the relayer context across all operations in each test. This is cleaner than individualvm.prank()calls and ensures proper cleanup. The pattern is consistently applied across all rate-limit boundary tests that involve multiple controller operations.Also applies to: 81-92, 94-105, 277-296, 298-317, 319-341
test/mainnet-fork/CCTPCalls.t.sol (8)
59-61: LGTM: Correct relayer context for rate-limit validation.The addition of
vm.prank(relayer)ensures the call is made with the required RELAYER role, allowing the test to reach and verify the zero-maxAmount rate-limit check.
69-71: LGTM: Consistent relayer context pattern.Correctly applies the same pattern as the domain rate-limit test, ensuring proper authorization before rate-limit validation.
98-103: LGTM: Proper per-call pranks for boundary testing.The two separate
vm.prank(relayer)calls correctly establish relayer context for eachtransferUSDCToCCTPinvocation. Sincevm.prankaffects only the next call, both the failing and succeeding boundary tests require their own prank.
132-137: LGTM: Consistent boundary test pattern.Applies the same correct per-call prank pattern as the CCTP boundary test, ensuring both the failing and succeeding cases execute with proper relayer authorization.
351-353: LGTM: ForeignController parity with MainnetController tests.Applies the same correct relayer context pattern to the ForeignController test suite, maintaining consistency across both controller implementations.
361-363: LGTM: Consistent CCTP rate-limit test pattern.Correctly applies the relayer prank pattern for the ForeignController CCTP rate-limit validation test.
390-395: LGTM: Correct boundary test implementation.The two per-call pranks properly establish relayer context for both the failing and succeeding boundary cases in the ForeignController CCTP rate-limit test.
424-429: LGTM: Completes consistent refactoring across test suites.The final application of the per-call prank pattern ensures the ForeignController domain rate-limit boundary test maintains the same structure as its MainnetController counterpart.
test/mainnet-fork/wstETH.t.sol (2)
71-76: LGTM: Per-call prank improves test clarity.Replacing the continuous prank block with discrete
vm.prank(relayer)calls before each operation makes the test more explicit about which calls run under the relayer context. This is a safer pattern that prevents accidental prank leakage.
146-151: LGTM: Consistent per-call prank pattern.This change mirrors the pattern at lines 71-76, providing explicit relayer context for each operation in the rate-limit boundary test.
test/mainnet-fork/VaultCalls.t.sol (1)
36-41: LGTM: Per-call prank pattern applied consistently.The shift from
vm.startPrank(relayer)to individualvm.prank(relayer)calls makes the relayer context explicit for each operation, consistent with the broader test suite refactor.test/mainnet-fork/TransferAsset.t.sol (1)
62-67: LGTM: Per-call prank improves test scoping.Consistent with the broader test suite refactor, this change makes the relayer context explicit for each
transferAssetcall.test/base-fork/Aave.t.sol (2)
187-187: LGTM: Explicit prank cleanup prevents state leakage.Adding
vm.stopPrank()after test operations prevents prank context from leaking into subsequent tests. This is good hygiene, especially in tests with complex prank sequences.Also applies to: 206-207
92-97: No action required—review comment is based on misunderstanding of Foundry's test framework.The
vm.expectRevert()call correctly applies to thedepositAave()call that follows, even withvm.prank()in between. In Foundry,prank()is a configuration call (not a state change), so it doesn't consume the expectation set byexpectRevert(). This pattern is already used elsewhere in the same file and is idiomatic.Likely an incorrect or invalid review comment.
src/MainnetController.sol (2)
1127-1135: LGTM: Edge case handling is correct.The
_getExchangeRatefunction properly handles edge cases:
- Returns 0 for zero assets (valid case of 0 shares and 0 assets)
- Reverts for zero shares with non-zero assets (infinite exchange rate)
- Calculates exchange rate with proper precision otherwise
This addresses the concerns raised in past review comments.
461-479: LGTM: Exchange-rate gating replaces slippage checks correctly.The updated
depositERC4626logic:
- Retrieves the per-token exchange rate ceiling (line 461)
- Validates it's set (line 463)
- Gets the asset from the ERC4626 token (line 466)
- Performs the deposit (lines 469-475)
- Validates the exchange rate (lines 477-480)
The flow is correct and the use of
IERC4626(token).asset()at line 466 is more general than hardcoding the asset address.test/mainnet-fork/OTCSwaps.t.sol (1)
152-157: LGTM: Per-call prank pattern applied consistently.These changes align with the broader test suite refactor, replacing continuous prank blocks with explicit per-call pranks. The pattern is consistent across both USDT and USDS rate-limit boundary tests.
Also applies to: 163-168
test/mainnet-fork/Buidl.t.sol (1)
50-55: LGTM: Consistent per-call prank pattern.This change maintains consistency with the test suite refactor, using explicit per-call pranks for clearer test control flow.
test/mainnet-fork/Aave.t.sol (3)
115-120: LGTM: Per-call prank pattern in rate-limit boundary tests.Both USDS and USDC rate-limit boundary tests now use explicit per-call pranks, making the relayer context clear for each operation. This is consistent with the broader test suite refactor.
Also applies to: 126-131
256-261: LGTM: Explicit prank for each operation.The test correctly uses separate
vm.prank(relayer)calls before the deposit and withdraw operations, ensuring explicit relayer context for each call.
275-276: LGTM: Proper prank cleanup.Adding
vm.stopPrank()at the end of these tests prevents prank state leakage into subsequent tests, which is good practice.Also applies to: 290-291
test/mainnet-fork/Curve.t.sol (2)
130-131: LGTM: Proper prank scope management.The added
vm.stopPrank()calls correctly terminate prank contexts that were previously left open, ensuring clean test isolation.Also applies to: 157-157, 196-196, 218-218, 338-338, 393-393, 461-461, 486-486, 910-910, 1003-1003
675-680: LGTM: Correct use of per-call pranks.Replacing
vm.startPrank(relayer)with separatevm.prank(relayer)calls is appropriate here since each call needs independent relayer context for the boundary test pattern (revert check followed by success check).Also applies to: 689-694, 714-719
test/base-fork/MorphoAllocations.t.sol (1)
222-222: LGTM: Exchange rate configuration correctly replaces slippage.The API change from
setMaxSlippagetosetMaxExchangeRateproperly configures per-token exchange-rate ceilings usingmorphoVault.convertToAssets(2_000_000e6)to derive the maximum expected asset value for the given shares.test/mainnet-fork/Ethena.t.sol (1)
664-664: LGTM: Exchange rate ceiling properly configured.The conversion from slippage to exchange-rate gating correctly uses
susde.convertToAssets(1.2e18)to set a 20% exchange-rate ceiling, which is appropriate for protecting against adverse rate movements during deposits.test/mainnet-fork/LayerZero.t.sol (3)
76-78: LGTM: Correct prank ordering for revert tests.Moving
vm.prank(relayer)beforevm.expectRevertensures the prank context is active when the reverting call executes, which is the correct pattern for access-control and rate-limit revert tests.Also applies to: 390-391
118-126: LGTM: Proper per-call prank usage for boundary tests.Using separate
vm.prank(relayer)calls for the failure and success boundary checks is correct, as eachvm.prank()only affects the immediately following external call.Also applies to: 435-443
213-214: LGTM: Clean prank scope termination.The added
vm.stopPrank()calls properly close the prank contexts initiated earlier in the tests, ensuring test isolation.Also applies to: 533-534
src/ForeignController.sol (4)
53-53: LGTM: Well-structured public API additions.The new event, constant, and mapping follow Solidity conventions and provide a clear public interface for per-token exchange-rate gating.
Also applies to: 61-61, 90-91
163-174: LGTM: Admin function correctly implements exchange-rate configuration.The
setMaxExchangeRatefunction properly:
- Enforces admin-only access via
_checkRole- Validates non-zero token address
- Computes and stores the exchange rate using
_getExchangeRate- Emits the appropriate event
The nonReentrant modifier ensures safe state updates.
352-371: LGTM: Deposit flow correctly validates exchange rate.The modified
depositERC4626function properly:
- Retrieves the configured max exchange rate
- Reverts if not set (
maxExchangeRate == 0)- Approves the correct asset via
IERC4626(token).asset()- Validates that the actual exchange rate doesn't exceed the ceiling
The check
_getExchangeRate(shares, amount) <= maxExchangeRatecorrectly protects against unfavorable exchange rates during deposit.
612-620: LGTM: Exchange rate calculation correctly handles edge cases.The
_getExchangeRateimplementation properly addresses the division-by-zero concerns raised in past reviews:
- Zero assets first (line 614): Returns 0 when assets is 0, correctly handling the valid case of 0 shares and 0 assets
- Zero shares with non-zero assets (line 617): Reverts with a clear error message, as this represents an infinite/undefined exchange rate
- Normal case (line 619): Calculates
(EXCHANGE_RATE_PRECISION * assets) / sharessafelyThis three-step guard ensures all edge cases are handled correctly.
test/mainnet-fork/Maple.t.sol (4)
59-59: LGTM: Exchange rate configuration replaces slippage check.The API change correctly configures a 20% exchange-rate ceiling using
syrup.convertToAssets(1.2e18)for the Maple syrup vault.
121-127: LGTM: Enhanced boundary test coverage.Adding the successful call at the boundary after the revert check improves test coverage by verifying both sides of the rate-limit boundary.
129-155: LGTM: Comprehensive exchange-rate validation tests.The two new tests provide excellent coverage:
test_depositERC4626_maple_maxExchangeRateNotSet: Verifies that deposits revert when no exchange-rate ceiling is configuredtest_depositERC4626_maple_exchangeRateTooHigh: Tests the boundary behavior when the exchange rate is too high, then confirms success when the ceiling is raisedBoth tests follow the proper pattern of setup → failure check → adjustment → success check.
233-238: LGTM: Consistent prank management.The changes properly use separate
vm.prank(relayer)calls for boundary tests and addvm.stopPrank()to terminate extended prank contexts, maintaining test isolation.Also applies to: 307-324
test/base-fork/Morpho.t.sol (5)
105-106: LGTM: Exchange rate configuration for both Morpho vaults.Both USDS and USDC vaults are correctly configured with exchange-rate ceilings using their respective
convertToAssets(1e18)calls, replacing the previous slippage-based approach.
165-172: LGTM: Test coverage for missing exchange-rate configuration.The new
test_morpho_deposit_maxExchangeRateNotSettest correctly verifies that deposits revert with"ForeignController/max-exchange-rate-not-set"when no exchange-rate ceiling is configured.
174-191: LGTM: Comprehensive exchange-rate boundary test.The
test_depositERC4626_exchangeRateBoundarytest properly:
- Sets the max exchange rate just below the current rate
- Verifies the deposit reverts with
"ForeignController/exchange-rate-too-high"- Raises the ceiling to exactly the current rate
- Confirms the deposit succeeds
This thoroughly validates the boundary enforcement logic.
411-415: LGTM: More precise boundary calculations.Using
vault.convertToShares()andvault.previewRedeem()instead of direct calculations provides more accurate boundary values that account for the vault's actual rounding behavior. This improves test precision for rate-limit boundary checks.Also applies to: 432-436
146-151: LGTM: Consistent prank management throughout.All prank scope changes properly:
- Use
vm.prank(relayer)for single-call contexts- Use
vm.startPrank(relayer)/vm.stopPrank()pairs for multi-call contexts- Ensure pranks are active before reverting calls
This maintains clean test isolation and correct access-control context.
Also applies to: 157-162, 258-268, 273-283, 393-401, 406-422, 427-443
test/mainnet-fork/4626Calls.t.sol (6)
18-19: Good refactoring of rate limit keys to state variables.Moving these to state variables avoids redundant key generation across multiple test functions, improving maintainability.
21-32: Well-structured setUp for exchange rate testing.The setUp properly configures:
- Rate limit keys as state variables (lines 24-25)
- USDS mint rate limit to support test flows (line 28)
- Exchange rate ceiling with appropriate headroom for rounding (line 31)
The 1.2 multiplier on line 31 provides ~20% headroom above the current rate (~1.003), which appropriately accommodates rounding errors as noted in past reviews.
89-96: Good coverage for unconfigured exchange rate.This test ensures the system properly reverts when attempting deposits without a configured exchange rate ceiling, preventing unintentional bypasses.
98-116: Effective boundary testing for exchange rate ceiling.The test correctly verifies the boundary condition:
- At exactly the current rate (line 103): deposit fails
- Just above the current rate (line 111): deposit succeeds
This confirms the check uses strict inequality (rate > max), ensuring deposits at or above the ceiling are rejected.
183-199: Correct adjustment of test setup for rate limit validation.The test properly increases the deposit limit to 10M (lines 184-186) to enable depositing sufficient sUSDS for testing the 5M withdraw boundary. The added
vm.stopPrank()properly closes the prank context.
301-323: Consistent pattern for redeem rate limit testing.Mirrors the withdraw test adjustments: increases deposit limit to 10M (lines 302-304) and properly closes the prank context (lines 321-322). The test flow correctly validates the 5M redeem boundary.
| vm.startPrank(relayer); | ||
|
|
||
| mainnetController.depositToFarm(farm, 1_000_000e18); | ||
|
|
||
| vm.expectRevert("RateLimits/rate-limit-exceeded"); | ||
| mainnetController.withdrawFromFarm(farm, 1_000_000e18 + 1); | ||
|
|
||
| mainnetController.withdrawFromFarm(farm, 1_000_000e18); | ||
|
|
||
| vm.stopPrank(); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider per-call pranks for consistency.
The explicit vm.stopPrank() is good practice. However, this test uses a prank block while the structurally similar test_depositToFarm_rateLimitsBoundary (lines 81-86) uses per-call vm.prank(relayer) for each operation. For consistency and clarity, consider refactoring to match that pattern:
- vm.startPrank(relayer);
-
+ vm.prank(relayer);
mainnetController.depositToFarm(farm, 1_000_000e18);
+ vm.prank(relayer);
vm.expectRevert("RateLimits/rate-limit-exceeded");
mainnetController.withdrawFromFarm(farm, 1_000_000e18 + 1);
+ vm.prank(relayer);
mainnetController.withdrawFromFarm(farm, 1_000_000e18);
-
- vm.stopPrank();📝 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.
| vm.startPrank(relayer); | |
| mainnetController.depositToFarm(farm, 1_000_000e18); | |
| vm.expectRevert("RateLimits/rate-limit-exceeded"); | |
| mainnetController.withdrawFromFarm(farm, 1_000_000e18 + 1); | |
| mainnetController.withdrawFromFarm(farm, 1_000_000e18); | |
| vm.stopPrank(); | |
| vm.prank(relayer); | |
| mainnetController.depositToFarm(farm, 1_000_000e18); | |
| vm.prank(relayer); | |
| vm.expectRevert("RateLimits/rate-limit-exceeded"); | |
| mainnetController.withdrawFromFarm(farm, 1_000_000e18 + 1); | |
| vm.prank(relayer); | |
| mainnetController.withdrawFromFarm(farm, 1_000_000e18); |
🤖 Prompt for AI Agents
In test/mainnet-fork/Farm.t.sol around lines 155 to 164, the test currently uses
vm.startPrank(relayer) / vm.stopPrank() but similar tests use per-call
vm.prank(relayer); refactor to replace the prank block with individual
vm.prank(relayer) calls immediately before each interaction (depositToFarm,
expectRevert/withdrawFromFarm failing call, and the successful withdrawFromFarm)
so each call is executed under a per-call prank and remove the
vm.startPrank/vm.stopPrank pair.
There was a problem hiding this comment.
@coderabbitai Agreed, please make an issue to track the eventual refactor we would need to follow this practice throughout the tests. We will not address this in this PR.
There was a problem hiding this comment.
@deluca-mike, I'll be happy to create an issue to track this refactor. Which issue tracking platform would you prefer for this?
Options:
- GitHub - Issue will be created in the spark-alm-controller repository
- Linear - Issue will be created in team SC
Please let me know your preference, and I'll create the issue with:
- A clear title describing the refactor goal
- Context explaining the need for consistent per-call prank usage in tests
- A backlink to this PR (feat: ERC4626 Exchange Rate Threshold (SC-1229) #186) and this discussion
- Assignment to you (deluca-mike)
🧠 Learnings used
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.
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`.
There was a problem hiding this comment.
🧩 Analysis chain
@deluca-mike, I'll create the GitHub issue now.
🏁 Script executed:
#!/bin/bash
gh issue create \
--repo sparkdotfi/spark-alm-controller \
--title "Refactor tests to use per-call vm.prank() for consistency" \
--body "## Context
Several test files in the codebase use a mix of prank patterns:
- Some tests use \`vm.startPrank(relayer)\` / \`vm.stopPrank()\` blocks
- Other structurally similar tests use per-call \`vm.prank(relayer)\` before each interaction
For consistency and clarity, we should standardize on the per-call pattern throughout the test suite. Per-call pranks make it explicit which exact calls are executed under which identity, improving test readability and reducing the risk of unintended side effects from lingering prank contexts.
## Example
In \`test/mainnet-fork/Farm.t.sol\`, the \`test_withdrawFromFarm_rateLimitsBoundary\` test uses a prank block, while the structurally similar \`test_depositToFarm_rateLimitsBoundary\` uses per-call pranks.
## Scope
Review all test files and refactor prank blocks to per-call pranks where appropriate for consistency.
## References
- PR: #186
- Discussion: https://github.com/sparkdotfi/spark-alm-controller/pull/186#discussion_r2504521063
- Requested by: @deluca-mike" \
--assignee deluca-mike
Length of output: 61
@deluca-mike, I've created GitHub issue #187 to track the refactor for consistent per-call prank usage throughout the test suite. The issue has been assigned to you and includes:
- Context explaining the inconsistency
- An example from the Farm.t.sol test file
- Backlinks to this PR (feat: ERC4626 Exchange Rate Threshold (SC-1229) #186) and discussion
- Scope for the refactor work
🧠 Learnings used
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.
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`.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (18)
src/ForeignController.sol(13 hunks)src/MainnetController.sol(18 hunks)src/libraries/CCTPLib.sol(2 hunks)src/libraries/CurveLib.sol(6 hunks)test/base-fork/Aave.t.sol(3 hunks)test/base-fork/Morpho.t.sol(4 hunks)test/base-fork/MorphoAllocations.t.sol(4 hunks)test/base-fork/TransferAsset.t.sol(2 hunks)test/mainnet-fork/4626Calls.t.sol(5 hunks)test/mainnet-fork/Aave.t.sol(7 hunks)test/mainnet-fork/Approve.t.sol(2 hunks)test/mainnet-fork/CCTPCalls.t.sol(10 hunks)test/mainnet-fork/Curve.t.sol(19 hunks)test/mainnet-fork/ERC4626DonationAttack.t.sol(1 hunks)test/mainnet-fork/Maple.t.sol(6 hunks)test/mainnet-fork/OTCSwaps.t.sol(12 hunks)test/mainnet-fork/TransferAsset.t.sol(2 hunks)test/unit/controllers/Admin.t.sol(23 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/base-fork/TransferAsset.t.soltest/mainnet-fork/Approve.t.soltest/mainnet-fork/TransferAsset.t.soltest/mainnet-fork/Curve.t.soltest/mainnet-fork/Aave.t.soltest/base-fork/MorphoAllocations.t.soltest/mainnet-fork/OTCSwaps.t.soltest/mainnet-fork/ERC4626DonationAttack.t.soltest/base-fork/Aave.t.soltest/mainnet-fork/Maple.t.soltest/mainnet-fork/4626Calls.t.soltest/base-fork/Morpho.t.soltest/mainnet-fork/CCTPCalls.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/base-fork/TransferAsset.t.soltest/mainnet-fork/Approve.t.soltest/mainnet-fork/TransferAsset.t.soltest/mainnet-fork/Curve.t.soltest/mainnet-fork/Aave.t.soltest/base-fork/MorphoAllocations.t.soltest/mainnet-fork/OTCSwaps.t.soltest/mainnet-fork/ERC4626DonationAttack.t.soltest/base-fork/Aave.t.solsrc/MainnetController.soltest/mainnet-fork/4626Calls.t.soltest/base-fork/Morpho.t.soltest/mainnet-fork/CCTPCalls.t.soltest/unit/controllers/Admin.t.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/Approve.t.soltest/mainnet-fork/TransferAsset.t.soltest/mainnet-fork/ERC4626DonationAttack.t.soltest/mainnet-fork/Maple.t.soltest/mainnet-fork/4626Calls.t.soltest/base-fork/Morpho.t.soltest/unit/controllers/Admin.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/TransferAsset.t.soltest/mainnet-fork/Aave.t.soltest/mainnet-fork/OTCSwaps.t.solsrc/libraries/CCTPLib.solsrc/ForeignController.solsrc/MainnetController.soltest/mainnet-fork/4626Calls.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: test
- GitHub Check: coverage
- GitHub Check: build
🔇 Additional comments (22)
src/libraries/CCTPLib.sol (1)
54-54: LGTM! Error prefix standardization is complete and consistent across the codebase.The error message prefix has been correctly shortened from
"MainnetController/"to"MC/". Verification confirms all similar prefixes have been consistently updated: zero remaining instances of the old prefix pattern exist in the codebase, and the new"MC/"prefix is used uniformly across src/MainnetController.sol, src/libraries/CurveLib.sol, src/libraries/CCTPLib.sol (including line 54), and all corresponding test files.test/mainnet-fork/TransferAsset.t.sol (2)
62-68: LGTM: Improved test precision with individual vm.prank calls.Replacing the shared
vm.startPrank(relayer)context with individualvm.prank(relayer)calls before eachtransferAssetinvocation improves test clarity and precision. This is a best practice for Foundry tests.
90-90: No action required—revert message is consistent.The test expectation matches the source code: both use
"MC/transfer-failed"at test/mainnet-fork/TransferAsset.t.sol:90 and src/MainnetController.sol:1079.test/base-fork/Aave.t.sol (3)
85-85: LGTM: Revert message prefix shortened.The error message prefixes have been shortened from "ForeignController/" to "FC/" for consistency with the PR's standardized error message format.
Also applies to: 110-110
89-98: LGTM: Cleaner prank usage.The refactoring to use
vm.prank(relayer)for individual operations instead of persistent prank context improves test clarity by making authorization explicit for each call.
187-187: LGTM: Proper prank cleanup.The addition of
vm.stopPrank()correctly terminates the prank context started at line 181, ensuring proper test isolation.test/mainnet-fork/CCTPCalls.t.sol (2)
59-158: Shortcode revert strings align with controller updates.Per-call
vm.prank(relayer)plus the newRateLimits/...andMC/domain-not-configuredexpectations keep the boundary checks and domain wiring assertions in sync with the latest controller logic. Nicely tightened coverage.
351-449: Foreign controller failure paths stay in lockstep.Mirroring the shortcode revert strings and single-call pranks on the foreign side keeps both controller suites verifying the same rate-limit and domain-config guarantees after the ERC4626 changes.
test/mainnet-fork/Aave.t.sol (2)
115-132: LGTM! Improved test isolation with per-call pranks.The change from persistent
vm.startPrank(relayer)to per-callvm.prank(relayer)improves test clarity and reduces the risk of unintended side effects from lingering prank contexts. The pattern is consistently applied across both USDS and USDC rate limit boundary tests.
275-277: Good addition of vm.stopPrank() for cleanup.Adding
vm.stopPrank()at the end of these withdraw tests ensures the prank context is properly terminated, which is good practice for test hygiene.Also applies to: 290-292
test/mainnet-fork/Maple.t.sol (2)
59-59: LGTM! Exchange rate ceiling properly configured.The exchange rate is set using
syrup.convertToAssets(1.2e18)as the maximum, which allows up to a 20% exchange rate increase. This provides reasonable protection against donation attacks while allowing for normal yield accrual.
129-155: Excellent test coverage for exchange rate validation.The new test cases thoroughly validate the exchange-rate gating mechanism:
test_depositERC4626_maple_maxExchangeRateNotSet: Ensures deposits fail when no ceiling is configuredtest_depositERC4626_maple_exchangeRateTooHigh: Tests both the failure case (rate too high) and success case (rate within bounds) with appropriate boundariesThis comprehensive coverage helps prevent regressions in the exchange-rate protection logic.
test/mainnet-fork/Approve.t.sol (1)
265-265: LGTM! Consistent error message prefixes.The revert message updates align with the broader standardization of error prefixes across the codebase (MC/ for MainnetController, FC/ for ForeignController), improving consistency and reducing verbosity.
Also applies to: 288-288
src/libraries/CurveLib.sol (1)
80-240: LGTM! Consistent error prefix migration.All error messages have been updated from the verbose "MainnetController/" prefix to the more concise "MC/" prefix. This change improves readability and reduces gas costs for error strings while maintaining clear error identification. No logic changes were made.
test/base-fork/TransferAsset.t.sol (2)
62-67: LGTM! Consistent prank pattern update.The change from a persistent
vm.startPrank(relayer)to per-callvm.prank(relayer)invocations aligns with the broader test pattern improvements across the PR, improving test isolation.
90-90: LGTM! Error prefix standardization.Updated revert message from "ForeignController/transfer-failed" to "FC/transfer-failed" for consistency with the new error prefix convention.
test/mainnet-fork/Curve.t.sol (2)
108-665: LGTM! Comprehensive error message standardization.All revert expectations have been updated from "MainnetController/..." to "MC/..." prefixes throughout the Curve tests. The changes are consistent and maintain the same test logic while improving error message conciseness.
131-132: Good addition of vm.stopPrank() calls.The added
vm.stopPrank()calls properly clean up prank contexts after test execution, improving test hygiene and preventing potential state leakage between tests.Also applies to: 157-158, 196-197, 218-219, 338-339, 393-394, 461-462, 486-487, 910-911, 1003-1004
test/base-fork/MorphoAllocations.t.sol (2)
98-98: LGTM! Consistent error prefix for ForeignController.Updated revert messages from "ForeignController/invalid-action" to "FC/invalid-action" across all three test cases, maintaining consistency with the new error prefix convention.
Also applies to: 151-151, 204-204
222-222: LGTM! Exchange rate ceiling properly configured for Morpho vault.The exchange rate is set using
morphoVault.convertToAssets(2_000_000e6)as the maximum for a deposit of 1_000_000e6, effectively allowing up to a 2x exchange rate. This provides reasonable protection while accommodating legitimate yield scenarios in the Morpho vault.test/mainnet-fork/ERC4626DonationAttack.t.sol (2)
88-96: Excellent! Exchange rate ceiling prevents donation attack.The test properly validates that the exchange-rate ceiling mechanism prevents donation attacks:
- Sets a reasonable 20% exchange rate ceiling using
morphoVault.convertToAssets(1.2e18)- Confirms that deposits fail with "MC/exchange-rate-too-high" when the attack inflates the exchange rate
- The updated error message accurately reflects the new validation mechanism
This demonstrates the security improvement from replacing slippage checks with per-token exchange-rate ceilings.
100-103: Good test coverage for attack success scenario.Setting an extremely high exchange rate ceiling (
morphoVault.convertToAssets(1e24)) properly demonstrates that insufficient protection allows the donation attack to succeed, validating that the exchange-rate ceiling is the control preventing the attack.
| event MaxSlippageSet(address indexed pool, uint256 maxSlippage); | ||
| event MaxExchangeRateSet(address indexed token, uint256 maxExchangeRate); |
src/ForeignController.sol
Outdated
| _approve(IERC4626(token).asset(), token, amount); | ||
|
|
||
| // Deposit asset into the token, proxy receives token shares, decode the resulting shares. | ||
| // Deposit asset into the token, proxy receives token shares, decode the resulting shares |
There was a problem hiding this comment.
Shouldn't remove period
| "ForeignController/inflated-shares" | ||
| _getExchangeRate(shares, amount) <= maxExchangeRate, | ||
| "FC/exchange-rate-too-high" | ||
| ); |
There was a problem hiding this comment.
maxExchangeRate inline
src/ForeignController.sol
Outdated
|
|
||
| // Note that whitelist is done by rate limits. | ||
| IERC20 asset = IERC20(IERC4626(token).asset()); | ||
| require(maxExchangeRate != 0, "FC/max-exchange-rate-not-set"); |
src/ForeignController.sol
Outdated
| _getExchangeRate(shares, amount) <= maxExchangeRate, | ||
| "FC/exchange-rate-too-high" | ||
| ); | ||
| require(_getExchangeRate(shares, amount) <= maxExchangeRates[token], "FC/exchange-rate-too-high"); |
| vm.prank(Ethereum.SPARK_PROXY); | ||
| mainnetController.setMaxSlippage(address(morphoVault), 1e18 - 1e4); // Rounding slippage | ||
| vm.startPrank(Ethereum.SPARK_PROXY); | ||
| mainnetController.setMaxExchangeRate(address(morphoVault), morphoVault.convertToShares(1e18), 1.2e18); |
There was a problem hiding this comment.
setMaxExchangeRate(vault, 1e18, 10e18);
| mainnetController.depositERC4626(address(susds), 5_000_000e18); | ||
|
|
||
| vm.stopPrank(); | ||
| } |
There was a problem hiding this comment.
Add small test to do a deposit without an exchange rate set to show failure
| vm.prank(relayer); | ||
| foreignController.depositERC4626(MORPHO_VAULT_USDC, 25_000_000e6); | ||
| } | ||
|
|
There was a problem hiding this comment.
Add small test to do a deposit without an exchange rate set to show failure
There was a problem hiding this comment.
Actionable comments posted: 7
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
src/ForeignController.sol(14 hunks)src/MainnetController.sol(18 hunks)test/base-fork/Morpho.t.sol(4 hunks)test/mainnet-fork/4626Calls.t.sol(5 hunks)test/mainnet-fork/ERC4626DonationAttack.t.sol(1 hunks)test/mainnet-fork/Maple.t.sol(6 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 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/ERC4626DonationAttack.t.soltest/base-fork/Morpho.t.solsrc/MainnetController.soltest/mainnet-fork/Maple.t.soltest/mainnet-fork/4626Calls.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/ERC4626DonationAttack.t.soltest/base-fork/Morpho.t.soltest/mainnet-fork/Maple.t.soltest/mainnet-fork/4626Calls.t.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/ERC4626DonationAttack.t.soltest/base-fork/Morpho.t.soltest/mainnet-fork/Maple.t.soltest/mainnet-fork/4626Calls.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:
src/MainnetController.soltest/mainnet-fork/4626Calls.t.solsrc/ForeignController.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 (12)
test/mainnet-fork/ERC4626DonationAttack.t.sol (1)
95-95: LGTM! Revert message correctly updated.The change from "MainnetController/slippage-too-high" to "MC/exchange-rate-too-high" correctly reflects the new exchange-rate-based validation and the updated error prefix convention.
src/ForeignController.sol (2)
163-174: LGTM: Exchange rate setter correctly implemented.The function properly validates the token address, uses role-based access control, and computes the exchange rate using the helper function. The ability to set a zero exchange rate (when
maxExpectedAssets = 0) is intentional and allows pausing deposits for a token.
608-616: LGTM: Exchange rate calculation correctly handles edge cases.The function properly handles all scenarios:
- Returns 0 for zero assets (valid case where both shares and assets are zero)
- Reverts for zero shares with non-zero assets (infinite/invalid exchange rate)
- Safely calculates the exchange rate otherwise
This implementation aligns with the discussion in past review comments and prevents division-by-zero errors.
test/mainnet-fork/Maple.t.sol (1)
129-157: LGTM: Comprehensive test coverage for exchange rate validation.The new tests effectively validate:
- Deposits are blocked when the actual exchange rate exceeds the configured maximum
- Deposits succeed when the exchange rate is adjusted to acceptable levels
- Zero exchange rate configuration blocks all deposits
The test structure properly uses prank contexts and verifies both failure and success scenarios.
src/MainnetController.sol (2)
297-308: LGTM: Exchange rate setter correctly implemented.The function properly validates the token address, enforces role-based access control, and computes the exchange rate using the helper function. Implementation is consistent with
ForeignController.
1123-1131: LGTM: Exchange rate calculation correctly handles edge cases.The function properly handles all scenarios:
- Returns 0 for zero assets (valid case where both shares and assets are zero)
- Reverts for zero shares with non-zero assets (infinite/invalid exchange rate)
- Safely calculates the exchange rate otherwise
Implementation is consistent with
ForeignControllerand aligns with past review discussions.test/base-fork/Morpho.t.sol (2)
105-106: LGTM: Proper exchange rate configuration in test setup.The setup correctly configures exchange rates for both Morpho vaults using the vault-specific
convertToSharesfunctions, ensuring that deposits at 1:1 exchange rate will be accepted. This properly aligns with the new exchange rate validation logic.
165-182: LGTM: Comprehensive exchange rate boundary test.The test effectively validates:
- Deposits are blocked when the exchange rate exceeds the configured maximum
- Deposits succeed when the exchange rate is set to an acceptable level
- Proper use of vault-specific conversion functions for boundary calculations
Test structure follows established patterns with appropriate prank contexts.
test/mainnet-fork/4626Calls.t.sol (4)
18-31: LGTM - Clean refactoring of test setup.The refactoring to make
depositKeyandwithdrawKeyavailable as state variables is a good improvement for test reusability. The rate limit configurations and max exchange rate setup are properly scoped toSPARK_PROXY.
76-87: LGTM - Correct rate limit boundary testing.The test properly validates the rate limit boundary by attempting to exceed it (expecting revert) and then depositing exactly at the limit (expecting success). The addition of
vm.stopPrank()improves code clarity.
186-202: LGTM - Proper test setup for higher withdrawal limits.The addition of deposit rate limit configuration is necessary to support depositing 10M USDS before testing the 5M withdrawal boundary. The test correctly validates the rate limit boundary conditions.
304-326: LGTM - Consistent test setup with withdraw boundary test.The test mirrors the withdrawal boundary test pattern by increasing the deposit limit to enable larger deposits before testing the redeem functionality. The rate limit boundary validation is correct.
| require( | ||
| IERC4626(token).convertToAssets(shares) >= amount * maxSlippages[token] / 1e18, | ||
| "ForeignController/inflated-shares" | ||
| _getExchangeRate(shares, amount) <= maxExchangeRates[token], | ||
| "FC/exchange-rate-too-high" | ||
| ); |
There was a problem hiding this comment.
Add validation for configured exchange rate.
The function doesn't check whether maxExchangeRates[token] is non-zero before comparison. If the exchange rate hasn't been set (equals zero), deposits will always fail with "FC/exchange-rate-too-high" instead of a clearer error. This mirrors the pattern used in depositAave at line 427.
Based on past review comments requesting this check (see MainnetController line 459 comment by supercontracts), apply this diff:
shares = abi.decode(
proxy.doCall(
token,
abi.encodeCall(IERC4626(token).deposit, (amount, address(proxy)))
),
(uint256)
);
+ require(maxExchangeRates[token] != 0, "FC/max-exchange-rate-not-set");
+
require(
_getExchangeRate(shares, amount) <= maxExchangeRates[token],
"FC/exchange-rate-too-high"
);🤖 Prompt for AI Agents
In src/ForeignController.sol around lines 364 to 367, add a validation that
maxExchangeRates[token] is non-zero before comparing it to the computed exchange
rate. Specifically, insert a require(maxExchangeRates[token] != 0,
"FC/exchange-rate-not-configured") (matching the depositAave pattern at line
427) immediately before the existing require that checks
_getExchangeRate(shares, amount) <= maxExchangeRates[token], so an unset rate
fails with a clear error instead of "FC/exchange-rate-too-high".
| require( | ||
| IERC4626(token).convertToAssets(shares) >= amount * maxSlippages[token] / 1e18, | ||
| "MainnetController/slippage-too-high" | ||
| _getExchangeRate(shares, amount) <= maxExchangeRates[token], | ||
| "MC/exchange-rate-too-high" | ||
| ); |
There was a problem hiding this comment.
Add validation for configured exchange rate.
The function doesn't check whether maxExchangeRates[token] is non-zero before the comparison. If the exchange rate hasn't been configured (equals zero), deposits will always fail with "MC/exchange-rate-too-high" instead of a clearer error message.
Based on past review comments explicitly requesting this check (see comment by supercontracts at line 459), apply this diff:
shares = abi.decode(
proxy.doCall(
token,
abi.encodeCall(IERC4626(token).deposit, (amount, address(proxy)))
),
(uint256)
);
+ require(maxExchangeRates[token] != 0, "MC/max-exchange-rate-not-set");
+
require(
_getExchangeRate(shares, amount) <= maxExchangeRates[token],
"MC/exchange-rate-too-high"
);📝 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.
| require( | |
| IERC4626(token).convertToAssets(shares) >= amount * maxSlippages[token] / 1e18, | |
| "MainnetController/slippage-too-high" | |
| _getExchangeRate(shares, amount) <= maxExchangeRates[token], | |
| "MC/exchange-rate-too-high" | |
| ); | |
| shares = abi.decode( | |
| proxy.doCall( | |
| token, | |
| abi.encodeCall(IERC4626(token).deposit, (amount, address(proxy))) | |
| ), | |
| (uint256) | |
| ); | |
| require(maxExchangeRates[token] != 0, "MC/max-exchange-rate-not-set"); | |
| require( | |
| _getExchangeRate(shares, amount) <= maxExchangeRates[token], | |
| "MC/exchange-rate-too-high" | |
| ); |
🤖 Prompt for AI Agents
In src/MainnetController.sol around lines 473 to 476, add a validation that
maxExchangeRates[token] is non-zero before comparing it to the computed exchange
rate: require(maxExchangeRates[token] != 0, "MC/exchange-rate-not-configured");
then perform the existing compare require(_getExchangeRate(shares, amount) <=
maxExchangeRates[token], "MC/exchange-rate-too-high"); this ensures an explicit,
clearer revert when the exchange rate for the token hasn't been configured.
| function test_morpho_usdc_deposit_zeroExchangeRate() external { | ||
| deal(Base.USDS, address(almProxy), 25_000_000e18); | ||
|
|
||
| vm.prank(Base.SPARK_EXECUTOR); | ||
| foreignController.setMaxSlippage(MORPHO_VAULT_USDS, 1e18); | ||
| foreignController.setMaxExchangeRate(MORPHO_VAULT_USDS, 0, 0); | ||
|
|
||
| vm.prank(relayer); | ||
| foreignController.depositERC4626(MORPHO_VAULT_USDS, 5_000_000e18); | ||
| vm.expectRevert("FC/exchange-rate-too-high"); | ||
| foreignController.depositERC4626(MORPHO_VAULT_USDS, 1e18); | ||
| } |
There was a problem hiding this comment.
Fix test function naming inconsistency.
The test is named test_morpho_usdc_deposit_zeroExchangeRate but it deals USDS tokens (line 185), configures the USDS vault exchange rate (line 188), and deposits to the USDS vault (line 192). The test logic is correct, but the name is misleading.
Apply this diff to correct the function name:
- function test_morpho_usdc_deposit_zeroExchangeRate() external {
+ function test_morpho_usds_deposit_zeroExchangeRate() external {
deal(Base.USDS, address(almProxy), 25_000_000e18);🤖 Prompt for AI Agents
In test/base-fork/Morpho.t.sol around lines 184 to 193 the test function name
references "usdc" but the body uses USDS (deals Base.USDS, sets
MORPHO_VAULT_USDS exchange rate and deposits to MORPHO_VAULT_USDS); rename the
function to match the token (for example change
test_morpho_usdc_deposit_zeroExchangeRate to
test_morpho_usds_deposit_zeroExchangeRate) so the name reflects USDS and update
any references to the function within the file if present.
| function test_depositERC4626_exchangeRateBoundary() external { | ||
| vm.prank(relayer); | ||
| mainnetController.mintUSDS(5_000_000e18); | ||
|
|
||
| vm.prank(Ethereum.SPARK_PROXY); | ||
| mainnetController.setMaxSlippage(address(susds), 1e18); | ||
| vm.startPrank(Ethereum.SPARK_PROXY); | ||
| mainnetController.setMaxExchangeRate(address(susds), susds.convertToShares(5_000_000e18), 5_000_000e18 - 1); | ||
| vm.stopPrank(); | ||
|
|
||
| vm.prank(relayer); | ||
| vm.expectRevert("MainnetController/slippage-too-high"); | ||
| mainnetController.depositERC4626(address(susds), 5_000_000e18); // Rounding causes error | ||
| vm.expectRevert("MC/exchange-rate-too-high"); | ||
| mainnetController.depositERC4626(address(susds), 5_000_000e18); | ||
|
|
||
| vm.startPrank(Ethereum.SPARK_PROXY); | ||
| mainnetController.setMaxExchangeRate(address(susds), susds.convertToShares(5_000_000e18), 5_000_000e18); | ||
| vm.stopPrank(); | ||
|
|
||
| vm.prank(relayer); | ||
| mainnetController.depositERC4626(address(susds), 5_000_000e18); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider simplifying prank pattern for single operations.
The exchange rate boundary logic is correct and properly tests the edge cases. However, for single operations like lines 93-95 and 101-103, using vm.prank() instead of vm.startPrank()/vm.stopPrank() would be more concise and consistent with other single-operation pranks (e.g., lines 97, 105).
Example refactor for lines 93-95:
-vm.startPrank(Ethereum.SPARK_PROXY);
-mainnetController.setMaxExchangeRate(address(susds), susds.convertToShares(5_000_000e18), 5_000_000e18 - 1);
-vm.stopPrank();
+vm.prank(Ethereum.SPARK_PROXY);
+mainnetController.setMaxExchangeRate(address(susds), susds.convertToShares(5_000_000e18), 5_000_000e18 - 1);Apply similarly to lines 101-103.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In test/mainnet-fork/4626Calls.t.sol around lines 89 to 107, replace the two
single-operation vm.startPrank(...)/vm.stopPrank() blocks (currently wrapping
mainnetController.setMaxExchangeRate calls at lines ~93-95 and ~101-103) with
vm.prank(<same-address>); followed by the single setMaxExchangeRate call to make
the code concise and consistent with the other single-operation pranks in the
file; ensure you preserve the same caller address (Ethereum.SPARK_PROXY) and the
exact arguments to setMaxExchangeRate.
| function test_depositERC4626_zeroExchangeRate() external { | ||
| vm.prank(relayer); | ||
| mainnetController.mintUSDS(5_000_000e18); | ||
|
|
||
| vm.prank(Ethereum.SPARK_PROXY); | ||
| mainnetController.setMaxSlippage(address(susds), 1e18 - 1); | ||
| mainnetController.setMaxExchangeRate(address(susds), 0, 0); | ||
|
|
||
| vm.prank(relayer); | ||
| vm.expectRevert("MC/exchange-rate-too-high"); | ||
| mainnetController.depositERC4626(address(susds), 5_000_000e18); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider renaming test for clarity.
The test name test_depositERC4626_zeroExchangeRate could be more descriptive. Since this test validates behavior when the max exchange rate is not configured (set to 0), consider renaming it to test_depositERC4626_maxExchangeRateNotSet to better describe the failure condition being tested.
This would also better address the past review comment requesting "a small test to do a deposit without an exchange rate set to show failure."
🤖 Prompt for AI Agents
In test/mainnet-fork/4626Calls.t.sol around lines 109 to 119, rename the test
function from test_depositERC4626_zeroExchangeRate to
test_depositERC4626_maxExchangeRateNotSet to make the intent clearer; update the
function declaration and any references (if any) to the old name in the file so
the test suite compiles and continues to assert
vm.expectRevert("MC/exchange-rate-too-high") for the case when max exchange rate
is zero/not set.
| vm.startPrank(Ethereum.SPARK_PROXY); | ||
| mainnetController.setMaxExchangeRate(address(morphoVault), 1e18, 10e18); | ||
| vm.stopPrank(); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using vm.prank for a single call.
The startPrank/stopPrank block is correct but verbose for a single function call. Using vm.prank(Ethereum.SPARK_PROXY) would be more concise and conventional for single-call impersonation.
Apply this diff:
- vm.startPrank(Ethereum.SPARK_PROXY);
- mainnetController.setMaxExchangeRate(address(morphoVault), 1e18, 10e18);
- vm.stopPrank();
+ vm.prank(Ethereum.SPARK_PROXY);
+ mainnetController.setMaxExchangeRate(address(morphoVault), 1e18, 10e18);📝 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.
| vm.startPrank(Ethereum.SPARK_PROXY); | |
| mainnetController.setMaxExchangeRate(address(morphoVault), 1e18, 10e18); | |
| vm.stopPrank(); | |
| vm.prank(Ethereum.SPARK_PROXY); | |
| mainnetController.setMaxExchangeRate(address(morphoVault), 1e18, 10e18); |
🤖 Prompt for AI Agents
In test/mainnet-fork/ERC4626DonationAttack.t.sol around lines 88 to 90, replace
the verbose vm.startPrank(...)/vm.stopPrank() block used for a single call with
the concise vm.prank(Ethereum.SPARK_PROXY) usage: change the block to a single
vm.prank(Ethereum.SPARK_PROXY) call immediately before
mainnetController.setMaxExchangeRate(...), removing startPrank and stopPrank
while keeping the exact function arguments and call order.
| // Set max exchange rate too high | ||
| vm.startPrank(Ethereum.SPARK_PROXY); | ||
| mainnetController.setMaxExchangeRate(address(morphoVault), 1, morphoVault.convertToAssets(1e24)); | ||
| vm.stopPrank(); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Clarify the intentionally high exchange rate threshold.
Two observations:
-
Like the previous test,
startPrank/stopPrankcould be simplified tovm.prankfor a single call. -
Calling
morphoVault.convertToAssets(1e24)when the vault is empty (totalAssets=0, totalSupply=0) relies on implicit 1:1 conversion behavior. For clarity, consider using an explicit large constant to signal the intent of setting an unrealistically high ceiling.
Apply this diff:
- // Set max exchange rate too high
- vm.startPrank(Ethereum.SPARK_PROXY);
- mainnetController.setMaxExchangeRate(address(morphoVault), 1, morphoVault.convertToAssets(1e24));
- vm.stopPrank();
+ // Set max exchange rate unrealistically high to allow attack
+ vm.prank(Ethereum.SPARK_PROXY);
+ mainnetController.setMaxExchangeRate(address(morphoVault), 1, type(uint256).max);📝 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.
| // Set max exchange rate too high | |
| vm.startPrank(Ethereum.SPARK_PROXY); | |
| mainnetController.setMaxExchangeRate(address(morphoVault), 1, morphoVault.convertToAssets(1e24)); | |
| vm.stopPrank(); | |
| // Set max exchange rate unrealistically high to allow attack | |
| vm.prank(Ethereum.SPARK_PROXY); | |
| mainnetController.setMaxExchangeRate(address(morphoVault), 1, type(uint256).max); |
🤖 Prompt for AI Agents
In test/mainnet-fork/ERC4626DonationAttack.t.sol around lines 100 to 103,
replace the vm.startPrank/stopPrank pair with a single vm.prank for the one-off
call and stop relying on morphoVault.convertToAssets(1e24) when the vault is
empty; instead set the max exchange rate ceiling using an explicit large
constant (e.g. a clearly labeled BIG_EXCHANGE_RATE or a hardcoded large uint256)
to make the intentionally unrealistic threshold obvious and avoid implicit 1:1
conversion assumptions.
|
Coverage after merging sc1229-erc4626-exchange-rate-threshold into dev will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Summary by CodeRabbit
New Features
Bug Fixes / UX
Tests