feat: Add Reentrancy Guards (SC-1224) #185
Conversation
deluca-mike
requested review from
hacker-DOM,
lucas-manuel and
supercontracts
WalkthroughAdded OpenZeppelin ReentrancyGuard to MainnetController and ForeignController, applied nonReentrant to many external/admin/relayer/freezer/integration entrypoints, and extended tests and helpers across unit, base-fork, and mainnet-fork suites to validate ReentrancyGuard behavior on success and failure paths. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Controller
participant Guard as ReentrancyGuard
participant Integration as External/Protocol
User->>Controller: call external function(...)
activate Controller
Controller->>Guard: enter (check & set)
alt not reentered
Guard-->>Controller: allowed
Controller->>Integration: perform integration calls / state changes
Integration-->>Controller: return
Controller->>Guard: exit (unset)
Guard-->>Controller: cleared
Controller-->>User: return success
else reentrant
Guard-->>Controller: revert ReentrancyGuardReentrantCall
Controller-->>User: revert
end
deactivate Controller
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (4 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (3)📓 Common learnings📚 Learning: 2025-11-04T17:15:38.751ZApplied to files:
📚 Learning: 2025-11-04T17:15:20.954ZApplied to files:
⏰ 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). (6)
🔇 Additional comments (11)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/mainnet-fork/OTCSwaps.t.sol (1)
105-389: Point helpers atmainnetControllerin these testsRight now
_setControllerEntered()and_assertReeentrancyGuardWrittenToTwice()still targetforeignController, so the guard onmainnetControllerstays_NOT_ENTEREDand the failure-path tests revert withAccessControlUnauthorizedAccountinstead ofReentrancyGuardReentrantCall. Likewise, the success-path assertions look at the wrong contract and read zero writes. After generalizing the helpers, call them with the mainnet controller so these checks exercise the intended storage slot:- _setControllerEntered(); + _setControllerEntered(address(mainnetController)); vm.expectRevert(ReentrancyGuard.ReentrancyGuardReentrantCall.selector); mainnetController.otcSend(exchange, address(1), 1e18);and for the success paths:
- _assertReeentrancyGuardWrittenToTwice(); + _assertReeentrancyGuardWrittenToTwice(address(mainnetController));Apply the same adjustment to each invocation in this file so every test inspects the correct controller.
test/mainnet-fork/Superstate.t.sol (1)
31-129: Use the mainnet controller when forcing/inspecting reentrancy stateThese helpers still touch
foreignController, sotest_subscribeSuperstate_reentrancynever drivesmainnetControllerinto the entered state and the test will revert due to access control. The success test also inspects the wrong contract’s access log. After updating the helpers, make sure the calls here pass the mainnet controller:- _setControllerEntered(); + _setControllerEntered(address(mainnetController)); vm.expectRevert(ReentrancyGuard.ReentrancyGuardReentrantCall.selector); mainnetController.subscribeSuperstate(1_000_000e6);and
- _assertReeentrancyGuardWrittenToTwice(); + _assertReeentrancyGuardWrittenToTwice(address(mainnetController));Update the remaining occurrences in this file as well so the assertions observe the correct storage slot.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (29)
src/ForeignController.sol(19 hunks)src/MainnetController.sol(40 hunks)test/base-fork/Aave.t.sol(5 hunks)test/base-fork/ForkTestBase.t.sol(2 hunks)test/base-fork/Morpho.t.sol(9 hunks)test/base-fork/MorphoAllocations.t.sol(7 hunks)test/base-fork/PsmCalls.t.sol(9 hunks)test/base-fork/SparkVault.t.sol(3 hunks)test/base-fork/TransferAsset.t.sol(3 hunks)test/mainnet-fork/4626Calls.t.sol(7 hunks)test/mainnet-fork/Aave.t.sol(7 hunks)test/mainnet-fork/CCTPCalls.t.sol(6 hunks)test/mainnet-fork/Curve.t.sol(7 hunks)test/mainnet-fork/DaiUsds.t.sol(4 hunks)test/mainnet-fork/Ethena.t.sol(13 hunks)test/mainnet-fork/Farm.t.sol(5 hunks)test/mainnet-fork/ForkTestBase.t.sol(2 hunks)test/mainnet-fork/LayerZero.t.sol(8 hunks)test/mainnet-fork/Maple.t.sol(5 hunks)test/mainnet-fork/OTCSwaps.t.sol(7 hunks)test/mainnet-fork/PsmCalls.t.sol(5 hunks)test/mainnet-fork/SparkVault.t.sol(3 hunks)test/mainnet-fork/Superstate.t.sol(3 hunks)test/mainnet-fork/TransferAsset.t.sol(3 hunks)test/mainnet-fork/VaultCalls.t.sol(4 hunks)test/mainnet-fork/wstETH.t.sol(8 hunks)test/unit/UnitTestBase.t.sol(2 hunks)test/unit/controllers/Admin.t.sol(14 hunks)test/unit/controllers/Freezer.t.sol(5 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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/OTCSwaps.t.soltest/base-fork/SparkVault.t.soltest/mainnet-fork/CCTPCalls.t.soltest/mainnet-fork/SparkVault.t.soltest/mainnet-fork/LayerZero.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/VaultCalls.t.soltest/mainnet-fork/4626Calls.t.soltest/mainnet-fork/CCTPCalls.t.soltest/mainnet-fork/LayerZero.t.soltest/mainnet-fork/PsmCalls.t.solsrc/MainnetController.soltest/mainnet-fork/DaiUsds.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 (51)
test/mainnet-fork/PsmCalls.t.sol (4)
4-5: LGTM! Import added for reentrancy testing.The OpenZeppelin ReentrancyGuard import is correctly added and follows standard Foundry project structure.
141-145: LGTM! Reentrancy test for reverse swap direction.This test correctly validates reentrancy protection for
swapUSDCToUSDS, maintaining consistency with theswapUSDSToUSDCreentrancy test pattern.
15-19: No issues found. Helper function is properly accessible through inheritance.The
_setControllerEntered()helper function is defined intest/mainnet-fork/ForkTestBase.t.solas a virtual function, andPsmCalls.t.solcorrectly imports and inherits from it. The reentrancy test is properly structured and has access to the helper function.
15-19: Test coverage is complete and appropriate; AI summary count was inaccurate.Verification confirms exactly 2 reentrancy tests exist (lines 15 and 141), covering both swap directions. While the AI summary incorrectly claimed 3 tests, the code changes provide appropriate coverage with one reentrancy test per swap function. No issues require addressing.
test/mainnet-fork/wstETH.t.sol (5)
114-118: LGTM - consistent reentrancy test pattern.The test correctly follows the established pattern for verifying reentrancy protection. Ensure the base class helper method concerns raised in earlier comments are addressed.
218-222: LGTM - consistent reentrancy test pattern.The test correctly implements reentrancy protection verification for the claim withdrawal operation. Ensure the base class helper method concerns are addressed.
42-46: Helper method verified in base class.The
_setControllerEntered()method is properly defined intest/mainnet-fork/ForkTestBase.t.sol:330as an internal virtual function. It correctly simulates a reentrant call by usingvm.store()to set the reentrancy guard slot to the entered state, allowing the test to verify thatdepositToWstETH()correctly rejects reentrant calls. The test is correctly implemented.
100-100: ****The method name
_assertReeentrancyGuardWrittenToTwice()is not a typo. This is the actual, intentional method name defined consistently throughout the codebase in multiple base test classes (ForkTestBase.t.sol, UnitTestBase.t.sol, Admin.t.sol) and called across 60+ test locations. The correctly-spelled variation with two 'e's does not exist anywhere in the repository.Likely an incorrect or invalid review comment.
4-4: ****The import path is consistent with the project's established convention. All ReentrancyGuard imports throughout the codebase—including in other test/mainnet-fork files—use the same relative path pattern (
../../lib/openzeppelin-contracts/contracts/utils/ReentrancyGuard.sol). While foundry.toml defines a remapping for@openzeppelin/contracts/, the project deliberately uses relative paths for ReentrancyGuard imports across all files. No change is needed.Likely an incorrect or invalid review comment.
test/mainnet-fork/CCTPCalls.t.sol (1)
6-7: Fix ERC20Mock constructor usage after import change
Switching to the OpenZeppelin mock means its constructor now requires(string name, string symbol, address initialAccount, uint256 initialBalance). OursetUp()still callsnew ERC20Mock()with zero arguments, so the suite no longer compiles. Please instantiate with the required parameters (zero initial supply is fine) so the build succeeds.⛔ Skipped due to learnings
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.test/base-fork/SparkVault.t.sol (3)
4-6: LGTM: Import paths updated correctly.The imports now reference local OpenZeppelin contracts paths, maintaining consistency with the project structure.
74-78: LGTM: Reentrancy test correctly implemented.The test properly sets the controller into a reentrant state using
_setControllerEntered()and expects theReentrancyGuardReentrantCallrevert, validating thattakeFromSparkVaultis protected against reentrancy.
137-142: LGTM: Guard state verification correctly instrumented.The
vm.record()and_assertReeentrancyGuardWrittenToTwice()pattern correctly verifies that the ReentrancyGuard state transitions occur during the operation, confirming the nonReentrant modifier is active.test/mainnet-fork/Curve.t.sol (3)
6-6: LGTM: ReentrancyGuard import added.Import enables reentrancy protection testing for Curve operations.
70-80: LGTM: Comprehensive reentrancy test coverage for Curve operations.All three Curve integration functions (
addLiquidityCurve,removeLiquidityCurve,swapCurve) are correctly tested for reentrancy protection, ensuring the controller cannot be reentered during these operations.Also applies to: 385-395, 591-595
243-252: LGTM: Guard state transitions verified in success tests.The addition of
vm.record()and_assertReeentrancyGuardWrittenToTwice()in success test paths confirms that the ReentrancyGuard state is correctly modified during operations, validating the presence and correct behavior of the nonReentrant modifier.Also applies to: 551-560, 726-731
test/mainnet-fork/SparkVault.t.sol (1)
4-6: LGTM: Mainnet fork reentrancy tests correctly implemented.The mainnet fork version of SparkVault tests mirrors the base-fork implementation with proper reentrancy protection testing and guard state verification.
Also applies to: 69-73, 132-137
test/mainnet-fork/Aave.t.sol (2)
9-9: LGTM: Aave reentrancy protection thoroughly tested.Both
depositAaveandwithdrawAaveoperations are correctly tested for reentrancy protection with proper test structure in failure test contracts.Also applies to: 82-86, 224-228
183-188: LGTM: Guard state verification added to Aave success tests.The instrumentation confirms ReentrancyGuard state transitions during both deposit and withdraw operations.
Also applies to: 206-211, 323-329
test/mainnet-fork/Maple.t.sol (2)
4-4: LGTM: Maple redemption operations protected and tested.Both
requestMapleRedemptionandcancelMapleRedemptionare correctly tested for reentrancy protection.Also applies to: 165-169, 244-248
229-234: LGTM: Success test instrumentation for Maple operations.Guard state verification correctly added to success test paths for Maple redemption operations.
Also applies to: 284-288
test/mainnet-fork/4626Calls.t.sol (2)
4-4: LGTM: Complete ERC4626 reentrancy test coverage.All three ERC4626 operations (
depositERC4626,withdrawERC4626,redeemERC4626) are correctly tested for reentrancy protection.Also applies to: 51-55, 157-161, 256-260
132-137: LGTM: ERC4626 success tests properly instrumented.Guard state verification added to all ERC4626 success test paths.
Also applies to: 227-233, 349-354
test/mainnet-fork/LayerZero.t.sol (3)
8-9: LGTM: LayerZero transfer operations protected across both controllers.Reentrancy tests correctly implemented for both
MainnetControllerandForeignControllerLayerZero transfer operations.Also applies to: 48-52, 359-363
343-345: LGTM: Helper function correctly scoped to foreign controller test base.The
_setControllerEntered()override inArbitrumChainLayerZeroTestBasecorrectly targets theforeignControlleraddress for reentrancy state setup.
190-206: LGTM: LayerZero success tests properly instrumented.Guard state verification added to both mainnet and foreign controller success test paths. Note that Line 524 correctly passes the controller address to the assertion helper.
Also applies to: 508-524
test/mainnet-fork/DaiUsds.t.sol (2)
4-4: LGTM: DAI/USDS swap operations protected in both directions.Both
swapUSDSToDAIandswapDAIToUSDSoperations are correctly tested for reentrancy protection.Also applies to: 10-14, 61-65
41-46: LGTM: Swap success tests properly instrumented.Guard state verification added to both swap direction success tests.
Also applies to: 91-96
test/mainnet-fork/Ethena.t.sol (3)
4-4: LGTM: ReentrancyGuard import added for test assertions.The import enables test cases to reference
ReentrancyGuard.ReentrancyGuardReentrantCall.selectorfor verifying expected reverts.
21-25: LGTM: Reentrancy test correctly verifies guard protection.The test properly simulates an already-entered state and expects the appropriate revert. This pattern is consistently applied across all reentrancy test cases in this file.
49-56: LGTM: Success test correctly validates reentrancy guard storage writes.The use of
vm.record()before the action and_assertReeentrancyGuardWrittenToTwice()after confirms the guard slot is modified twice (entry and exit), verifying the guard is active.test/unit/controllers/Admin.t.sol (5)
4-4: LGTM: ReentrancyGuard import added.
51-57: LGTM: Helper functions properly encapsulate reentrancy guard test setup.The helper functions correctly:
- Set the controller to "entered" state for reentrancy failure tests
- Assert the guard was written to twice for success tests
63-67: LGTM: Comprehensive reentrancy test coverage for admin functions.All admin setters (
setMintRecipient,setLayerZeroRecipient,setMaxSlippage,setOTCBuffer,setOTCRechargeRate,setOTCWhitelistedAsset) have corresponding reentrancy tests following the established pattern.
414-420: LGTM: ForeignController helper functions mirror MainnetController pattern.Consistent implementation across both controller types ensures uniform test coverage.
422-532: LGTM: ForeignController admin functions have reentrancy test coverage.Tests for
setMaxSlippage,setMintRecipient, andsetLayerZeroRecipientfollow the same comprehensive pattern as MainnetController tests.src/MainnetController.sol (15)
7-12: LGTM: Imports updated to use local OpenZeppelin contracts.The change to local lib paths ensures version consistency and eliminates external dependencies.
92-92: LGTM: ReentrancyGuard inheritance properly added.The inheritance order (
ReentrancyGuard, AccessControlEnumerable) ensures reentrancy checks execute before access control checks, which is the correct sequence for defense-in-depth security.
230-236: LGTM: Comprehensive nonReentrant coverage on admin functions.All admin setters now have the
nonReentrantmodifier applied, protecting against reentrancy attacks during administrative operations.
305-337: LGTM: Vault operations protected with nonReentrant.Both
mintUSDSandburnUSDScorrectly apply the guard, protecting critical vault interactions that involve external calls tovault.draw(),vault.wipe(), and token transfers.
359-430: LGTM: wstETH integration functions protected.All three wstETH operations (
depositToWstETH,requestWithdrawFromWstETH,claimWithdrawalFromWstETH) are guarded, protecting complex multi-step operations involving external protocol interactions.
436-506: LGTM: ERC4626 operations comprehensively protected.All three ERC4626 functions (
depositERC4626,withdrawERC4626,redeemERC4626) have reentrancy guards, protecting against potential vault reentrancy during deposit/withdrawal flows.
512-570: LGTM: Aave integration protected.Both
depositAaveandwithdrawAavecorrectly apply the guard, protecting interactions with Aave's lending pool.
576-635: LGTM: Curve operations protected.All three Curve functions (
swapCurve,addLiquidityCurve,removeLiquidityCurve) have reentrancy guards, protecting complex DEX interactions.
641-706: LGTM: Ethena operations comprehensively protected.All Ethena-related functions (
setDelegatedSigner,removeDelegatedSigner,prepareUSDeMint,prepareUSDeBurn,cooldownAssetsSUSDe,cooldownSharesSUSDe,unstakeSUSDe) now have reentrancy guards, protecting the multi-step mint/burn and staking flows.
712-750: LGTM: Maple and Superstate operations protected.All Maple redemption functions and the Superstate subscribe function correctly apply the nonReentrant modifier.
756-814: LGTM: PSM and DaiUsds swap operations protected.All four swap functions (
swapUSDSToDAI,swapDAIToUSDS,swapUSDSToUSDC,swapUSDCToUSDS) have reentrancy guards, with correct modifier ordering (nonReentrant onlyRole(RELAYER)ornonReentrantfollowed by_checkRole(RELAYER)).
819-862: LGTM: LayerZero transfer protected.The
transferTokenLayerZerofunction correctly appliesnonReentrantalong withpayable, protecting cross-chain token transfers.
868-920: LGTM: Bridging and farm operations protected.CCTP bridging (
transferUSDCToCCTP) and SPK farm operations (depositToFarm,withdrawFromFarm) all have reentrancy guards applied.
926-999: LGTM: Spark Vault and OTC operations protected.All functions (
takeFromSparkVault,otcSend,otcClaim) have reentrancy guards. Note that existing comments at lines 967 and 995 mentioned reentrancy was "not relevant" or "not possible," but the guards are applied anyway, which is excellent defense-in-depth practice.
1001-1015: LGTM: View functions correctly excluded from reentrancy guards.The view functions
getOtcClaimWithRechargeandisOtcSwapReadycorrectly do not have thenonReentrantmodifier, as they don't modify state and cannot be vulnerable to reentrancy attacks.
deluca-mike
force-pushed
the
sc-1224-reentrancy-guards
branch
from
f75fdef to
023b5b7
Compare
deluca-mike
force-pushed
the
sc-1224-reentrancy-guards
branch
from
023b5b7 to
1713ec9
Compare
There was a problem hiding this comment.
Actionable comments posted: 10
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (27)
test/base-fork/Aave.t.sol(5 hunks)test/base-fork/ForkTestBase.t.sol(2 hunks)test/base-fork/Morpho.t.sol(13 hunks)test/base-fork/MorphoAllocations.t.sol(7 hunks)test/base-fork/PsmCalls.t.sol(9 hunks)test/base-fork/SparkVault.t.sol(3 hunks)test/base-fork/TransferAsset.t.sol(3 hunks)test/mainnet-fork/4626Calls.t.sol(7 hunks)test/mainnet-fork/Aave.t.sol(7 hunks)test/mainnet-fork/CCTPCalls.t.sol(6 hunks)test/mainnet-fork/Curve.t.sol(7 hunks)test/mainnet-fork/DaiUsds.t.sol(4 hunks)test/mainnet-fork/Ethena.t.sol(13 hunks)test/mainnet-fork/Farm.t.sol(5 hunks)test/mainnet-fork/ForkTestBase.t.sol(2 hunks)test/mainnet-fork/LayerZero.t.sol(8 hunks)test/mainnet-fork/Maple.t.sol(5 hunks)test/mainnet-fork/OTCSwaps.t.sol(7 hunks)test/mainnet-fork/PsmCalls.t.sol(5 hunks)test/mainnet-fork/SparkVault.t.sol(3 hunks)test/mainnet-fork/Superstate.t.sol(3 hunks)test/mainnet-fork/TransferAsset.t.sol(3 hunks)test/mainnet-fork/VaultCalls.t.sol(4 hunks)test/mainnet-fork/wstETH.t.sol(8 hunks)test/unit/UnitTestBase.t.sol(2 hunks)test/unit/controllers/Admin.t.sol(14 hunks)test/unit/controllers/Freezer.t.sol(5 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
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:38.751Z
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`.
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:20.954Z
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.
📚 Learning: 2025-11-04T17:15:20.954Z
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:20.954Z
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/TransferAsset.t.soltest/unit/controllers/Freezer.t.soltest/base-fork/SparkVault.t.soltest/base-fork/Aave.t.soltest/base-fork/PsmCalls.t.soltest/unit/UnitTestBase.t.soltest/mainnet-fork/TransferAsset.t.soltest/base-fork/ForkTestBase.t.soltest/mainnet-fork/Maple.t.soltest/mainnet-fork/ForkTestBase.t.soltest/mainnet-fork/OTCSwaps.t.soltest/mainnet-fork/Curve.t.soltest/base-fork/MorphoAllocations.t.soltest/mainnet-fork/Ethena.t.soltest/mainnet-fork/VaultCalls.t.soltest/mainnet-fork/LayerZero.t.soltest/mainnet-fork/Superstate.t.soltest/base-fork/Morpho.t.soltest/mainnet-fork/PsmCalls.t.soltest/mainnet-fork/Farm.t.soltest/mainnet-fork/4626Calls.t.soltest/mainnet-fork/wstETH.t.soltest/mainnet-fork/CCTPCalls.t.soltest/unit/controllers/Admin.t.soltest/mainnet-fork/DaiUsds.t.soltest/mainnet-fork/Aave.t.sol
📚 Learning: 2025-11-04T17:15:38.751Z
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:38.751Z
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/TransferAsset.t.soltest/unit/controllers/Freezer.t.soltest/base-fork/SparkVault.t.soltest/base-fork/Aave.t.soltest/base-fork/PsmCalls.t.soltest/unit/UnitTestBase.t.soltest/mainnet-fork/TransferAsset.t.soltest/base-fork/ForkTestBase.t.soltest/mainnet-fork/Maple.t.soltest/mainnet-fork/ForkTestBase.t.soltest/mainnet-fork/OTCSwaps.t.soltest/mainnet-fork/Curve.t.soltest/base-fork/MorphoAllocations.t.soltest/mainnet-fork/Ethena.t.soltest/mainnet-fork/VaultCalls.t.soltest/mainnet-fork/LayerZero.t.soltest/mainnet-fork/Superstate.t.soltest/base-fork/Morpho.t.soltest/mainnet-fork/PsmCalls.t.soltest/mainnet-fork/Farm.t.soltest/mainnet-fork/4626Calls.t.soltest/mainnet-fork/wstETH.t.soltest/mainnet-fork/CCTPCalls.t.soltest/unit/controllers/Admin.t.soltest/mainnet-fork/DaiUsds.t.soltest/mainnet-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/SparkVault.t.soltest/base-fork/SparkVault.t.soltest/base-fork/Aave.t.soltest/mainnet-fork/OTCSwaps.t.soltest/base-fork/MorphoAllocations.t.soltest/mainnet-fork/LayerZero.t.soltest/base-fork/Morpho.t.soltest/mainnet-fork/CCTPCalls.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/OTCSwaps.t.soltest/mainnet-fork/VaultCalls.t.soltest/mainnet-fork/LayerZero.t.soltest/mainnet-fork/PsmCalls.t.soltest/mainnet-fork/4626Calls.t.soltest/mainnet-fork/CCTPCalls.t.soltest/mainnet-fork/DaiUsds.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). (2)
- GitHub Check: test
- GitHub Check: coverage
🔇 Additional comments (5)
test/mainnet-fork/Farm.t.sol (5)
4-5: LGTM!The import is correctly added to enable testing of the ReentrancyGuard behavior.
47-53: LGTM!The contract rename improves naming consistency, and the reentrancy test correctly verifies that
depositToFarmis protected against reentrant calls.
120-126: LGTM!The contract rename improves naming consistency, and the reentrancy test correctly verifies that
withdrawFromFarmis protected against reentrant calls.
105-110: LGTM!The addition of
vm.record()and_assertReentrancyGuardWrittenToTwice()correctly validates that the reentrancy guard engages and releases properly during successfuldepositToFarmexecution.
184-189: LGTM!The addition of
vm.record()and_assertReentrancyGuardWrittenToTwice()correctly validates that the reentrancy guard engages and releases properly during successfulwithdrawFromFarmexecution.
|
Coverage after merging sc-1224-reentrancy-guards into dev will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Summary by CodeRabbit
Security Improvements
Tests