feat: canonical ERC-4626 vault with inflation-attack guard#36
Open
invisiblemonsters wants to merge 1 commit into
Open
feat: canonical ERC-4626 vault with inflation-attack guard#36invisiblemonsters wants to merge 1 commit into
invisiblemonsters wants to merge 1 commit into
Conversation
- Vault4626: minimal ERC-4626 implementation using OpenZeppelin v5.3.0 - Inflation-attack guard: 1e3 dead shares minted to address(0xdead) - Donation flow: donate() transfers assets without minting shares - Hook pattern: _afterDeposit / _beforeWithdraw for yield extensions - Correct rounding: convertToShares rounds UP, convertToAssets rounds DOWN - All 4 entry points: deposit, withdraw, mint, redeem - Tests: 22 passing (share price math, donation, rounding, multi-user, access control) Closes sentrix-labs#27
📝 WalkthroughWalkthroughThis pull request introduces a minimal, hardened ERC-4626 vault implementation to serve as a canonical standard for the repository. The Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@contracts/Vault4626.sol`:
- Around line 91-93: Add documentation to the _beforeWithdraw hook warning
implementers that it executes before _spendAllowance and _burn inside _withdraw,
creating a potential reentrancy window if implementations perform external calls
(e.g., pulling from strategies); instruct implementers to follow CEI (perform
necessary state writes in the parent or child before making external calls),
avoid making external calls that can re-enter vault logic, or protect
implementations with ReentrancyGuard where appropriate, and include a short
example note describing safe patterns for yield-bearing children.
- Around line 42-53: The _convertToShares function currently subtracts the
dead-share offset from supply but does not add it to totalAssets(), which breaks
the virtual offset protection; update the conversion to use the same offset on
both sides (e.g., use (supply + OFFSET) and (totalAssets() + OFFSET) where
OFFSET is the existing dead-share constant 1e3) so the return becomes
_tryMulDiv(assets, supply + OFFSET, totalAssets() + OFFSET, rounding) and retain
the initial special-case check for supply == OFFSET to preserve 1:1 behavior
before live supply exists; ensure you reference the existing supply variable,
totalAssets() call, the OFFSET constant (1e3), and the _tryMulDiv helper in the
change.
In `@test/Vault4626.t.sol`:
- Around line 47-312: The test suite lacks fuzz/invariant tests for monetary
entrypoints; add property-based/fuzz and invariant tests covering deposit, mint,
withdraw, redeem, donate and all preview/max functions
(convertToAssets/convertToShares,
previewDeposit/previewMint/previewWithdraw/previewRedeem,
maxDeposit/maxMint/maxWithdraw/maxRedeem) to exercise rounding and state
transitions. Implement fuzzed calls with random asset/share amounts and actors,
assert invariants such as totalAssets() == asset.balanceOf(address(vault)) (plus
donated transfers), totalSupply() == DEAD_SHARES + sum(live user balances), no
unexpected reverts for valid inputs, dead shares remain at DEAD and cannot be
redeemed, preview* and max* are consistent with actual operations (e.g.,
previewDeposit(x) <= deposit outcome considering rounding), and that rounding
favors the vault; run these as invariant tests (Forge invariant/fuzz harness) to
catch edge cases.
- Around line 17-313: Create a small test subclass (e.g., HookedVault4626 or
TestVaultHooks) that inherits Vault4626 and overrides _afterDeposit and
_beforeWithdraw to flip booleans and increment counters and record arguments
(caller, owner, assets/shares); add tests that deploy this hooked vault, perform
a deposit and a withdraw, and assert the hooks were invoked with expected values
and in expected order (ensure _afterDeposit runs after deposit and
_beforeWithdraw runs before withdraw) by checking the flags/counters and
recorded parameters on the hook subclass after calling deposit/mint and
withdraw/redeem; use the existing test patterns and accounts (alice, bob,
charlie) and existing assertions style to validate correct hook behavior.
- Around line 145-183: The test_inflationAttackBlocked currently never asserts
the attack outcome and redeems using the stale attackerShares variable; update
it to redeem the attacker's full current share balance (use
vault.balanceOf(charlie)) and assert attacker cannot profit: capture charlie's
asset balance before redeem, perform vault.redeem(vault.balanceOf(charlie),
charlie, charlie), capture after balance and assert received_assets <=
assets_deposited (or received_assets - deposited <= acceptable_loss/tolerance);
also add an explicit assertion that aliceShares is small (already present) and
assert attacker final asset delta does not exceed the 1 wei they originally
deposited (or does not produce a net gain) to prove the mitigation. Ensure
references: test_inflationAttackBlocked, attackerShares (stop using stale),
vault.balanceOf(charlie), vault.redeem(...), and aliceShares.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3466a6b3-0ce7-4562-a7a5-94c2e1fcefd6
📒 Files selected for processing (2)
contracts/Vault4626.soltest/Vault4626.t.sol
Comment on lines
+42
to
+53
| function _convertToShares(uint256 assets, Math.Rounding rounding) | ||
| internal | ||
| view | ||
| override | ||
| returns (uint256) | ||
| { | ||
| uint256 supply = totalSupply(); | ||
| // If no live supply beyond our dead shares, shares = assets (1:1 after offset) | ||
| if (supply == 1e3) return assets; | ||
|
|
||
| return _tryMulDiv(assets, supply - 1e3, totalAssets(), rounding); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
OpenZeppelin ERC4626 decimalsOffset virtual shares inflation attack protection pattern
💡 Result:
The ERC4626 inflation attack is a vulnerability where an attacker manipulates the vault's exchange rate by donating assets to an empty or near-empty vault, causing subsequent depositors to receive zero shares due to rounding [1][2]. OpenZeppelin mitigates this using a combination of virtual shares and a decimals offset [3][4]. The protection pattern works through two primary mechanisms: 1. Virtual Shares and Assets: The vault implementation adds a "virtual" quantity of shares and assets to the exchange rate computation [3]. By effectively seeding the vault with these virtual amounts, the vault enforces a non-zero exchange rate even when empty [3]. This ensures that any donation by an attacker is partially captured by the vault, making the attack significantly less profitable or entirely unprofitable [1][3]. 2. Decimals Offset: The _decimalsOffset function allows developers to increase the precision of share representation relative to the underlying asset [3]. By setting an offset (e.g., 9), the vault uses more decimal places for shares than the underlying token uses for assets [1][3]. This increased precision reduces rounding errors and forces an attacker to donate significantly larger amounts to achieve the same dilution effect, effectively making the attack prohibitively expensive [1][3]. Key considerations for this pattern include: - Default State: By default, OpenZeppelin's implementation may return 0 for _decimalsOffset, which provides minimal protection [5][6]. Developers are encouraged to override this function to return a higher value (e.g., 9) to enhance security [5][7][4]. - Trade-offs: While higher offsets increase security, they also change the decimal representation of the vault's shares, which may impact integration with other protocols [3][4]. - Inherent Loss: Because virtual shares and assets are included in the math, a small, usually negligible amount of value may be "captured" by the vault, making it unrecoverable by early depositors [3][4]. In summary, the combination of virtual shares and a non-zero decimals offset creates a robust defense by ensuring that the cost of an inflation attack is orders of magnitude higher than any potential gain [1][3].
Citations:
- 1: https://old-docs.openzeppelin.com/contracts/5.x/erc4626
- 2: https://docs.openzeppelin.com/contracts-cairo/2.x/erc4626
- 3: https://www.openzeppelin.com/news/a-novel-defense-against-erc4626-inflation-attacks
- 4: ERC4626 inflation attack mitigation OpenZeppelin/openzeppelin-contracts#3979
- 5: ERC4626 inflation issue OpenZeppelin/openzeppelin-contracts#5223
- 6: https://medium.com/@regis-graptin/build-secure-erc-4626-vaults-mastering-inflation-attack-prevention-64169912f188
- 7: https://forum.openzeppelin.com/t/erc4626-inflation-attack-discussion/41643
🏁 Script executed:
fd -t f "Vault4626.sol" --exec cat -n {} \;Repository: sentrix-labs/canonical-contracts
Length of output: 6646
🏁 Script executed:
rg "function _tryMulDiv|function _convertToAssets" -A 5 contracts/Repository: sentrix-labs/canonical-contracts
Length of output: 679
🏁 Script executed:
rg "function _convertToShares.*Math.Rounding" -A 8 contracts/ -t solRepository: sentrix-labs/canonical-contracts
Length of output: 104
🏁 Script executed:
grep -n "Rounds UP" contracts/Vault4626.solRepository: sentrix-labs/canonical-contracts
Length of output: 157
🏁 Script executed:
git log --oneline --all -S "supply - 1e3" 2>/dev/null | head -5 || echo "Git history check complete"Repository: sentrix-labs/canonical-contracts
Length of output: 143
Inflation attack protection is ineffective — dead shares are excluded from the conversion formula.
The formula assets * (supply - 1e3) / totalAssets() subtracts dead shares from supply but does not add them to totalAssets. This asymmetry defeats the protection. Once any user deposits, subsequent depositors are vulnerable:
- Attacker deposits 1 wei → receives 1 share (1:1 since supply == 1e3)
- Attacker donates ~1e18 assets → supply = 1001, totalAssets = 1e18
- Victim deposits 1e18 → receives
1e18 * 1 / 1e18 = 1 share - Victim holds 1/1002 of the vault; attacker redeems their share for nearly all assets
The OpenZeppelin virtual offset pattern mitigates this by adding the offset to both sides: assets * (supply + offset) / (totalAssets + offset). This ensures the offset continuously dilutes donation attacks throughout the vault's lifetime, not just initially.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@contracts/Vault4626.sol` around lines 42 - 53, The _convertToShares function
currently subtracts the dead-share offset from supply but does not add it to
totalAssets(), which breaks the virtual offset protection; update the conversion
to use the same offset on both sides (e.g., use (supply + OFFSET) and
(totalAssets() + OFFSET) where OFFSET is the existing dead-share constant 1e3)
so the return becomes _tryMulDiv(assets, supply + OFFSET, totalAssets() +
OFFSET, rounding) and retain the initial special-case check for supply == OFFSET
to preserve 1:1 behavior before live supply exists; ensure you reference the
existing supply variable, totalAssets() call, the OFFSET constant (1e3), and the
_tryMulDiv helper in the change.
Comment on lines
+91
to
+93
| /// @notice Hook called before shares are burned (withdraw/redeem). | ||
| /// @dev Override in yield-bearing children to pull assets from strategies. | ||
| function _beforeWithdraw(address caller, uint256 assets, uint256 shares) internal virtual {} |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Document reentrancy considerations for _beforeWithdraw hook implementations.
The hook executes before _spendAllowance and _burn in _withdraw. Child contracts that make external calls here (e.g., pulling assets from strategies) create a reentrancy window. While the impact is limited since shares are eventually checked/burned, downstream implementers should be aware:
/// `@notice` Hook called before shares are burned (withdraw/redeem).
/// `@dev` Override in yield-bearing children to pull assets from strategies.
+ /// WARNING: If making external calls in override, be aware this executes
+ /// before shares are burned. Consider ReentrancyGuard for complex flows.
function _beforeWithdraw(address caller, uint256 assets, uint256 shares) internal virtual {}As per coding guidelines: "Reentrancy: state writes before external calls (CEI pattern); ReentrancyGuard where applicable."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@contracts/Vault4626.sol` around lines 91 - 93, Add documentation to the
_beforeWithdraw hook warning implementers that it executes before
_spendAllowance and _burn inside _withdraw, creating a potential reentrancy
window if implementations perform external calls (e.g., pulling from
strategies); instruct implementers to follow CEI (perform necessary state writes
in the parent or child before making external calls), avoid making external
calls that can re-enter vault logic, or protect implementations with
ReentrancyGuard where appropriate, and include a short example note describing
safe patterns for yield-bearing children.
Comment on lines
+17
to
+313
| contract Vault4626Test is Test { | ||
| Vault4626 public vault; | ||
| MockAsset public asset; | ||
|
|
||
| address alice = makeAddr("alice"); | ||
| address bob = makeAddr("bob"); | ||
| address charlie = makeAddr("charlie"); | ||
| address DEAD = address(0xdead); | ||
|
|
||
| uint256 constant DEAD_SHARES = 1e3; | ||
|
|
||
| function setUp() public { | ||
| asset = new MockAsset("Mock USDC", "mUSDC"); | ||
| vault = new Vault4626(IERC20(address(asset)), "Vault mUSDC", "vmUSDC"); | ||
|
|
||
| // Fund test accounts | ||
| asset.mint(alice, 1_000_000 ether); | ||
| asset.mint(bob, 1_000_000 ether); | ||
| asset.mint(charlie, 1_000_000 ether); | ||
|
|
||
| vm.prank(alice); | ||
| asset.approve(address(vault), type(uint256).max); | ||
| vm.prank(bob); | ||
| asset.approve(address(vault), type(uint256).max); | ||
| vm.prank(charlie); | ||
| asset.approve(address(vault), type(uint256).max); | ||
| } | ||
|
|
||
| // ─── Basic flows ─────────────────────────────────────────────── | ||
|
|
||
| function test_deposit() public { | ||
| vm.prank(alice); | ||
| uint256 shares = vault.deposit(1_000 ether, alice); | ||
|
|
||
| assertEq(shares, 1_000 ether); // 1:1 after dead-share offset | ||
| assertEq(vault.totalSupply(), DEAD_SHARES + 1_000 ether); | ||
| assertEq(vault.totalAssets(), 1_000 ether); | ||
| assertEq(vault.balanceOf(alice), 1_000 ether); | ||
| assertEq(asset.balanceOf(address(vault)), 1_000 ether); | ||
| } | ||
|
|
||
| function test_withdraw() public { | ||
| vm.prank(alice); | ||
| vault.deposit(1_000 ether, alice); | ||
|
|
||
| uint256 preBal = asset.balanceOf(alice); | ||
| vm.prank(alice); | ||
| vault.withdraw(500 ether, alice, alice); | ||
|
|
||
| assertEq(asset.balanceOf(alice) - preBal, 500 ether); | ||
| assertEq(vault.totalAssets(), 500 ether); | ||
| } | ||
|
|
||
| function test_mint() public { | ||
| vm.prank(alice); | ||
| uint256 assets = vault.mint(1_000 ether, alice); | ||
|
|
||
| assertEq(assets, 1_000 ether); // 1:1 | ||
| assertEq(vault.balanceOf(alice), 1_000 ether); | ||
| assertEq(vault.totalAssets(), 1_000 ether); | ||
| } | ||
|
|
||
| function test_redeem() public { | ||
| vm.prank(alice); | ||
| vault.deposit(1_000 ether, alice); | ||
|
|
||
| uint256 preBal = asset.balanceOf(alice); | ||
| vm.prank(alice); | ||
| uint256 assets = vault.redeem(500 ether, alice, alice); | ||
|
|
||
| assertEq(assets, 500 ether); | ||
| assertEq(asset.balanceOf(alice) - preBal, 500 ether); | ||
| } | ||
|
|
||
| // ─── Share price math ────────────────────────────────────────── | ||
|
|
||
| function test_sharePriceAfterDonation() public { | ||
| // Alice deposits 1000 | ||
| vm.prank(alice); | ||
| vault.deposit(1_000 ether, alice); | ||
|
|
||
| // Bob donates 500 (no shares minted) | ||
| vm.prank(bob); | ||
| vault.donate(500 ether); | ||
|
|
||
| // Share price should now be 1.5 assets per share | ||
| // totalAssets = 1500, live shares = 1000, price = 1.5 | ||
| assertEq(vault.totalAssets(), 1_500 ether); | ||
|
|
||
| uint256 preview = vault.previewRedeem(100 ether); | ||
| // 100 shares * 1.5 = 150 assets | ||
| assertEq(preview, 150 ether); | ||
| } | ||
|
|
||
| function test_sharePriceAfterYield() public { | ||
| vm.prank(alice); | ||
| vault.deposit(1_000 ether, alice); | ||
|
|
||
| // Simulate yield: transfer assets directly to vault (bypassing deposit) | ||
| vm.prank(bob); | ||
| asset.transfer(address(vault), 500 ether); | ||
|
|
||
| // Share price doubled: 1500 assets / 1000 live shares | ||
| assertEq(vault.totalAssets(), 1_500 ether); | ||
| assertEq(vault.previewRedeem(100 ether), 150 ether); | ||
| } | ||
|
|
||
| function test_sharePriceMultiUser() public { | ||
| vm.prank(alice); | ||
| vault.deposit(1_000 ether, alice); | ||
|
|
||
| vm.prank(bob); | ||
| vault.deposit(2_000 ether, bob); | ||
|
|
||
| // Alice: 1000 shares, Bob: 2000 shares | ||
| assertEq(vault.balanceOf(alice), 1_000 ether); | ||
| assertEq(vault.balanceOf(bob), 2_000 ether); | ||
| assertEq(vault.totalAssets(), 3_000 ether); | ||
|
|
||
| // Alice withdraws proportional amount | ||
| vm.prank(alice); | ||
| vault.redeem(1_000 ether, alice, alice); | ||
| // Alice should get her share of total assets: 1000/3000 * 3000 = 1000 | ||
| assertApproxEqAbs(asset.balanceOf(address(vault)), 2_000 ether, 1); | ||
| } | ||
|
|
||
| // ─── Inflation attack guard ──────────────────────────────────── | ||
|
|
||
| function test_inflationAttackBlocked() public { | ||
| // Attacker tries donation attack: donate 1 wei to inflate share price, | ||
| // then front-run victim's deposit. | ||
|
|
||
| // Dead shares exist: 1000 | ||
| assertEq(vault.balanceOf(DEAD), DEAD_SHARES); | ||
|
|
||
| // Attacker deposits 1 wei — pays dearly due to dead-share offset | ||
| vm.prank(charlie); | ||
| uint256 attackerShares = vault.deposit(1, charlie); | ||
|
|
||
| // Attacker gets ~0 shares because 1 asset / (DEAD_SHARES floor) ≈ 0 | ||
| // Actually: convertToShares(1) with supply=DEAD_SHARES, assets=0 | ||
| // _convertToShares: supply > DEAD_SHARES? No (supply==DEAD_SHARES). Return assets directly = 1 | ||
| // Wait — first deposit after dead shares: supply==DEAD_SHARES so return assets == 1 share for 1 wei | ||
| // That's the expected 1:1 start for the first real depositor | ||
|
|
||
| // The real test: attacker donates massive amount, then tries to steal | ||
| // Fund charlie with enough to do the donation AND the deposit | ||
| asset.mint(charlie, 2_000_000 ether); | ||
| vm.startPrank(charlie); | ||
| asset.approve(address(vault), type(uint256).max); | ||
| vault.deposit(1, charlie); // attacker deposits 1 wei | ||
| asset.transfer(address(vault), 1_000_000 ether); // donate via direct transfer | ||
| vm.stopPrank(); | ||
|
|
||
| // Share price is now massive: (1_000_000 + 1) / (DEAD_SHARES + 1) ≈ 999 ether/share | ||
| // Alice deposits 1 ether — gets ~0.001 shares | ||
| vm.prank(alice); | ||
| uint256 aliceShares = vault.deposit(1 ether, alice); | ||
|
|
||
| // Alice should get very few shares due to inflated share price | ||
| assertTrue(aliceShares < 1 ether / 100); // less than 0.01 shares | ||
|
|
||
| // Attacker tries to withdraw their 1 share — gets massive assets | ||
| vm.prank(charlie); | ||
| // charlie has attackerShares shares | ||
| vault.redeem(attackerShares, charlie, charlie); | ||
| } | ||
|
|
||
| function test_deadSharesPermanent() public { | ||
| // Dead shares can never be withdrawn — they're at address(0xdead) | ||
| assertEq(vault.balanceOf(DEAD), DEAD_SHARES); | ||
| // maxRedeem reflects share balance but DEAD has no way to call redeem | ||
| assertEq(vault.maxRedeem(DEAD), DEAD_SHARES); | ||
|
|
||
| // Deposits still work normally | ||
| vm.prank(alice); | ||
| vault.deposit(500 ether, alice); | ||
| assertEq(vault.balanceOf(alice), 500 ether); | ||
| } | ||
|
|
||
| // ─── Rounding direction ──────────────────────────────────────── | ||
|
|
||
| function test_roundingFavorsVault() public { | ||
| vm.prank(alice); | ||
| vault.deposit(1_000 ether, alice); | ||
|
|
||
| // Donate a weird amount to create rounding issues | ||
| vm.prank(bob); | ||
| vault.donate(333 ether); | ||
|
|
||
| // previewWithdraw: rounds UP (user needs more shares) | ||
| // previewMint: rounds UP (user needs more assets) | ||
| // previewDeposit: rounds DOWN (user gets fewer shares) | ||
| // previewRedeem: rounds DOWN (user gets fewer assets) | ||
|
|
||
| uint256 assetsForWithdraw = vault.previewWithdraw(100 ether); | ||
| uint256 sharesForDeposit = vault.previewDeposit(100 ether); | ||
|
|
||
| // withdraw should cost >= deposit for same asset amount | ||
| assertGe(assetsForWithdraw, sharesForDeposit); | ||
| } | ||
|
|
||
| // ─── Access control ──────────────────────────────────────────── | ||
|
|
||
| function test_withdrawWithApproval() public { | ||
| vm.prank(alice); | ||
| vault.deposit(1_000 ether, alice); | ||
|
|
||
| // Alice approves Bob to withdraw | ||
| vm.prank(alice); | ||
| vault.approve(bob, 500 ether); | ||
|
|
||
| vm.prank(bob); | ||
| vault.withdraw(500 ether, bob, alice); | ||
|
|
||
| assertEq(asset.balanceOf(bob), 1_000_000 ether + 500 ether); // initial + withdrawn | ||
| } | ||
|
|
||
| function test_revert_withdrawWithoutApproval() public { | ||
| vm.prank(alice); | ||
| vault.deposit(1_000 ether, alice); | ||
|
|
||
| vm.prank(bob); | ||
| vm.expectRevert(); | ||
| vault.withdraw(500 ether, bob, alice); | ||
| } | ||
|
|
||
| // ─── Donation ────────────────────────────────────────────────── | ||
|
|
||
| function test_donate() public { | ||
| vm.prank(alice); | ||
| vault.deposit(1_000 ether, alice); | ||
|
|
||
| uint256 prePrice = vault.convertToAssets(1 ether); | ||
|
|
||
| vm.prank(bob); | ||
| vault.donate(500 ether); | ||
|
|
||
| uint256 postPrice = vault.convertToAssets(1 ether); | ||
| assertGt(postPrice, prePrice); // Share price increased | ||
| assertEq(vault.totalAssets(), 1_500 ether); | ||
| assertEq(vault.balanceOf(bob), 0); // Bob got no shares | ||
| } | ||
|
|
||
| function test_revert_donateZero() public { | ||
| vm.prank(alice); | ||
| vm.expectRevert(Vault4626.Vault4626__ZeroDeposit.selector); | ||
| vault.donate(0); | ||
| } | ||
|
|
||
| // ─── Max functions ───────────────────────────────────────────── | ||
|
|
||
| function test_maxDeposit() public { | ||
| assertEq(vault.maxDeposit(alice), type(uint256).max); | ||
| } | ||
|
|
||
| function test_maxMint() public { | ||
| assertEq(vault.maxMint(alice), type(uint256).max); | ||
| } | ||
|
|
||
| function test_maxWithdraw() public { | ||
| vm.prank(alice); | ||
| vault.deposit(500 ether, alice); | ||
| assertEq(vault.maxWithdraw(alice), 500 ether); | ||
| } | ||
|
|
||
| function test_maxRedeem() public { | ||
| vm.prank(alice); | ||
| vault.deposit(500 ether, alice); | ||
| assertEq(vault.maxRedeem(alice), 500 ether); | ||
| } | ||
|
|
||
| // ─── Preview functions ───────────────────────────────────────── | ||
|
|
||
| function test_previewDeposit() public { | ||
| uint256 shares = vault.previewDeposit(100 ether); | ||
| assertEq(shares, 100 ether); // 1:1 from clean state | ||
| } | ||
|
|
||
| function test_previewMint() public { | ||
| uint256 assets = vault.previewMint(100 ether); | ||
| assertEq(assets, 100 ether); // 1:1 | ||
| } | ||
|
|
||
| function test_previewWithdraw() public { | ||
| vm.prank(alice); | ||
| vault.deposit(1_000 ether, alice); | ||
| // 1000 assets withdrawn costs 1000 shares (1:1) | ||
| assertEq(vault.previewWithdraw(100 ether), 100 ether); | ||
| } | ||
|
|
||
| function test_previewRedeem() public { | ||
| vm.prank(alice); | ||
| vault.deposit(1_000 ether, alice); | ||
| assertEq(vault.previewRedeem(100 ether), 100 ether); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Hook pattern objective is untested (_afterDeposit, _beforeWithdraw).
Given this PR’s extension design, add a small test subclass of Vault4626 that toggles flags/counters in both hooks and assert they fire with expected arguments/order during deposit and withdraw flows.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/Vault4626.t.sol` around lines 17 - 313, Create a small test subclass
(e.g., HookedVault4626 or TestVaultHooks) that inherits Vault4626 and overrides
_afterDeposit and _beforeWithdraw to flip booleans and increment counters and
record arguments (caller, owner, assets/shares); add tests that deploy this
hooked vault, perform a deposit and a withdraw, and assert the hooks were
invoked with expected values and in expected order (ensure _afterDeposit runs
after deposit and _beforeWithdraw runs before withdraw) by checking the
flags/counters and recorded parameters on the hook subclass after calling
deposit/mint and withdraw/redeem; use the existing test patterns and accounts
(alice, bob, charlie) and existing assertions style to validate correct hook
behavior.
Comment on lines
+47
to
+312
| function test_deposit() public { | ||
| vm.prank(alice); | ||
| uint256 shares = vault.deposit(1_000 ether, alice); | ||
|
|
||
| assertEq(shares, 1_000 ether); // 1:1 after dead-share offset | ||
| assertEq(vault.totalSupply(), DEAD_SHARES + 1_000 ether); | ||
| assertEq(vault.totalAssets(), 1_000 ether); | ||
| assertEq(vault.balanceOf(alice), 1_000 ether); | ||
| assertEq(asset.balanceOf(address(vault)), 1_000 ether); | ||
| } | ||
|
|
||
| function test_withdraw() public { | ||
| vm.prank(alice); | ||
| vault.deposit(1_000 ether, alice); | ||
|
|
||
| uint256 preBal = asset.balanceOf(alice); | ||
| vm.prank(alice); | ||
| vault.withdraw(500 ether, alice, alice); | ||
|
|
||
| assertEq(asset.balanceOf(alice) - preBal, 500 ether); | ||
| assertEq(vault.totalAssets(), 500 ether); | ||
| } | ||
|
|
||
| function test_mint() public { | ||
| vm.prank(alice); | ||
| uint256 assets = vault.mint(1_000 ether, alice); | ||
|
|
||
| assertEq(assets, 1_000 ether); // 1:1 | ||
| assertEq(vault.balanceOf(alice), 1_000 ether); | ||
| assertEq(vault.totalAssets(), 1_000 ether); | ||
| } | ||
|
|
||
| function test_redeem() public { | ||
| vm.prank(alice); | ||
| vault.deposit(1_000 ether, alice); | ||
|
|
||
| uint256 preBal = asset.balanceOf(alice); | ||
| vm.prank(alice); | ||
| uint256 assets = vault.redeem(500 ether, alice, alice); | ||
|
|
||
| assertEq(assets, 500 ether); | ||
| assertEq(asset.balanceOf(alice) - preBal, 500 ether); | ||
| } | ||
|
|
||
| // ─── Share price math ────────────────────────────────────────── | ||
|
|
||
| function test_sharePriceAfterDonation() public { | ||
| // Alice deposits 1000 | ||
| vm.prank(alice); | ||
| vault.deposit(1_000 ether, alice); | ||
|
|
||
| // Bob donates 500 (no shares minted) | ||
| vm.prank(bob); | ||
| vault.donate(500 ether); | ||
|
|
||
| // Share price should now be 1.5 assets per share | ||
| // totalAssets = 1500, live shares = 1000, price = 1.5 | ||
| assertEq(vault.totalAssets(), 1_500 ether); | ||
|
|
||
| uint256 preview = vault.previewRedeem(100 ether); | ||
| // 100 shares * 1.5 = 150 assets | ||
| assertEq(preview, 150 ether); | ||
| } | ||
|
|
||
| function test_sharePriceAfterYield() public { | ||
| vm.prank(alice); | ||
| vault.deposit(1_000 ether, alice); | ||
|
|
||
| // Simulate yield: transfer assets directly to vault (bypassing deposit) | ||
| vm.prank(bob); | ||
| asset.transfer(address(vault), 500 ether); | ||
|
|
||
| // Share price doubled: 1500 assets / 1000 live shares | ||
| assertEq(vault.totalAssets(), 1_500 ether); | ||
| assertEq(vault.previewRedeem(100 ether), 150 ether); | ||
| } | ||
|
|
||
| function test_sharePriceMultiUser() public { | ||
| vm.prank(alice); | ||
| vault.deposit(1_000 ether, alice); | ||
|
|
||
| vm.prank(bob); | ||
| vault.deposit(2_000 ether, bob); | ||
|
|
||
| // Alice: 1000 shares, Bob: 2000 shares | ||
| assertEq(vault.balanceOf(alice), 1_000 ether); | ||
| assertEq(vault.balanceOf(bob), 2_000 ether); | ||
| assertEq(vault.totalAssets(), 3_000 ether); | ||
|
|
||
| // Alice withdraws proportional amount | ||
| vm.prank(alice); | ||
| vault.redeem(1_000 ether, alice, alice); | ||
| // Alice should get her share of total assets: 1000/3000 * 3000 = 1000 | ||
| assertApproxEqAbs(asset.balanceOf(address(vault)), 2_000 ether, 1); | ||
| } | ||
|
|
||
| // ─── Inflation attack guard ──────────────────────────────────── | ||
|
|
||
| function test_inflationAttackBlocked() public { | ||
| // Attacker tries donation attack: donate 1 wei to inflate share price, | ||
| // then front-run victim's deposit. | ||
|
|
||
| // Dead shares exist: 1000 | ||
| assertEq(vault.balanceOf(DEAD), DEAD_SHARES); | ||
|
|
||
| // Attacker deposits 1 wei — pays dearly due to dead-share offset | ||
| vm.prank(charlie); | ||
| uint256 attackerShares = vault.deposit(1, charlie); | ||
|
|
||
| // Attacker gets ~0 shares because 1 asset / (DEAD_SHARES floor) ≈ 0 | ||
| // Actually: convertToShares(1) with supply=DEAD_SHARES, assets=0 | ||
| // _convertToShares: supply > DEAD_SHARES? No (supply==DEAD_SHARES). Return assets directly = 1 | ||
| // Wait — first deposit after dead shares: supply==DEAD_SHARES so return assets == 1 share for 1 wei | ||
| // That's the expected 1:1 start for the first real depositor | ||
|
|
||
| // The real test: attacker donates massive amount, then tries to steal | ||
| // Fund charlie with enough to do the donation AND the deposit | ||
| asset.mint(charlie, 2_000_000 ether); | ||
| vm.startPrank(charlie); | ||
| asset.approve(address(vault), type(uint256).max); | ||
| vault.deposit(1, charlie); // attacker deposits 1 wei | ||
| asset.transfer(address(vault), 1_000_000 ether); // donate via direct transfer | ||
| vm.stopPrank(); | ||
|
|
||
| // Share price is now massive: (1_000_000 + 1) / (DEAD_SHARES + 1) ≈ 999 ether/share | ||
| // Alice deposits 1 ether — gets ~0.001 shares | ||
| vm.prank(alice); | ||
| uint256 aliceShares = vault.deposit(1 ether, alice); | ||
|
|
||
| // Alice should get very few shares due to inflated share price | ||
| assertTrue(aliceShares < 1 ether / 100); // less than 0.01 shares | ||
|
|
||
| // Attacker tries to withdraw their 1 share — gets massive assets | ||
| vm.prank(charlie); | ||
| // charlie has attackerShares shares | ||
| vault.redeem(attackerShares, charlie, charlie); | ||
| } | ||
|
|
||
| function test_deadSharesPermanent() public { | ||
| // Dead shares can never be withdrawn — they're at address(0xdead) | ||
| assertEq(vault.balanceOf(DEAD), DEAD_SHARES); | ||
| // maxRedeem reflects share balance but DEAD has no way to call redeem | ||
| assertEq(vault.maxRedeem(DEAD), DEAD_SHARES); | ||
|
|
||
| // Deposits still work normally | ||
| vm.prank(alice); | ||
| vault.deposit(500 ether, alice); | ||
| assertEq(vault.balanceOf(alice), 500 ether); | ||
| } | ||
|
|
||
| // ─── Rounding direction ──────────────────────────────────────── | ||
|
|
||
| function test_roundingFavorsVault() public { | ||
| vm.prank(alice); | ||
| vault.deposit(1_000 ether, alice); | ||
|
|
||
| // Donate a weird amount to create rounding issues | ||
| vm.prank(bob); | ||
| vault.donate(333 ether); | ||
|
|
||
| // previewWithdraw: rounds UP (user needs more shares) | ||
| // previewMint: rounds UP (user needs more assets) | ||
| // previewDeposit: rounds DOWN (user gets fewer shares) | ||
| // previewRedeem: rounds DOWN (user gets fewer assets) | ||
|
|
||
| uint256 assetsForWithdraw = vault.previewWithdraw(100 ether); | ||
| uint256 sharesForDeposit = vault.previewDeposit(100 ether); | ||
|
|
||
| // withdraw should cost >= deposit for same asset amount | ||
| assertGe(assetsForWithdraw, sharesForDeposit); | ||
| } | ||
|
|
||
| // ─── Access control ──────────────────────────────────────────── | ||
|
|
||
| function test_withdrawWithApproval() public { | ||
| vm.prank(alice); | ||
| vault.deposit(1_000 ether, alice); | ||
|
|
||
| // Alice approves Bob to withdraw | ||
| vm.prank(alice); | ||
| vault.approve(bob, 500 ether); | ||
|
|
||
| vm.prank(bob); | ||
| vault.withdraw(500 ether, bob, alice); | ||
|
|
||
| assertEq(asset.balanceOf(bob), 1_000_000 ether + 500 ether); // initial + withdrawn | ||
| } | ||
|
|
||
| function test_revert_withdrawWithoutApproval() public { | ||
| vm.prank(alice); | ||
| vault.deposit(1_000 ether, alice); | ||
|
|
||
| vm.prank(bob); | ||
| vm.expectRevert(); | ||
| vault.withdraw(500 ether, bob, alice); | ||
| } | ||
|
|
||
| // ─── Donation ────────────────────────────────────────────────── | ||
|
|
||
| function test_donate() public { | ||
| vm.prank(alice); | ||
| vault.deposit(1_000 ether, alice); | ||
|
|
||
| uint256 prePrice = vault.convertToAssets(1 ether); | ||
|
|
||
| vm.prank(bob); | ||
| vault.donate(500 ether); | ||
|
|
||
| uint256 postPrice = vault.convertToAssets(1 ether); | ||
| assertGt(postPrice, prePrice); // Share price increased | ||
| assertEq(vault.totalAssets(), 1_500 ether); | ||
| assertEq(vault.balanceOf(bob), 0); // Bob got no shares | ||
| } | ||
|
|
||
| function test_revert_donateZero() public { | ||
| vm.prank(alice); | ||
| vm.expectRevert(Vault4626.Vault4626__ZeroDeposit.selector); | ||
| vault.donate(0); | ||
| } | ||
|
|
||
| // ─── Max functions ───────────────────────────────────────────── | ||
|
|
||
| function test_maxDeposit() public { | ||
| assertEq(vault.maxDeposit(alice), type(uint256).max); | ||
| } | ||
|
|
||
| function test_maxMint() public { | ||
| assertEq(vault.maxMint(alice), type(uint256).max); | ||
| } | ||
|
|
||
| function test_maxWithdraw() public { | ||
| vm.prank(alice); | ||
| vault.deposit(500 ether, alice); | ||
| assertEq(vault.maxWithdraw(alice), 500 ether); | ||
| } | ||
|
|
||
| function test_maxRedeem() public { | ||
| vm.prank(alice); | ||
| vault.deposit(500 ether, alice); | ||
| assertEq(vault.maxRedeem(alice), 500 ether); | ||
| } | ||
|
|
||
| // ─── Preview functions ───────────────────────────────────────── | ||
|
|
||
| function test_previewDeposit() public { | ||
| uint256 shares = vault.previewDeposit(100 ether); | ||
| assertEq(shares, 100 ether); // 1:1 from clean state | ||
| } | ||
|
|
||
| function test_previewMint() public { | ||
| uint256 assets = vault.previewMint(100 ether); | ||
| assertEq(assets, 100 ether); // 1:1 | ||
| } | ||
|
|
||
| function test_previewWithdraw() public { | ||
| vm.prank(alice); | ||
| vault.deposit(1_000 ether, alice); | ||
| // 1000 assets withdrawn costs 1000 shares (1:1) | ||
| assertEq(vault.previewWithdraw(100 ether), 100 ether); | ||
| } | ||
|
|
||
| function test_previewRedeem() public { | ||
| vm.prank(alice); | ||
| vault.deposit(1_000 ether, alice); | ||
| assertEq(vault.previewRedeem(100 ether), 100 ether); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Missing fuzz/invariant coverage for monetary public functions.
The suite is mostly example-based; it does not provide invariant and fuzz coverage for monetary entry points (deposit, mint, withdraw, redeem, donate, previews/max paths). This leaves rounding and state-transition safety underexplored.
As per coding guidelines, "Foundry tests. Want invariant + fuzz coverage on every public function with monetary impact."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/Vault4626.t.sol` around lines 47 - 312, The test suite lacks
fuzz/invariant tests for monetary entrypoints; add property-based/fuzz and
invariant tests covering deposit, mint, withdraw, redeem, donate and all
preview/max functions (convertToAssets/convertToShares,
previewDeposit/previewMint/previewWithdraw/previewRedeem,
maxDeposit/maxMint/maxWithdraw/maxRedeem) to exercise rounding and state
transitions. Implement fuzzed calls with random asset/share amounts and actors,
assert invariants such as totalAssets() == asset.balanceOf(address(vault)) (plus
donated transfers), totalSupply() == DEAD_SHARES + sum(live user balances), no
unexpected reverts for valid inputs, dead shares remain at DEAD and cannot be
redeemed, preview* and max* are consistent with actual operations (e.g.,
previewDeposit(x) <= deposit outcome considering rounding), and that rounding
favors the vault; run these as invariant tests (Forge invariant/fuzz harness) to
catch edge cases.
Comment on lines
+145
to
+183
| function test_inflationAttackBlocked() public { | ||
| // Attacker tries donation attack: donate 1 wei to inflate share price, | ||
| // then front-run victim's deposit. | ||
|
|
||
| // Dead shares exist: 1000 | ||
| assertEq(vault.balanceOf(DEAD), DEAD_SHARES); | ||
|
|
||
| // Attacker deposits 1 wei — pays dearly due to dead-share offset | ||
| vm.prank(charlie); | ||
| uint256 attackerShares = vault.deposit(1, charlie); | ||
|
|
||
| // Attacker gets ~0 shares because 1 asset / (DEAD_SHARES floor) ≈ 0 | ||
| // Actually: convertToShares(1) with supply=DEAD_SHARES, assets=0 | ||
| // _convertToShares: supply > DEAD_SHARES? No (supply==DEAD_SHARES). Return assets directly = 1 | ||
| // Wait — first deposit after dead shares: supply==DEAD_SHARES so return assets == 1 share for 1 wei | ||
| // That's the expected 1:1 start for the first real depositor | ||
|
|
||
| // The real test: attacker donates massive amount, then tries to steal | ||
| // Fund charlie with enough to do the donation AND the deposit | ||
| asset.mint(charlie, 2_000_000 ether); | ||
| vm.startPrank(charlie); | ||
| asset.approve(address(vault), type(uint256).max); | ||
| vault.deposit(1, charlie); // attacker deposits 1 wei | ||
| asset.transfer(address(vault), 1_000_000 ether); // donate via direct transfer | ||
| vm.stopPrank(); | ||
|
|
||
| // Share price is now massive: (1_000_000 + 1) / (DEAD_SHARES + 1) ≈ 999 ether/share | ||
| // Alice deposits 1 ether — gets ~0.001 shares | ||
| vm.prank(alice); | ||
| uint256 aliceShares = vault.deposit(1 ether, alice); | ||
|
|
||
| // Alice should get very few shares due to inflated share price | ||
| assertTrue(aliceShares < 1 ether / 100); // less than 0.01 shares | ||
|
|
||
| // Attacker tries to withdraw their 1 share — gets massive assets | ||
| vm.prank(charlie); | ||
| // charlie has attackerShares shares | ||
| vault.redeem(attackerShares, charlie, charlie); | ||
| } |
There was a problem hiding this comment.
test_inflationAttackBlocked does not actually assert the attack is blocked.
This test currently executes a scenario but never proves the mitigation outcome, and it redeems a stale share count (attackerShares from Line 154) even after a second attacker deposit (Line 167). Add explicit assertions on attacker profit/loss and redeem full attacker balance (vault.balanceOf(charlie)), otherwise this can pass while the vault is still vulnerable.
Proposed test hardening
function test_inflationAttackBlocked() public {
+ uint256 attackerStart = asset.balanceOf(charlie);
vm.prank(charlie);
- uint256 attackerShares = vault.deposit(1, charlie);
+ vault.deposit(1, charlie);
...
vm.startPrank(charlie);
asset.approve(address(vault), type(uint256).max);
vault.deposit(1, charlie); // attacker deposits 1 wei
asset.transfer(address(vault), 1_000_000 ether); // donate via direct transfer
vm.stopPrank();
...
vm.prank(alice);
uint256 aliceShares = vault.deposit(1 ether, alice);
assertTrue(aliceShares < 1 ether / 100);
vm.prank(charlie);
- vault.redeem(attackerShares, charlie, charlie);
+ vault.redeem(vault.balanceOf(charlie), charlie, charlie);
+
+ // mitigation proof: attacker cannot extract more than they put in
+ uint256 attackerEnd = asset.balanceOf(charlie);
+ assertLe(attackerEnd, attackerStart);
}📝 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.
Suggested change
| function test_inflationAttackBlocked() public { | |
| // Attacker tries donation attack: donate 1 wei to inflate share price, | |
| // then front-run victim's deposit. | |
| // Dead shares exist: 1000 | |
| assertEq(vault.balanceOf(DEAD), DEAD_SHARES); | |
| // Attacker deposits 1 wei — pays dearly due to dead-share offset | |
| vm.prank(charlie); | |
| uint256 attackerShares = vault.deposit(1, charlie); | |
| // Attacker gets ~0 shares because 1 asset / (DEAD_SHARES floor) ≈ 0 | |
| // Actually: convertToShares(1) with supply=DEAD_SHARES, assets=0 | |
| // _convertToShares: supply > DEAD_SHARES? No (supply==DEAD_SHARES). Return assets directly = 1 | |
| // Wait — first deposit after dead shares: supply==DEAD_SHARES so return assets == 1 share for 1 wei | |
| // That's the expected 1:1 start for the first real depositor | |
| // The real test: attacker donates massive amount, then tries to steal | |
| // Fund charlie with enough to do the donation AND the deposit | |
| asset.mint(charlie, 2_000_000 ether); | |
| vm.startPrank(charlie); | |
| asset.approve(address(vault), type(uint256).max); | |
| vault.deposit(1, charlie); // attacker deposits 1 wei | |
| asset.transfer(address(vault), 1_000_000 ether); // donate via direct transfer | |
| vm.stopPrank(); | |
| // Share price is now massive: (1_000_000 + 1) / (DEAD_SHARES + 1) ≈ 999 ether/share | |
| // Alice deposits 1 ether — gets ~0.001 shares | |
| vm.prank(alice); | |
| uint256 aliceShares = vault.deposit(1 ether, alice); | |
| // Alice should get very few shares due to inflated share price | |
| assertTrue(aliceShares < 1 ether / 100); // less than 0.01 shares | |
| // Attacker tries to withdraw their 1 share — gets massive assets | |
| vm.prank(charlie); | |
| // charlie has attackerShares shares | |
| vault.redeem(attackerShares, charlie, charlie); | |
| } | |
| function test_inflationAttackBlocked() public { | |
| // Attacker tries donation attack: donate 1 wei to inflate share price, | |
| // then front-run victim's deposit. | |
| // Dead shares exist: 1000 | |
| assertEq(vault.balanceOf(DEAD), DEAD_SHARES); | |
| // Attacker deposits 1 wei — pays dearly due to dead-share offset | |
| uint256 attackerStart = asset.balanceOf(charlie); | |
| vm.prank(charlie); | |
| vault.deposit(1, charlie); | |
| // Attacker gets ~0 shares because 1 asset / (DEAD_SHARES floor) ≈ 0 | |
| // Actually: convertToShares(1) with supply=DEAD_SHARES, assets=0 | |
| // _convertToShares: supply > DEAD_SHARES? No (supply==DEAD_SHARES). Return assets directly = 1 | |
| // Wait — first deposit after dead shares: supply==DEAD_SHARES so return assets == 1 share for 1 wei | |
| // That's the expected 1:1 start for the first real depositor | |
| // The real test: attacker donates massive amount, then tries to steal | |
| // Fund charlie with enough to do the donation AND the deposit | |
| asset.mint(charlie, 2_000_000 ether); | |
| vm.startPrank(charlie); | |
| asset.approve(address(vault), type(uint256).max); | |
| vault.deposit(1, charlie); // attacker deposits 1 wei | |
| asset.transfer(address(vault), 1_000_000 ether); // donate via direct transfer | |
| vm.stopPrank(); | |
| // Share price is now massive: (1_000_000 + 1) / (DEAD_SHARES + 1) ≈ 999 ether/share | |
| // Alice deposits 1 ether — gets ~0.001 shares | |
| vm.prank(alice); | |
| uint256 aliceShares = vault.deposit(1 ether, alice); | |
| // Alice should get very few shares due to inflated share price | |
| assertTrue(aliceShares < 1 ether / 100); // less than 0.01 shares | |
| // Attacker tries to withdraw their 1 share — gets massive assets | |
| vm.prank(charlie); | |
| vault.redeem(vault.balanceOf(charlie), charlie, charlie); | |
| // mitigation proof: attacker cannot extract more than they put in | |
| uint256 attackerEnd = asset.balanceOf(charlie); | |
| assertLe(attackerEnd, attackerStart); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/Vault4626.t.sol` around lines 145 - 183, The test_inflationAttackBlocked
currently never asserts the attack outcome and redeems using the stale
attackerShares variable; update it to redeem the attacker's full current share
balance (use vault.balanceOf(charlie)) and assert attacker cannot profit:
capture charlie's asset balance before redeem, perform
vault.redeem(vault.balanceOf(charlie), charlie, charlie), capture after balance
and assert received_assets <= assets_deposited (or received_assets - deposited
<= acceptable_loss/tolerance); also add an explicit assertion that aliceShares
is small (already present) and assert attacker final asset delta does not exceed
the 1 wei they originally deposited (or does not produce a net gain) to prove
the mitigation. Ensure references: test_inflationAttackBlocked, attackerShares
(stop using stale), vault.balanceOf(charlie), vault.redeem(...), and
aliceShares.
satyakwok
requested changes
May 16, 2026
satyakwok
left a comment
satyakwok
left a comment
Member
There was a problem hiding this comment.
Thanks for the PR. Direction looks good, but I don’t want to merge a canonical ERC-4626 implementation until the share/asset math and tests are stronger.
Please address the CodeRabbit findings before merge, especially:
-
Fix the inflation-attack protection math in
_convertToShares.
The current formula subtracts the dead-share offset from supply but does not apply a matching virtual asset/share offset. Please use a symmetric virtual offset pattern and keep the initial 1:1 special case where appropriate. -
Strengthen
test_inflationAttackBlocked.
Redeem the attacker’s current share balance withvault.balanceOf(charlie)and explicitly assert that the attacker cannot profit. -
Add hook tests with a small subclass overriding
_afterDepositand_beforeWithdraw. -
Add documentation warning about reentrancy considerations for
_beforeWithdrawhook implementations. -
Add at least targeted fuzz/invariant tests for deposit/mint/withdraw/redeem/donate and preview/max consistency.
This is intended to become a canonical contract for the Sentrix ecosystem, so I’d rather keep the standard high before merging.
Member
|
Tests pass locally, but I’d prefer not to merge this yet until the vault accounting assumptions are explicit. Local result: This contract mints Please add or confirm tests covering:
Also, please avoid wording that implies I am not seeing a compiler or test failure here, but I want the accounting assumptions locked down before this is merged. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements a canonical ERC-4626 vault as described in #27. Minimal, OpenZeppelin-derived reference implementation for the Sentrix ecosystem.
What's included
Vault4626.sol
Design decisions
Tests (Vault4626.t.sol)
22 tests covering:
Out of scope (per issue)
Verification
forge test --match-contract Vault4626Test
Suite result: ok. 22 passed; 0 failed; 0 skipped
Closes #27
Summary by CodeRabbit
New Features
Tests