feat: Test maxSlippage with ERC4626 donation attack #167
Conversation
hacker-DOM
force-pushed
the
sc-1183-test-maxSlippage-with-4626-donation-attack
branch
from
ed6341f to
f24d6ce
Compare
|
|
||
| contract ERC4626DonationAttackTestBase is ForkTestBase { | ||
|
|
||
| IMetaMorpho morpho_vault = IMetaMorpho(0xe41a0583334f0dc4E023Acd0bFef3667F6FE0597); |
There was a problem hiding this comment.
camelCase for all values in this PR
| assertEq(morpho_vault.totalSupply(), 0); | ||
|
|
||
| // Initialization | ||
| vm.startPrank(Ethereum.SPARK_PROXY); |
There was a problem hiding this comment.
New line after this and before stopPrank to make it clearer that its for the whole block between
| assertEq(morpho_vault.curator(), curator); | ||
| assertEq(morpho_vault.guardian(), guardian); | ||
| assertEq(morpho_vault.feeRecipient(), fee_recipient); | ||
| assertTrue(morpho_vault.isAllocator(allocator)); |
| contract ERC4626DonationAttack is ERC4626DonationAttackTestBase { | ||
|
|
||
| function test_donationAttackERC4626_usds() external { | ||
| address mallory = makeAddr("mallory"); |
There was a problem hiding this comment.
attacker = "attacker"
| assertEq(morpho_vault.balanceOf(mallory), 1); | ||
| assertEq(morpho_vault.totalSupply(), 1); |
| try mainnetController.depositERC4626(address(morpho_vault), 2_000_000e18) { | ||
| // The deposit went through. The only time this is permissible is if the attack had no | ||
| // effect. | ||
| uint256 assetsOfProxy = morpho_vault.convertToAssets(morpho_vault.balanceOf(address(almProxy))); | ||
| assertEq(assetsOfProxy, 2_000_000e18); | ||
| assertEq(morpho_vault.balanceOf(address(almProxy)), 2_000_000e24); | ||
| } catch Error(string memory reason) { | ||
| // The deposit was correctly reverted. | ||
| assertEq(reason, "MainnetController/slippage-too-high"); | ||
| } |
There was a problem hiding this comment.
This should be two separate tests, one with a really conservative slippage set and one with a loose slippage set and show success and revert cases. Abstract attack setup into a helper function. Tabish did the same thing for the AAVE testing so can follow his implementation for this.
Should assert result of successful attack, showing lost funds.
WalkthroughAdds a new mainnet-fork test suite that deploys/configures a Morpho ERC4626 vault, sets timelock-driven caps/queues and per-function rate limits, simulates a donation-style supply attack, and verifies depositERC4626 behavior under low- and high-slippage scenarios. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Contract
participant Timelock as Timelock
participant Vault as Morpho ERC4626 Vault
participant Controller as Controller/Market
participant Relayer as Relayer/Proxy
Note right of Test #DDEEFF: Setup & governance
Test->>Timelock: schedule & execute cap/supplyQueue/rate-limit changes
Timelock-->>Controller: apply caps/queues/roles
Note right of Test #E8F8E0: Attack simulation
Test->>Vault: _doAttack() — supply (donation) into market
Vault-->>Vault: update totalAssets / totalSupply
Note right of Controller #FFF2CC: depositERC4626 flows
Relayer->>Controller: depositERC4626(amount, slippage)
alt slippage too low
Controller-->>Relayer: revert (slippage)
else slippage allowed
Controller->>Vault: mint shares / update totals
Vault-->>Relayer: return shares
Test->>Test: assert shares, totals, conversions
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
test/mainnet-fork/ERC4626DonationAttack.t.sol(1 hunks)
⏰ 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: build
- GitHub Check: coverage
🔇 Additional comments (1)
test/mainnet-fork/ERC4626DonationAttack.t.sol (1)
124-125: Verify hardcoded market state at fork block 22932160.
Lines 124–125 assert exact on-chain totals; they’ll break if the fork block or asset balances change. Fetch these values dynamically in your setup or pin the fork to block 22932160 to ensure test stability.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
test/mainnet-fork/ERC4626DonationAttack.t.sol (5)
43-43: Use direct string equality in assertion.No need for keccak hashing here.
- assertEq(keccak256(abi.encode(morphoVault.symbol())), keccak256(abi.encode("sparkUSDS"))); + assertEq(morphoVault.symbol(), "sparkUSDS");
26-31: Adopt camelCase for identifiers (consistency with project style).Rename fee_recipient/skim_recipient and update their usages.
- address fee_recipient = makeAddr("fee_recipient"); - address skim_recipient = makeAddr("skim_recipient"); + address feeRecipient = makeAddr("fee_recipient"); + address skimRecipient = makeAddr("skim_recipient"); @@ - morphoVault.setFeeRecipient(fee_recipient); + morphoVault.setFeeRecipient(feeRecipient); @@ - morphoVault.setSkimRecipient(skim_recipient); + morphoVault.setSkimRecipient(skimRecipient); @@ - assertEq(morphoVault.feeRecipient(), fee_recipient); + assertEq(morphoVault.feeRecipient(), feeRecipient);Also applies to: 50-55, 67-70
48-65: Add a blank line before stopPrank for readability.Clarifies the prank scope block.
morphoVault.setSupplyQueue(supplyOrder); + vm.stopPrank();
89-89: Clarify the conservative slippage setting.Document the actual tolerance to avoid confusion.
- mainnetController.setMaxSlippage(address(morphoVault), 1e18 - 1e4); // Rounding slippage + // Extremely conservative: allows only 1e-14 relative slippage (~0.000000000001%), i.e., rounding-only + mainnetController.setMaxSlippage(address(morphoVault), 1e18 - 1e4);
98-101: Explain the permissive slippage configuration.Make intent explicit for the success path.
- // Set max slippage to (close to) 100% + // Permissive: nearly 100% slippage allowed (minOut ≈ 1 / 1e18 of expected); enables attack success vm.prank(Ethereum.SPARK_PROXY); mainnetController.setMaxSlippage(address(morphoVault), 1);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
test/mainnet-fork/ERC4626DonationAttack.t.sol(1 hunks)
⏰ 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 (1)
test/mainnet-fork/ERC4626DonationAttack.t.sol (1)
92-95: Ensure revert expectation matches implementation (string vs custom error).If depositERC4626 uses a custom error, switch to selector-based expectRevert.
- vm.expectRevert("MainnetController/slippage-too-high"); + // If depositERC4626 uses a custom error, prefer: + // vm.expectRevert(MainnetController.SlippageTooHigh.selector); + vm.expectRevert("MainnetController/slippage-too-high");
hacker-DOM
force-pushed
the
sc-1183-test-maxSlippage-with-4626-donation-attack
branch
from
05f9350 to
39607a3
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
test/mainnet-fork/ERC4626DonationAttack.t.sol (2)
26-30: Use camelCase consistently for address variables.Variables
fee_recipientandskim_recipientuse snake_case, while other similar variables likeattackeruse camelCase. Past review comments requested camelCase for all values in this PR.Apply this diff to use consistent camelCase naming:
- address fee_recipient = makeAddr("fee_recipient"); + address feeRecipient = makeAddr("feeRecipient");- address skim_recipient = makeAddr("skim_recipient"); + address skimRecipient = makeAddr("skimRecipient");Update all references to these variables throughout the file (lines 52, 54, 69).
129-133: Consider renaming market variable for clarity.The
marketvariable captures the pre-attack snapshot and is correctly used at line 147 to verify expected shares. However, as noted in past review comments, renaming it tomarketBeforewould make it clearer that this snapshot represents state before the mutation.Apply this diff:
- Market memory market = morpho.market(marketId); + Market memory marketBefore = morpho.market(marketId); - assertEq(market.totalSupplyAssets, 36_095_481.319542091092211965e18); // ~36M USDS - assertEq(market.totalSupplyShares, 36_095_481.319542091092211965000000e24); + assertEq(marketBefore.totalSupplyAssets, 36_095_481.319542091092211965e18); // ~36M USDS + assertEq(marketBefore.totalSupplyShares, 36_095_481.319542091092211965000000e24);Also update line 147:
- assertEq(shares, uint256(1_000_000e18) * market.totalSupplyShares / market.totalSupplyAssets); + assertEq(shares, uint256(1_000_000e18) * marketBefore.totalSupplyShares / marketBefore.totalSupplyAssets);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
test/mainnet-fork/ERC4626DonationAttack.t.sol(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build
- GitHub Check: test
- GitHub Check: coverage
🔇 Additional comments (5)
test/mainnet-fork/ERC4626DonationAttack.t.sol (5)
34-77: Well-structured test setup.The setUp function properly configures the Morpho vault with all necessary roles, market parameters, timelock handling, and rate limits. The sequencing of operations (cap submission → timelock wait → cap acceptance → supply queue setup) correctly follows the protocol's requirements.
87-95: Test correctly validates slippage protection.The test properly demonstrates that conservative slippage settings (allowing only ~0.01% deviation) successfully prevent the donation attack from causing excessive loss. The explicit revert check confirms the controller's slippage protection is working as intended.
97-114: Test demonstrates successful donation attack under permissive slippage.The test correctly shows that with near-100% slippage allowance, the donation attack succeeds, resulting in significant value extraction (proxy deposits 2M but only receives 3 shares worth ~1.8M). The assertions properly verify the post-attack state.
The calculation comments (lines 106-110) accurately explain the MetaMorpho inflation protection mechanism that adds +1 to both numerator and denominator during share minting.
115-126: Asset conversion assertions properly demonstrate attack impact.The test correctly verifies that the donation attack causes value loss: the proxy deposits 2M USDS but recovers only 1.8M worth of assets, demonstrating a ~10% immediate loss due to the inflated share price. The attacker also experiences losses relative to their 1M deposit. These assertions effectively prove the vulnerability under permissive slippage settings.
79-81: Confirm block timestamp for test forkThe block number
22932160is annotated as July 16 2025, but couldn’t be fetched via Cloudflare RPC. Please verify with a reliable mainnet RPC (or Etherscan) that this block exists and has the intended timestamp before merging.
to test donation attack.
hacker-DOM
force-pushed
the
sc-1183-test-maxSlippage-with-4626-donation-attack
branch
from
6b5b2ea to
49ba71a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
test/mainnet-fork/ERC4626DonationAttack.t.sol (5)
147-148: Verify that share calculation uses pre-supply market state.The assertion uses
market(captured at line 129) to validate shares received from the supply operation. Confirm this is comparing against the intended pre-supply snapshot, not post-supply state. The memory snapshot approach is correct, but renaming tomarketBeforewould make the intent explicit, as previously suggested.
43-43: Inefficient string comparison remains.This line still uses
keccak256(abi.encode(...))for string comparison despite being flagged in a previous review. Direct string comparison is clearer and cheaper.Apply this diff:
- assertEq(keccak256(abi.encode(morphoVault.symbol())), keccak256(abi.encode("sparkUSDS"))); + assertEq(morphoVault.symbol(), "sparkUSDS");
89-89: Consider documenting the slippage tolerance.The value
1e18 - 1e4represents approximately 0.01% slippage tolerance. Adding an inline comment would improve readability, as noted in a previous review.
100-100: Document the permissive slippage setting.Setting
maxSlippageto1allows nearly 100% slippage. An explanatory comment would clarify this is intentional for demonstrating the attack's success case, as previously noted.
160-160: Missing explanation for test setup.The
deal()call funds almProxy for the subsequent deposit test in the calling function but lacks an explanatory comment, as noted in a previous review.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
test/mainnet-fork/ERC4626DonationAttack.t.sol(1 hunks)
⏰ 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 (6)
test/mainnet-fork/ERC4626DonationAttack.t.sol (6)
17-24: Market params correctly configured for supply-only vault.The zero values for
collateralToken,oracle,irm, andlltvappropriately model a supply-only market with no borrowing. The market ID derivation is correct.
34-77: Well-structured setUp with proper timelock flow.The initialization sequence correctly follows the Morpho vault's governance flow: role assignment → cap submission → timelock wait → cap acceptance → supply queue configuration → rate limit setup. The assertions validate each step.
79-81: LGTM!The block number override correctly targets a specific mainnet state for deterministic fork testing.
87-95: Test correctly validates slippage protection.The test properly configures conservative slippage limits, executes the attack, and verifies that the controller rejects the deposit due to excessive slippage. The revert message check is specific and appropriate.
106-126: Excellent verification of donation attack impact.The test thoroughly validates the attack mechanics with clear inline math explanations. The assertions correctly demonstrate:
- Share dilution (victim receives 3 shares instead of expected value)
- Value extraction (victim's 2M deposit converts to ~1.8M assets)
- Attacker profit (1 share converts to 600K assets from 1M+1 investment)
The mathematical comments make the attack vector transparent.
128-161: Donation attack simulation is technically sound.The helper correctly implements the ERC4626 donation attack vector:
- Attacker establishes minimal position (1 wei → 1 share)
- Attacker donates assets directly via Morpho.supply() to inflate totalAssets
- Subsequent depositors suffer severe share dilution
The assertions validate both the Morpho protocol behavior and the MetaMorpho vault's +1 offset logic. The inline math comments at lines 154-158 are particularly helpful.
| usds.approve(address(morphoVault), 1); | ||
| morphoVault.deposit(1, attacker); | ||
| usds.approve(address(morpho), 1_000_000e18); | ||
| // Donation attack |
There was a problem hiding this comment.
Donation attack performed by donating shares of morpho market supply to morpho vault
| vm.prank(relayer); | ||
| uint256 shares = mainnetController.depositERC4626(address(morphoVault), 2_000_000e18); | ||
|
|
||
| // We can compute: |
There was a problem hiding this comment.
No "we" or "us" language
|
Coverage after merging sc-1183-test-maxSlippage-with-4626-donation-attack into dev will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
test/mainnet-fork/ERC4626DonationAttack.t.sol (6)
43-43: Simplify string comparison.Past review feedback identified this, but it remains unaddressed. The
keccak256(abi.encode(...))pattern is unnecessarily expensive for a simple string equality check.Apply this diff:
- assertEq(keccak256(abi.encode(morphoVault.symbol())), keccak256(abi.encode("sparkUSDS"))); + assertEq(morphoVault.symbol(), "sparkUSDS");
161-161: Add comment explaining test setup.The
deal()call funds the almProxy for the subsequent deposit operation in the calling test. A brief comment improves test flow clarity.+ // Fund almProxy for subsequent deposit test deal(address(usds), address(almProxy), 2_000_000e18);
147-149: Clarify share type distinction.Line 149 asserts
shares == 1e30, representing Morpho protocol shares from themorpho.supply()call. This is distinct from MetaMorpho vault shares (which total 1 at this point). Adding a comment prevents confusion.assertEq(assets, 1_000_000e18); assertEq(shares, uint256(1_000_000e18) * market.totalSupplyShares / market.totalSupplyAssets); - assertEq(shares, 1e30); + assertEq(shares, 1e30); // Morpho protocol shares (not vault shares)
88-89: Clarify the slippage tolerance value.The inline comment "Rounding slippage" doesn't explain what
1e18 - 1e4represents. This value means 99.99% (allowing only ~1 basis point of slippage), which is critical for understanding why the attack fails.Apply this diff:
vm.prank(Ethereum.SPARK_PROXY); - mainnetController.setMaxSlippage(address(morphoVault), 1e18 - 1e4); // Rounding slippage + // Conservative slippage: 99.99% of expected shares (allows only ~1 basis point slippage) + mainnetController.setMaxSlippage(address(morphoVault), 1e18 - 1e4);
98-100: Enhance comment explaining permissive slippage.While the existing comment mentions "close to 100%", it could be more explicit about the implications for demonstration purposes.
Apply this diff:
- // Set max slippage to (close to) 100% + // Set max slippage to nearly 100% (allows up to ~99.9999...% slippage) + // This permissive setting demonstrates the attack succeeds with inadequate protection vm.prank(Ethereum.SPARK_PROXY); mainnetController.setMaxSlippage(address(morphoVault), 1);
136-144: Consider adding step comments for attack clarity.The attack sequence is correct but would benefit from brief step annotations to improve readability.
deal(address(usds), attacker, 1_000_000e18 + 1); vm.startPrank(attacker); + // Step 1: Deposit 1 wei to establish minimal vault share usds.approve(address(morphoVault), 1); morphoVault.deposit(1, attacker); + + // Step 2: Direct donation to Morpho inflates totalAssets without minting vault shares usds.approve(address(morpho), 1_000_000e18); - // Donation attack performed by donating shares of Morpho market supply to Morpho vault
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
test/mainnet-fork/ERC4626DonationAttack.t.sol(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: coverage
- GitHub Check: build
- GitHub Check: test
🔇 Additional comments (1)
test/mainnet-fork/ERC4626DonationAttack.t.sol (1)
79-81: LGTM: Block override for fork testing.The fixed block number with date comment provides clear context for the test environment.
| // One can compute: | ||
| // shares == assets * (totalSupply + 1) / (totalAssets + 1) | ||
| // == 2_000_000e18 * (1 + 1) / (1_000_000e18 + 1 + 1) | ||
| // == 3.9.. | ||
| // Rounding down, the proxy receives 3 shares. | ||
| assertEq(shares, 3); | ||
| assertEq(morphoVault.totalAssets(), 3_000_000e18 + 1); | ||
| assertEq(morphoVault.totalSupply(), 4); | ||
|
|
||
| uint256 assetsOfProxy = morphoVault.convertToAssets(morphoVault.balanceOf(address(almProxy))); | ||
| uint256 assetsOfAttacker = morphoVault.convertToAssets(morphoVault.balanceOf(attacker)); | ||
|
|
||
| // convertToAssets(shares) == shares * (totalAssets + 1) / (totalSupply + 1) | ||
| // convertToAssets(3) == 3 * (3_000_000e18 + 1 + 1) / (4 + 1) | ||
| // == 1_800_000e18 + 1 | ||
| assertEq(assetsOfProxy, 1_800_000e18 + 1); | ||
| assertLt(assetsOfProxy, 2_000_000e18); // The proxy owns less than it deposited | ||
| // convertToAssets(1) == 1 * (3_000_000e18 + 1 + 1) / (4 + 1) | ||
| // == 600_000e18 | ||
| assertEq(assetsOfAttacker, 600_000e18); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider asserting that attacker loses money in the attack.
The detailed math shows the proxy loses ~200k, but notably the attacker invests 1_000_000e18 + 1 and can only redeem 600_000e18 (line 125), making this attack unprofitable for the attacker. Adding an assertion would strengthen the test's security demonstration.
assertEq(assetsOfAttacker, 600_000e18);
+ assertLt(assetsOfAttacker, 1_000_000e18); // Attacker loses more than the victim📝 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.
| // One can compute: | |
| // shares == assets * (totalSupply + 1) / (totalAssets + 1) | |
| // == 2_000_000e18 * (1 + 1) / (1_000_000e18 + 1 + 1) | |
| // == 3.9.. | |
| // Rounding down, the proxy receives 3 shares. | |
| assertEq(shares, 3); | |
| assertEq(morphoVault.totalAssets(), 3_000_000e18 + 1); | |
| assertEq(morphoVault.totalSupply(), 4); | |
| uint256 assetsOfProxy = morphoVault.convertToAssets(morphoVault.balanceOf(address(almProxy))); | |
| uint256 assetsOfAttacker = morphoVault.convertToAssets(morphoVault.balanceOf(attacker)); | |
| // convertToAssets(shares) == shares * (totalAssets + 1) / (totalSupply + 1) | |
| // convertToAssets(3) == 3 * (3_000_000e18 + 1 + 1) / (4 + 1) | |
| // == 1_800_000e18 + 1 | |
| assertEq(assetsOfProxy, 1_800_000e18 + 1); | |
| assertLt(assetsOfProxy, 2_000_000e18); // The proxy owns less than it deposited | |
| // convertToAssets(1) == 1 * (3_000_000e18 + 1 + 1) / (4 + 1) | |
| // == 600_000e18 | |
| assertEq(assetsOfAttacker, 600_000e18); | |
| } | |
| // One can compute: | |
| // shares == assets * (totalSupply + 1) / (totalAssets + 1) | |
| // == 2_000_000e18 * (1 + 1) / (1_000_000e18 + 1 + 1) | |
| // == 3.9.. | |
| // Rounding down, the proxy receives 3 shares. | |
| assertEq(shares, 3); | |
| assertEq(morphoVault.totalAssets(), 3_000_000e18 + 1); | |
| assertEq(morphoVault.totalSupply(), 4); | |
| uint256 assetsOfProxy = morphoVault.convertToAssets(morphoVault.balanceOf(address(almProxy))); | |
| uint256 assetsOfAttacker = morphoVault.convertToAssets(morphoVault.balanceOf(attacker)); | |
| // convertToAssets(shares) == shares * (totalAssets + 1) / (totalSupply + 1) | |
| // convertToAssets(3) == 3 * (3_000_000e18 + 1 + 1) / (4 + 1) | |
| // == 1_800_000e18 + 1 | |
| assertEq(assetsOfProxy, 1_800_000e18 + 1); | |
| assertLt(assetsOfProxy, 2_000_000e18); // The proxy owns less than it deposited | |
| // convertToAssets(1) == 1 * (3_000_000e18 + 1 + 1) / (4 + 1) | |
| // == 600_000e18 | |
| assertEq(assetsOfAttacker, 600_000e18); | |
| assertLt(assetsOfAttacker, 1_000_000e18); // Attacker loses more than the victim | |
| } |
lucas-manuel
deleted the
sc-1183-test-maxSlippage-with-4626-donation-attack
branch
Summary by CodeRabbit