feat: TokenFactoryV2 with ERC-20 Permit + supply cap + freeze controls#35
feat: TokenFactoryV2 with ERC-20 Permit + supply cap + freeze controls#35invisiblemonsters wants to merge 3 commits into
Conversation
- FactoryTokenV2: ERC-20 with EIP-2612 Permit, optional supply cap (ERC20Capped), and freeze controls (Pausable + Ownable). Uses OpenZeppelin v5.3.0. - TokenFactoryV2: deploys FactoryTokenV2, tracks deployments (next to v1). - ITokenFactoryV2: interface for the factory. - Tests: 15 passing (deploy, cap, mint, pause/unpause, permit, transfer, access control). Closes sentrix-labs#30
📝 WalkthroughWalkthroughAdds TokenFactoryV2 + ITokenFactoryV2 and FactoryTokenV2: deploys configurable ERC‑20 tokens with optional EIP‑2612 permit, optional supply cap, pausable controls, and owner minting. Also adds Vault4626, an ERC‑4626 vault that mints permanent dead shares to 0xdead, supports donate(), adjusts conversion math for dead shares, and exposes hooks. Comprehensive Foundry tests are included for factory/token behaviors and vault accounting, share‑price effects, rounding, access control, and donation semantics. Sequence Diagram(s)sequenceDiagram
participant Deployer
participant TokenFactoryV2
participant FactoryTokenV2
Deployer->>TokenFactoryV2: deployToken(name,symbol,decimals,initialSupply,cap,permitEnabled,pauseEnabled)
TokenFactoryV2->>TokenFactoryV2: validate name/symbol lengths
alt cap > 0
TokenFactoryV2->>TokenFactoryV2: require initialSupply <= cap
end
TokenFactoryV2->>FactoryTokenV2: new FactoryTokenV2(...)
TokenFactoryV2->>TokenFactoryV2: deployedTokens[msg.sender].push(token)
TokenFactoryV2->>Deployer: emit TokenDeployed(token, owner, ...)
sequenceDiagram
participant User
participant Vault4626
participant AssetToken
User->>Vault4626: deposit(assets)
Vault4626->>AssetToken: transferFrom(user -> vault, assets)
Vault4626->>Vault4626: mint shares (accounting includes deadShares offset)
Vault4626-->>User: emit Deposit / return shares
User->>Vault4626: donate(assets)
Vault4626->>AssetToken: transferFrom(user -> vault, assets)
Vault4626-->>User: emit Donation (no shares minted)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 4
🤖 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/interfaces/ITokenFactoryV2.sol`:
- Around line 21-26: The ITokenFactoryV2 interface's deployToken function
currently lacks the per-token configuration fields required by Issue `#30`; update
the function signature for deployToken in ITokenFactoryV2.sol to include the
missing parameters decimals (uint8 or uint256 consistent with your codebase),
bool permitEnabled, and bool pauseEnabled so consumers can opt into those
features during deployment; ensure the same parameter order and types align with
the implementing contract(s) and any callers to avoid mismatches, and update any
related docs/tests that reference ITokenFactoryV2::deployToken.
In `@contracts/TokenFactoryV2.sol`:
- Around line 11-20: Add an explicit interface implementation: import the
ITokenFactoryV2 interface (import { ITokenFactoryV2 } from
"./interfaces/ITokenFactoryV2.sol";) at the top of TokenFactoryV2.sol and change
the contract declaration from "contract TokenFactoryV2 {" to "contract
TokenFactoryV2 is ITokenFactoryV2 {" so the contract implements the
ITokenFactoryV2 interface and the compiler enforces the public API (ensure the
import name matches the interface symbol ITokenFactoryV2 and that all
public/external function signatures in TokenFactoryV2 match the interface).
In `@test/TokenFactoryV2.t.sol`:
- Around line 224-235: Add boundary tests around the name/symbol length limits:
extend the test suite that currently contains test_revert_emptyName and
test_revert_emptySymbol by adding tests that call factory.deployToken with a
64-character name and a 16-character symbol (expect success, no revert) and with
a 65-character name and a 17-character symbol (expect revert with
"TokenFactoryV2: BAD_NAME" and "TokenFactoryV2: BAD_SYMBOL" respectively). Use
vm.prank(deployer) and vm.expectRevert where appropriate, reuse
factory.deployToken for the calls, and add distinct test functions (e.g.,
test_deploy_maxName_and_maxSymbol and test_revert_tooLongName_and_tooLongSymbol)
to cover both success and failure boundaries.
- Around line 10-251: Add Foundry fuzz and invariant tests covering
monetary-impact public functions: create fuzz tests (test_fuzz_deployToken,
test_fuzz_mint, test_fuzz_transfer, test_fuzz_transferFrom, test_fuzz_permit)
that randomly exercise deployToken, mint, transfer, transferFrom, permit with
vm.assume to constrain sane values, and add invariants
(invariant_totalSupply_le_cap, invariant_balances_sum_equals_totalSupply,
invariant_balances_nonnegative, invariant_allowances_nonnegative,
invariant_factory_tracking) that assert for any state the
FactoryTokenV2/TokenFactoryV2 invariants hold: if cap > 0 then totalSupply() <=
cap, sum(balanceOf(...) for tracked holders) == totalSupply(), no negative
balances/allowances, owner-only mint reverts for non-owner, permit actually sets
allowance for approved spender, and factory.tokenCount(deployer) and
tokensOf(deployer) remain consistent; write these as standard Foundry test
functions using existing symbols deployToken, mint, transfer, transferFrom,
permit, cap, totalSupply, balanceOf, allowance, tokenCount, tokensOf and use
vm.prank/vm.sign/vm.expectRevert where appropriate.
🪄 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: 6d031f37-2b61-4847-a85b-0ce5a60cdc83
📒 Files selected for processing (4)
contracts/FactoryTokenV2.solcontracts/TokenFactoryV2.solcontracts/interfaces/ITokenFactoryV2.soltest/TokenFactoryV2.t.sol
| contract TokenFactoryV2Test is Test { | ||
| TokenFactoryV2 public factory; | ||
|
|
||
| address deployer = makeAddr("deployer"); | ||
| address alice = makeAddr("alice"); | ||
| address bob = makeAddr("bob"); | ||
|
|
||
| function setUp() public { | ||
| vm.prank(deployer); | ||
| factory = new TokenFactoryV2(); | ||
| } | ||
|
|
||
| /// @notice Basic deploy — no cap, no pause. | ||
| function test_deployBasic() public { | ||
| vm.prank(deployer); | ||
| address token = factory.deployToken("Test Token", "TST", 1_000_000 ether, 0); | ||
|
|
||
| FactoryTokenV2 t = FactoryTokenV2(token); | ||
| assertEq(t.name(), "Test Token"); | ||
| assertEq(t.symbol(), "TST"); | ||
| assertEq(t.totalSupply(), 1_000_000 ether); | ||
| assertEq(t.balanceOf(deployer), 1_000_000 ether); | ||
| assertEq(t.owner(), deployer); | ||
| assertFalse(t.isCapped()); // unlimited | ||
| assertFalse(t.paused()); | ||
| } | ||
|
|
||
| /// @notice Deploy with a supply cap. | ||
| function test_deployWithCap() public { | ||
| uint256 cap = 2_000_000 ether; | ||
| vm.prank(deployer); | ||
| address token = factory.deployToken("Capped", "CAP", 500_000 ether, cap); | ||
|
|
||
| FactoryTokenV2 t = FactoryTokenV2(token); | ||
| assertEq(t.cap(), cap); | ||
| assertEq(t.totalSupply(), 500_000 ether); | ||
| } | ||
|
|
||
| /// @notice Deploy with initialSupply exceeding cap must revert. | ||
| function test_revert_initialExceedsCap() public { | ||
| vm.prank(deployer); | ||
| vm.expectRevert("TokenFactoryV2: CAP_EXCEEDED"); | ||
| factory.deployToken("Bad", "BAD", 2_000_000 ether, 1_000_000 ether); | ||
| } | ||
|
|
||
| /// @notice Mint respects the cap. | ||
| function test_mintCapped() public { | ||
| uint256 cap = 1_000_000 ether; | ||
| vm.prank(deployer); | ||
| address token = factory.deployToken("Capped", "CAP", 0, cap); | ||
| FactoryTokenV2 t = FactoryTokenV2(token); | ||
|
|
||
| vm.prank(deployer); | ||
| t.mint(alice, 600_000 ether); | ||
| assertEq(t.totalSupply(), 600_000 ether); | ||
|
|
||
| // Mint up to cap — should succeed | ||
| vm.prank(deployer); | ||
| t.mint(bob, 400_000 ether); | ||
| assertEq(t.totalSupply(), cap); | ||
|
|
||
| // Exceed cap — must revert | ||
| vm.prank(deployer); | ||
| vm.expectRevert(); | ||
| t.mint(alice, 1); | ||
| } | ||
|
|
||
| /// @notice Mint fails for non-owner. | ||
| function test_revert_mintNotOwner() public { | ||
| vm.prank(deployer); | ||
| address token = factory.deployToken("Owned", "OWN", 100 ether, 0); | ||
| FactoryTokenV2 t = FactoryTokenV2(token); | ||
|
|
||
| vm.prank(alice); | ||
| vm.expectRevert(); | ||
| t.mint(alice, 50 ether); | ||
| } | ||
|
|
||
| /// @notice Transfers work when not paused. | ||
| function test_transfer() public { | ||
| vm.prank(deployer); | ||
| address token = factory.deployToken("Transfer", "XFR", 1_000 ether, 0); | ||
| FactoryTokenV2 t = FactoryTokenV2(token); | ||
|
|
||
| vm.prank(deployer); | ||
| t.transfer(alice, 400 ether); | ||
| assertEq(t.balanceOf(alice), 400 ether); | ||
| assertEq(t.balanceOf(deployer), 600 ether); | ||
| } | ||
|
|
||
| /// @notice Pause stops transfers, unpause restores them. | ||
| function test_pauseUnpause() public { | ||
| vm.prank(deployer); | ||
| address token = factory.deployToken("Pausable", "PAUS", 1_000 ether, 0); | ||
| FactoryTokenV2 t = FactoryTokenV2(token); | ||
|
|
||
| // Transfer works | ||
| vm.prank(deployer); | ||
| t.transfer(alice, 100 ether); | ||
|
|
||
| // Pause | ||
| vm.prank(deployer); | ||
| t.pause(); | ||
| assertTrue(t.paused()); | ||
|
|
||
| // Transfer blocked | ||
| vm.prank(alice); | ||
| vm.expectRevert(); | ||
| t.transfer(bob, 50 ether); | ||
|
|
||
| // Unpause | ||
| vm.prank(deployer); | ||
| t.unpause(); | ||
| assertFalse(t.paused()); | ||
|
|
||
| // Transfer works again | ||
| vm.prank(alice); | ||
| t.transfer(bob, 50 ether); | ||
| assertEq(t.balanceOf(bob), 50 ether); | ||
| } | ||
|
|
||
| /// @notice Only owner can pause. | ||
| function test_revert_pauseNotOwner() public { | ||
| vm.prank(deployer); | ||
| address token = factory.deployToken("Owned", "OWN", 100 ether, 0); | ||
| FactoryTokenV2 t = FactoryTokenV2(token); | ||
|
|
||
| vm.prank(alice); | ||
| vm.expectRevert(); | ||
| t.pause(); | ||
| } | ||
|
|
||
| /// @notice EIP-2612 permit works for gasless approvals. | ||
| function test_permit() public { | ||
| uint256 ownerKey = 0xA11CE; | ||
| address owner = vm.addr(ownerKey); | ||
|
|
||
| // Deploy token with alice as owner so she has tokens | ||
| vm.prank(owner); | ||
| address tokenAddr = factory.deployToken("Permit", "PRMT", 1_000 ether, 0); | ||
| FactoryTokenV2 t = FactoryTokenV2(tokenAddr); | ||
|
|
||
| // Build a permit | ||
| uint256 value = 100 ether; | ||
| uint256 deadline = block.timestamp + 1 days; | ||
|
|
||
| // Sign permit off-chain | ||
| bytes32 structHash = keccak256( | ||
| abi.encode( | ||
| keccak256("Permit(address owner,address spender,uint256 value,uint256 nonce,uint256 deadline)"), | ||
| owner, | ||
| bob, | ||
| value, | ||
| t.nonces(owner), | ||
| deadline | ||
| ) | ||
| ); | ||
|
|
||
| bytes32 domainSeparator = t.DOMAIN_SEPARATOR(); | ||
| bytes32 digest = keccak256( | ||
| abi.encodePacked("\x19\x01", domainSeparator, structHash) | ||
| ); | ||
|
|
||
| (uint8 v, bytes32 r, bytes32 s) = vm.sign(ownerKey, digest); | ||
|
|
||
| // Execute permit (anyone can call) | ||
| t.permit(owner, bob, value, deadline, v, r, s); | ||
|
|
||
| // Bob can now transferFrom | ||
| vm.prank(bob); | ||
| t.transferFrom(owner, bob, value); | ||
| assertEq(t.balanceOf(bob), value); | ||
| } | ||
|
|
||
| /// @notice Supply cap works with mint() from owner. | ||
| function test_mintToCap() public { | ||
| vm.prank(deployer); | ||
| address token = factory.deployToken("CappedMint", "CMT", 10_000 ether, 10_000 ether); | ||
| FactoryTokenV2 t = FactoryTokenV2(token); | ||
|
|
||
| // Already at cap — mint should revert | ||
| vm.prank(deployer); | ||
| vm.expectRevert(); | ||
| t.mint(deployer, 1); | ||
| } | ||
|
|
||
| /// @notice Zero cap means unlimited. | ||
| function test_unlimitedCap() public { | ||
| vm.prank(deployer); | ||
| address token = factory.deployToken("Unlimited", "UNL", 1_000 ether, 0); | ||
| FactoryTokenV2 t = FactoryTokenV2(token); | ||
|
|
||
| // Mint a large amount — should succeed | ||
| vm.prank(deployer); | ||
| t.mint(alice, 1_000_000_000 ether); | ||
| assertEq(t.totalSupply(), 1_000_001_000 ether); | ||
| } | ||
|
|
||
| /// @notice Factory tracks deployed tokens. | ||
| function test_factoryTracking() public { | ||
| vm.startPrank(deployer); | ||
|
|
||
| address t1 = factory.deployToken("One", "ONE", 100 ether, 0); | ||
| address t2 = factory.deployToken("Two", "TWO", 200 ether, 0); | ||
|
|
||
| assertEq(factory.tokenCount(deployer), 2); | ||
|
|
||
| address[] memory tokens = factory.tokensOf(deployer); | ||
| assertEq(tokens[0], t1); | ||
| assertEq(tokens[1], t2); | ||
|
|
||
| vm.stopPrank(); | ||
| } | ||
|
|
||
| /// @notice Name/symbol validation. | ||
| function test_revert_emptyName() public { | ||
| vm.prank(deployer); | ||
| vm.expectRevert("TokenFactoryV2: BAD_NAME"); | ||
| factory.deployToken("", "SYM", 100 ether, 0); | ||
| } | ||
|
|
||
| function test_revert_emptySymbol() public { | ||
| vm.prank(deployer); | ||
| vm.expectRevert("TokenFactoryV2: BAD_SYMBOL"); | ||
| factory.deployToken("Name", "", 100 ether, 0); | ||
| } | ||
|
|
||
| /// @notice TransferFrom with allowance works. | ||
| function test_transferFrom() public { | ||
| vm.prank(deployer); | ||
| address token = factory.deployToken("Approve", "APR", 1_000 ether, 0); | ||
| FactoryTokenV2 t = FactoryTokenV2(token); | ||
|
|
||
| vm.prank(deployer); | ||
| t.approve(alice, 300 ether); | ||
|
|
||
| vm.prank(alice); | ||
| t.transferFrom(deployer, bob, 300 ether); | ||
| assertEq(t.balanceOf(bob), 300 ether); | ||
| assertEq(t.allowance(deployer, alice), 0); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Add invariant + fuzz coverage for value-moving public paths.
This suite is currently example-based only. Add Foundry fuzz/invariant tests for monetary-impact flows (deployToken, mint, transfer, transferFrom, permit) with invariants like capped totalSupply <= cap and balance/supply consistency.
As per coding guidelines test/**/*.sol: 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/TokenFactoryV2.t.sol` around lines 10 - 251, Add Foundry fuzz and
invariant tests covering monetary-impact public functions: create fuzz tests
(test_fuzz_deployToken, test_fuzz_mint, test_fuzz_transfer,
test_fuzz_transferFrom, test_fuzz_permit) that randomly exercise deployToken,
mint, transfer, transferFrom, permit with vm.assume to constrain sane values,
and add invariants (invariant_totalSupply_le_cap,
invariant_balances_sum_equals_totalSupply, invariant_balances_nonnegative,
invariant_allowances_nonnegative, invariant_factory_tracking) that assert for
any state the FactoryTokenV2/TokenFactoryV2 invariants hold: if cap > 0 then
totalSupply() <= cap, sum(balanceOf(...) for tracked holders) == totalSupply(),
no negative balances/allowances, owner-only mint reverts for non-owner, permit
actually sets allowance for approved spender, and factory.tokenCount(deployer)
and tokensOf(deployer) remain consistent; write these as standard Foundry test
functions using existing symbols deployToken, mint, transfer, transferFrom,
permit, cap, totalSupply, balanceOf, allowance, tokenCount, tokensOf and use
vm.prank/vm.sign/vm.expectRevert where appropriate.
- 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
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 108-113: Add ReentrancyGuard inheritance and apply nonReentrant to
the external entry points so deposits/withdrawals cannot be reentered: import
OpenZeppelin's ReentrancyGuard, have the contract inherit ReentrancyGuard, and
add the nonReentrant modifier to the public deposit and withdraw functions (the
external entry points that call internal _deposit and _withdraw/_burn logic);
leave internal helpers like _deposit and _afterDeposit unchanged so derived
contracts still run but are protected by the nonReentrant guard at the
boundaries.
- Around line 42-68: The current dead-share shortcut in _convertToShares and
_convertToAssets returns a 1:1 mapping when totalSupply() == 1e3, letting the
first depositor steal donated assets; instead, when supply == 1e3 and
totalAssets() > 0 compute conversions proportionally using the dead-share offset
(i.e., treat the virtual offset as added to both sides): in _convertToShares
replace the `if (supply == 1e3) return assets;` branch with a proportional
calculation like assets * supply / totalAssets (use _tryMulDiv and Math.Rounding
as in the rest), and in _convertToAssets replace the `if (supply == 1e3) return
shares;` branch with shares * totalAssets / supply (again via _tryMulDiv); keep
the special-case zero-totalAssets behavior (if totalAssets() == 0 fall back to
1:1) and reuse the DEAD_SHARES constant (1e3), totalSupply(), totalAssets(),
_convertToShares, and _convertToAssets identifiers to locate changes.
- Around line 129-138: The helper _tryMulDiv currently returns 0 when
denominator == 0 which can silently lose funds; change it to revert instead: in
function _tryMulDiv(uint256 x, uint256 y, uint256 denominator, Math.Rounding
rounding) check denominator and revert (use a clear message or a custom error
like DivisionByZero) if denominator == 0, otherwise return x.mulDiv(y,
denominator, rounding); keep the rest of the logic and signature unchanged so
callers of _tryMulDiv detect the failure instead of receiving a silent zero.
In `@test/Vault4626.t.sol`:
- Around line 47-312: The test suite lacks fuzz and invariant tests for
monetary-impact public methods; add property-based/fuzz tests and invariants
covering deposit, mint, withdraw, redeem, donate and their preview/max helpers
(convertToAssets/convertToShares,
previewDeposit/previewMint/previewWithdraw/previewRedeem,
maxDeposit/maxMint/maxWithdraw/maxRedeem) to exercise adversarial scenarios
(random assets/shares, multiple actors, direct asset transfers) and assert core
invariants (totalAssets vs totalSupply relation, non-negative balances,
dead-share immutability for DEAD/DEAD_SHARES, rounding favors vault,
approvals/allowances behavior) using Foundry’s fuzz and invariant harness APIs
in this test file so each public monetary path is fuzzed and an invariant test
asserts preserved properties across operations.
- Around line 154-183: The test captures attackerShares too early and doesn't
assert any outcome; update the scenario so attackerShares is read after the
attacker deposit (use vault.deposit when charlie deposits 1 wei) and then assert
economic outcomes: record asset balances (or vault.totalAssets()) for charlie
and alice before and after vault.redeem(attackerShares, charlie, charlie) and
assert charlie's redeemed assets do not exceed his legitimate entitlement (e.g.,
his pre-deposit + reasonable expected yield) and that alice's shares/asset loss
matches the inflated-share effect (e.g., aliceShares is very small and alice's
post-deposit asset balance decreased by ~1 ether while vault assets reflect the
direct transfer donation). Refer to attackerShares, vault.deposit, vault.redeem,
aliceShares, charlie and alice when adding these checks so the test fails if the
inflation donation allows theft.
- Around line 239-241: Replace the broad revert assertion with a specific ERC20
allowance error: instead of calling vm.expectRevert() before calling
vault.withdraw(500 ether, bob, alice), use
vm.expectRevert(ERC20.ERC20InsufficientAllowance.selector) so the test asserts
the withdraw call (vault.withdraw) fails for the expected
ERC20InsufficientAllowance error selector; update the assertion that precedes
the vault.withdraw invocation accordingly.
🪄 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: 18967d91-159b-40ff-8fd7-a414d6f5a445
📒 Files selected for processing (2)
contracts/Vault4626.soltest/Vault4626.t.sol
| 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); | ||
| } | ||
|
|
||
| /// @inheritdoc ERC4626 | ||
| /// @dev Rounds DOWN (protects vault). Uses mulDiv with Math.Rounding.Floor. | ||
| function _convertToAssets(uint256 shares, Math.Rounding rounding) | ||
| internal | ||
| view | ||
| override | ||
| returns (uint256) | ||
| { | ||
| uint256 supply = totalSupply(); | ||
| // If only dead shares exist, assets = shares (1:1) | ||
| if (supply == 1e3) return shares; | ||
|
|
||
| return _tryMulDiv(shares, totalAssets(), supply - 1e3, rounding); | ||
| } |
There was a problem hiding this comment.
Critical: Dead-share accounting allows first depositor to steal donated assets.
The conversion logic returns 1:1 when supply == 1e3 regardless of totalAssets. If assets are donated before the first real deposit, the first depositor can drain them:
- After constructor:
supply=1000,totalAssets=0 - Donation of 10,000 assets:
totalAssets=10000,supply=1000 - User deposits 1,000:
supply==1e3→ user receives 1,000 shares (1:1) - Now:
supply=2000,totalAssets=11000 - User redeems 1,000 shares:
assets = 1000 * 11000 / (2000-1000) = 11000
The user deposited 1,000 but withdraws 11,000—stealing the entire donation.
The fix should use a proper virtual offset that adds to both numerator and denominator rather than subtracting dead shares only from supply. Alternatively, when supply == 1e3, compute shares as assets * 1e3 / totalAssets (if totalAssets > 0) to properly dilute against existing assets.
🤖 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 - 68, The current dead-share
shortcut in _convertToShares and _convertToAssets returns a 1:1 mapping when
totalSupply() == 1e3, letting the first depositor steal donated assets; instead,
when supply == 1e3 and totalAssets() > 0 compute conversions proportionally
using the dead-share offset (i.e., treat the virtual offset as added to both
sides): in _convertToShares replace the `if (supply == 1e3) return assets;`
branch with a proportional calculation like assets * supply / totalAssets (use
_tryMulDiv and Math.Rounding as in the rest), and in _convertToAssets replace
the `if (supply == 1e3) return shares;` branch with shares * totalAssets /
supply (again via _tryMulDiv); keep the special-case zero-totalAssets behavior
(if totalAssets() == 0 fall back to 1:1) and reuse the DEAD_SHARES constant
(1e3), totalSupply(), totalAssets(), _convertToShares, and _convertToAssets
identifiers to locate changes.
| function _deposit(address caller, address receiver, uint256 assets, uint256 shares) internal override { | ||
| SafeERC20.safeTransferFrom(IERC20(asset()), caller, address(this), assets); | ||
| _mint(receiver, shares); | ||
| _afterDeposit(caller, assets, shares); | ||
| emit Deposit(caller, receiver, assets, shares); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚖️ Poor tradeoff
Consider adding ReentrancyGuard for defense-in-depth.
While the current implementation has no obvious reentrancy exploit, the _deposit function performs external calls before state changes (safeTransferFrom before _mint). If the underlying asset has transfer callbacks (ERC-777) and derived contracts add stateful logic in _afterDeposit, unexpected interactions could arise.
Adding nonReentrant to deposit/withdraw entry points would provide defense-in-depth as yield-strategy children extend this contract.
🤖 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 108 - 113, Add ReentrancyGuard
inheritance and apply nonReentrant to the external entry points so
deposits/withdrawals cannot be reentered: import OpenZeppelin's ReentrancyGuard,
have the contract inherit ReentrancyGuard, and add the nonReentrant modifier to
the public deposit and withdraw functions (the external entry points that call
internal _deposit and _withdraw/_burn logic); leave internal helpers like
_deposit and _afterDeposit unchanged so derived contracts still run but are
protected by the nonReentrant guard at the boundaries.
| /// @dev Safe mulDiv that returns 0 when operands overflow, with configurable rounding. | ||
| function _tryMulDiv( | ||
| uint256 x, | ||
| uint256 y, | ||
| uint256 denominator, | ||
| Math.Rounding rounding | ||
| ) internal pure returns (uint256) { | ||
| if (denominator == 0) return 0; | ||
| return x.mulDiv(y, denominator, rounding); | ||
| } |
There was a problem hiding this comment.
Silent zero return on division-by-zero can cause loss of funds.
When denominator == 0, this returns 0 rather than reverting. If a depositor calls when totalAssets() == 0 but supply > 1e3 (possible with fee-on-transfer tokens or edge scenarios), they would receive 0 shares for their deposit—silently losing funds.
Prefer reverting on zero denominator to fail-safe rather than fail-silent:
Proposed fix
function _tryMulDiv(
uint256 x,
uint256 y,
uint256 denominator,
Math.Rounding rounding
) internal pure returns (uint256) {
- if (denominator == 0) return 0;
+ require(denominator > 0, "Vault4626: zero denominator");
return x.mulDiv(y, denominator, rounding);
}🤖 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 129 - 138, The helper _tryMulDiv
currently returns 0 when denominator == 0 which can silently lose funds; change
it to revert instead: in function _tryMulDiv(uint256 x, uint256 y, uint256
denominator, Math.Rounding rounding) check denominator and revert (use a clear
message or a custom error like DivisionByZero) if denominator == 0, otherwise
return x.mulDiv(y, denominator, rounding); keep the rest of the logic and
signature unchanged so callers of _tryMulDiv detect the failure instead of
receiving a silent zero.
| 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
Add fuzz + invariant coverage for monetary paths.
This suite is example-based only; it does not include fuzz/invariant tests for monetary-impact public functions (deposit, mint, withdraw, redeem, donate, and related preview/max behavior), so it misses the required adversarial coverage profile.
As per coding guidelines, test/**/*.sol: 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 and
invariant tests for monetary-impact public methods; add property-based/fuzz
tests and invariants covering deposit, mint, withdraw, redeem, donate and their
preview/max helpers (convertToAssets/convertToShares,
previewDeposit/previewMint/previewWithdraw/previewRedeem,
maxDeposit/maxMint/maxWithdraw/maxRedeem) to exercise adversarial scenarios
(random assets/shares, multiple actors, direct asset transfers) and assert core
invariants (totalAssets vs totalSupply relation, non-negative balances,
dead-share immutability for DEAD/DEAD_SHARES, rounding favors vault,
approvals/allowances behavior) using Foundry’s fuzz and invariant harness APIs
in this test file so each public monetary path is fuzzed and an invariant test
asserts preserved properties across operations.
| 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.
Inflation-attack test does not prove safety and uses stale share accounting.
attackerShares is captured before the later attacker deposit, and the test has no assertion on attacker profit/loss or victim outcome after redemption. This can pass without validating that the attack is actually blocked.
Suggested tightening
- vm.prank(charlie);
- uint256 attackerShares = vault.deposit(1, charlie);
+ // Remove the early standalone attacker deposit; capture shares only in the attack sequence.
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
+ uint256 attackerShares = vault.deposit(1, charlie); // attacker deposit used in attack
+ uint256 attackerPre = asset.balanceOf(charlie);
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);
+ uint256 redeemed = vault.redeem(attackerShares, charlie, charlie);
+ uint256 attackerPost = asset.balanceOf(charlie);
+ assertLe(attackerPost, attackerPre + redeemed); // no hidden gain path🤖 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 154 - 183, The test captures
attackerShares too early and doesn't assert any outcome; update the scenario so
attackerShares is read after the attacker deposit (use vault.deposit when
charlie deposits 1 wei) and then assert economic outcomes: record asset balances
(or vault.totalAssets()) for charlie and alice before and after
vault.redeem(attackerShares, charlie, charlie) and assert charlie's redeemed
assets do not exceed his legitimate entitlement (e.g., his pre-deposit +
reasonable expected yield) and that alice's shares/asset loss matches the
inflated-share effect (e.g., aliceShares is very small and alice's post-deposit
asset balance decreased by ~1 ether while vault assets reflect the direct
transfer donation). Refer to attackerShares, vault.deposit, vault.redeem,
aliceShares, charlie and alice when adding these checks so the test fails if the
inflation donation allows theft.
| vm.prank(bob); | ||
| vm.expectRevert(); | ||
| vault.withdraw(500 ether, bob, alice); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the test file
find . -name "Vault4626.t.sol" -type fRepository: sentrix-labs/canonical-contracts
Length of output: 99
🏁 Script executed:
# Read the test file to see the context around lines 239-241
wc -l test/Vault4626.t.solRepository: sentrix-labs/canonical-contracts
Length of output: 101
🏁 Script executed:
# Read the relevant section with more context
sed -n '230,250p' test/Vault4626.t.solRepository: sentrix-labs/canonical-contracts
Length of output: 728
🏁 Script executed:
# Find the Vault4626 contract implementation
find . -name "*.sol" -type f | grep -i vault | grep -v testRepository: sentrix-labs/canonical-contracts
Length of output: 102
🏁 Script executed:
# Search for Vault4626 contract definition
rg "contract Vault4626" --type solRepository: sentrix-labs/canonical-contracts
Length of output: 104
🏁 Script executed:
# Search for Vault4626 contract without file type filter
rg "contract Vault4626" --type-add 'sol:*.sol'Repository: sentrix-labs/canonical-contracts
Length of output: 186
🏁 Script executed:
# Read the Vault4626 contract to find withdraw function
cat -n contracts/Vault4626.sol | head -100Repository: sentrix-labs/canonical-contracts
Length of output: 4910
🏁 Script executed:
# Read more of the contract to find the withdraw function
cat -n contracts/Vault4626.sol | sed -n '100,200p'Repository: sentrix-labs/canonical-contracts
Length of output: 1877
🏁 Script executed:
# Check the OpenZeppelin version
cat contracts/Vault4626.sol | grep -i "openzeppelin"Repository: sentrix-labs/canonical-contracts
Length of output: 527
🏁 Script executed:
# Check package.json or foundry.toml for OpenZeppelin version
find . -name "foundry.toml" -o -name "package.json" | xargs cat 2>/dev/nullRepository: sentrix-labs/canonical-contracts
Length of output: 2299
🏁 Script executed:
# Check OpenZeppelin ERC20 to see what error _spendAllowance throws
# First find if there's a lib/openzeppelin path
find . -path "*/openzeppelin*" -name "ERC20.sol" | head -1Repository: sentrix-labs/canonical-contracts
Length of output: 58
🏁 Script executed:
# Search for ERC20InsufficientAllowance error in the codebase
rg "ERC20InsufficientAllowance" --type-add 'sol:*.sol'Repository: sentrix-labs/canonical-contracts
Length of output: 58
🏁 Script executed:
# Check what errors are defined or used in the test file
rg "expectRevert" test/Vault4626.t.sol --type-add 'sol:*.sol' -A 2 -B 2Repository: sentrix-labs/canonical-contracts
Length of output: 362
🌐 Web query:
OpenZeppelin ERC20 insufficient allowance error 0.8.24
💡 Result:
The "ERC20: insufficient allowance" error in OpenZeppelin ERC20 contracts occurs when a contract attempts to execute a transferFrom function without having been previously authorized to spend the required amount of tokens on behalf of the token owner [1][2][3]. Key points regarding this error: 1. Mechanism: The transferFrom(address sender, address recipient, uint256 amount) function requires that the caller (the spender) has been granted an allowance by the sender [2][3]. This allowance must be greater than or equal to the amount being transferred [4][2]. 2. Common Cause: This error is frequently encountered when a user or contract calls transferFrom without first calling the approve(address spender, uint256 amount) function on the token contract to authorize the spender [5][1]. 3. Versioning and Errors: - In OpenZeppelin Contracts v4.x and earlier, this error is typically thrown as a require statement with the string "ERC20: insufficient allowance" [6][7]. - In OpenZeppelin Contracts v5.x, the library transitioned to using custom errors (e.g., ERC20InsufficientAllowance) to save gas and provide more structured error data [8][3]. If you are using v5.x, you should expect a custom error rather than a string-based revert [8]. 4. Troubleshooting: - Verify that the approve function was called successfully by the token owner for the spender address before the transferFrom call [1]. - Ensure the allowance amount is sufficient for the intended transfer [5]. - If you are developing a contract that needs to move tokens, remember that the contract itself must be approved to spend the user's tokens, or the user must approve the contract address [1]. If you are seeing this error on a transfer function (rather than transferFrom), it is often a sign that your contract logic is incorrectly calling transferFrom or that there is a misunderstanding of which function is being executed [9]. Always check the stack trace to confirm which function is triggering the revert [9].
Citations:
- 1: https://ethereum.stackexchange.com/questions/121479/error-erc20-insufficient-allowance
- 2: https://old-docs.openzeppelin.com/contracts/5.x/api/token/ERC20
- 3: https://docs.openzeppelin.com/contracts/5.x/api/token/ERC20
- 4: https://docs.openzeppelin.com/contracts-cairo/3.x/api/erc20
- 5: https://forum.openzeppelin.com/t/insufficient-allowance-error/42903
- 6: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/9425a7e0ab1d45c48845d47afc33856ad9d98b0c/contracts/token/ERC20/ERC20.sol
- 7: https://docs.openzeppelin.com/contracts/4.x/api/token/ERC20
- 8: https://ethereum.stackexchange.com/questions/163715/assertionerror-expected-transaction-to-be-reverted-with-reason-erc20-transfer
- 9: https://ethereum.stackexchange.com/questions/130689/execution-reverted-erc20-insufficient-allowance-on-transfer-method-not-tran
🏁 Script executed:
# Check for remappings in foundry.toml or remappings.txt
find . -name "remappings.txt" -o -name ".remappings.json" | xargs cat 2>/dev/nullRepository: sentrix-labs/canonical-contracts
Length of output: 212
🏁 Script executed:
# Check lib directory for openzeppelin version
ls -la lib/ 2>/dev/null | head -20Repository: sentrix-labs/canonical-contracts
Length of output: 318
🏁 Script executed:
# Check if there's a package-lock or pnpm lockfile that shows OZ version
find . -name "package-lock.json" -o -name "pnpm-lock.yaml" | head -1Repository: sentrix-labs/canonical-contracts
Length of output: 93
🏁 Script executed:
# Check the openzeppelin-contracts version
cat lib/openzeppelin-contracts/package.json | grep versionRepository: sentrix-labs/canonical-contracts
Length of output: 148
🏁 Script executed:
# Look for ERC20 implementation in the lib to see what error it uses
find lib/openzeppelin-contracts -name "ERC20.sol" | xargs cat -n | head -150Repository: sentrix-labs/canonical-contracts
Length of output: 58
🏁 Script executed:
# Check what's in the openzeppelin-contracts directory
ls -la lib/openzeppelin-contracts/Repository: sentrix-labs/canonical-contracts
Length of output: 183
🏁 Script executed:
# Look for ERC20.sol in the lib
find lib -name "ERC20.sol" -type fRepository: sentrix-labs/canonical-contracts
Length of output: 58
🏁 Script executed:
# Read the ERC20 implementation to find the error type
cat lib/openzeppelin-contracts/contracts/token/ERC20/ERC20.sol | grep -A 10 "insufficientAllowance\|InsufficientAllowance"Repository: sentrix-labs/canonical-contracts
Length of output: 167
🏁 Script executed:
# Check git submodules
git config --file .gitmodules --name-only --get-regexp pathRepository: sentrix-labs/canonical-contracts
Length of output: 147
🏁 Script executed:
# Check what Solidity version is specified and infer OZ version
cat foundry.toml | grep -i solcRepository: sentrix-labs/canonical-contracts
Length of output: 92
🏁 Script executed:
# Search in the test file for any imports that show OZ error types
rg "import.*ERC20" test/Vault4626.t.sol --type-add 'sol:*.sol'Repository: sentrix-labs/canonical-contracts
Length of output: 216
🏁 Script executed:
# Check if ERC20InsufficientAllowance is used anywhere in the codebase
rg "ERC20InsufficientAllowance" . --type-add 'sol:*.sol'Repository: sentrix-labs/canonical-contracts
Length of output: 58
🏁 Script executed:
# Check OpenZeppelin v5.x docs for ERC20InsufficientAllowance error
# First, let's check if there are any other tests using ERC20 errors
rg "Insufficient" test/ --type-add 'sol:*.sol'Repository: sentrix-labs/canonical-contracts
Length of output: 58
🏁 Script executed:
# Look at the full test file to understand the pattern
cat -n test/Vault4626.t.sol | sed -n '1,50p'Repository: sentrix-labs/canonical-contracts
Length of output: 2094
🏁 Script executed:
# Check if there are other tests in the file using expectRevert with selectors
cat -n test/Vault4626.t.sol | grep -A 2 "expectRevert"Repository: sentrix-labs/canonical-contracts
Length of output: 300
Use a specific revert selector for the unauthorized withdraw check.
A bare vm.expectRevert() is too broad and can pass for unrelated failures. The withdraw should revert with ERC20InsufficientAllowance when the caller lacks approval. Match the expected error selector:
vm.expectRevert(ERC20.ERC20InsufficientAllowance.selector);
This follows the pattern already established in the same test file (line 263 with Vault4626.Vault4626__ZeroDeposit.selector).
🤖 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 239 - 241, Replace the broad revert
assertion with a specific ERC20 allowance error: instead of calling
vm.expectRevert() before calling vault.withdraw(500 ether, bob, alice), use
vm.expectRevert(ERC20.ERC20InsufficientAllowance.selector) so the test asserts
the withdraw call (vault.withdraw) fails for the expected
ERC20InsufficientAllowance error selector; update the assertion that precedes
the vault.withdraw invocation accordingly.
satyakwok
left a comment
satyakwok
left a comment
There was a problem hiding this comment.
Thanks for the PR. Direction looks good, but I don’t want to merge this yet because TokenFactoryV2 is intended to be a canonical contract and the public API/tests need to be tighter.
Please address these before merge:
-
Update ITokenFactoryV2 so deployToken exposes the full V2 configuration:
- decimals
- permitEnabled
- pauseEnabled
-
Make TokenFactoryV2 explicitly implement ITokenFactoryV2 so the compiler enforces the public API.
-
Add boundary tests for name/symbol limits:
- 64-character name succeeds
- 65-character name reverts
- 16-character symbol succeeds
- 17-character symbol reverts
-
Add targeted fuzz/invariant tests for deployToken, mint, transfer, transferFrom, permit, cap enforcement, factory tracking, and totalSupply consistency.
-
Please keep this PR scoped only to TokenFactoryV2. If Vault4626 changes are included, move them to the separate Vault4626 PR.
After changes, please run:
- forge fmt --check
- forge build
- forge test
- forge test --match-contract TokenFactoryV2Test
Address review feedback: - Add decimals, permitEnabled, pauseEnabled to ITokenFactoryV2.deployToken + event - Add to TokenFactoryV2 for compiler-enforced API - Configurable decimals in FactoryTokenV2 (override decimals()) - Pause controls gated by pauseEnabled flag - Boundary tests: name max 64/symbol max 16, pause-disabled revert, custom decimals
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/FactoryTokenV2.sol`:
- Around line 40-41: Store the _permitEnabled flag in the contract state (e.g.,
a public bool permitEnabled set in the constructor) and explicitly disable
EIP‑2612 by overriding the inherited permit(...) function to revert (or
require(permitEnabled)) when permitEnabled is false; keep calling
ERC20Permit(...) in the constructor for domain setup if you want but rely on the
stored permitEnabled flag and the overridden permit method to actually prevent
use when disabled, and expose the flag via a public getter so callers can query
whether permit was intended to be enabled.
In `@test/TokenFactoryV2.t.sol`:
- Around line 362-374: Extend test_deployPermitDisabled to assert that permit()
is rejected when permitEnabled is false: after deploying via factory.deployToken
and casting to FactoryTokenV2 (t), construct a permit call using t.permit(...)
or the helper that signs and call it via vm.prank(signer) and expectRevert (or
vm.expectRevert) before calling t.permit to ensure the transaction reverts when
permitEnabled is false; reference test_deployPermitDisabled,
factory.deployToken, FactoryTokenV2, and the permit() method so the negative
assertion checks the feature toggle behaviorally.
🪄 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: 82e722a6-3a7d-41c6-9439-5e0f57f7fc07
📒 Files selected for processing (4)
contracts/FactoryTokenV2.solcontracts/TokenFactoryV2.solcontracts/interfaces/ITokenFactoryV2.soltest/TokenFactoryV2.t.sol
| ERC20Permit(_permitEnabled ? _name : "") // permit salt must be non-empty when enabled | ||
| ERC20Capped(_cap == 0 ? type(uint256).max : _cap) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Check how permitEnabled is used in FactoryTokenV2:"
rg -n "permitEnabled|ERC20Permit\\(|function permit\\(" contracts/FactoryTokenV2.sol
echo
echo "2) Inspect OpenZeppelin ERC20Permit constructor + permit function:"
PERMIT_FILE="$(fd -i 'ERC20Permit.sol' | head -n1 || true)"
if [ -z "${PERMIT_FILE:-}" ]; then
echo "ERC20Permit.sol not found in-repo (dependency may be external)."
exit 0
fi
echo "Found: $PERMIT_FILE"
rg -n "constructor\\(|function permit\\(|function DOMAIN_SEPARATOR\\(" "$PERMIT_FILE"Repository: sentrix-labs/canonical-contracts
Length of output: 461
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== 1) Read full FactoryTokenV2 constructor and permit-related code ==="
cat -n contracts/FactoryTokenV2.sol | head -100
echo
echo "=== 2) Check for any permit overrides in FactoryTokenV2 ==="
rg -n "override.*permit|function permit" contracts/FactoryTokenV2.sol || echo "No permit overrides found"
echo
echo "=== 3) Search for tests referencing permit behavior ==="
fd -e test.ts -e test.sol -e spec.ts -o -path '*test*' -name '*FactoryToken*' | head -5Repository: sentrix-labs/canonical-contracts
Length of output: 4966
🌐 Web query:
OpenZeppelin ERC20Permit constructor empty name parameter behavior domain separator permit function
💡 Result:
In OpenZeppelin's ERC20Permit implementation, the constructor accepts a string parameter name which is passed to the EIP712 base contract [1][2]. If you provide an empty string ("") as the name parameter, the contract will function technically, but it will result in a domain separator that is derived from an empty name [1][3]. Specifically, the EIP712 domain separator is calculated using the hash of the name, the version, the chain ID, and the contract address [4][3][5]. If the name is an empty string, its hash (keccak256(bytes(""))) is 0xc5d2460186f7233c927e7db2dcc703c0e500b653ca82273b7bfad8045d85a470. This value will be used in the domain separator calculation [3]. While this is not a functional error that prevents the permit function from working, it is generally discouraged for the following reasons: 1. Domain Uniqueness: The name parameter is intended to provide a human-readable identifier for the signing domain [4][6]. Using an empty string makes the domain separator less unique and potentially confusing for users or wallet interfaces that display the signing domain [6][5]. 2. Best Practices: OpenZeppelin documentation explicitly recommends using the same name that is defined as the ERC-20 token name [1][2]. This ensures that the domain separator is tied to the specific identity of the token, which is a core purpose of EIP-712 domain separation [5]. 3. Wallet Compatibility: Wallets and other EIP-712-compliant tools often use the name field to present the signing context to the user [6][5]. An empty name may lead to poor user experience or security warnings in some wallet implementations that expect a descriptive name [6]. In summary, passing an empty string will not cause the contract to revert or fail to generate a domain separator, but it will result in a non-descriptive domain separator that deviates from standard practices and may negatively impact user experience and security clarity [1][6][5].
Citations:
- 1: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/extensions/ERC20Permit.sol
- 2: https://docs.openzeppelin.com/contracts/5.x/api/token/ERC20
- 3: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/cryptography/EIP712.sol
- 4: https://evmauth.io/lib/openzeppelin-contracts-upgradeable/lib/openzeppelin-contracts/contracts/utils/cryptography/EIP712.sol/abstract.EIP712.html
- 5: https://ercs.ethereum.org/ERCS/erc-2612
- 6: https://github.com/ethereum/EIPs/blob/master/EIPS/eip-712.md
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Search for test files referencing FactoryTokenV2 or permit ==="
find . -type f \( -name "*.test.ts" -o -name "*.spec.ts" -o -name "*.test.js" -o -name "*.test.sol" \) 2>/dev/null | head -20
echo
echo "=== Search for permit-related tests in test directory ==="
if [ -d "test" ] || [ -d "tests" ]; then
rg -l "permit|FactoryTokenV2" --type ts --type js test* 2>/dev/null | head -10 || echo "No matches"
fi
echo
echo "=== Verify _permitEnabled is stored as immutable or used elsewhere ==="
rg -n "_permitEnabled" contracts/FactoryTokenV2.solRepository: sentrix-labs/canonical-contracts
Length of output: 484
permitEnabled flag does not disable permit() in its current implementation.
The constructor passes _permitEnabled ? _name : "" to ERC20Permit, but this does not prevent the inherited permit() function from being called and working normally. An empty domain name still produces a valid (though non-descriptive) domain separator. This breaks the advertised feature toggle: users who deploy with _permitEnabled=false will still have permit functionality available.
Additionally, _permitEnabled is read once during construction and never stored, making it impossible to query whether permit was intended to be enabled.
Suggested fix
contract FactoryTokenV2 is ERC20, ERC20Permit, ERC20Capped, Ownable, Pausable {
error FactoryTokenV2__CapExceeded(uint256 cap, uint256 requested);
+ error FactoryTokenV2__PermitDisabled();
uint8 private immutable _decimals;
bool private immutable _pauseEnabled;
+ bool private immutable _permitEnabledFlag;
@@
{
_decimals = _decimals_;
_pauseEnabled = _pauseEnabled;
+ _permitEnabledFlag = _permitEnabled;
@@
}
+
+ function permit(
+ address owner,
+ address spender,
+ uint256 value,
+ uint256 deadline,
+ uint8 v,
+ bytes32 r,
+ bytes32 s
+ ) public override {
+ if (!_permitEnabledFlag) revert FactoryTokenV2__PermitDisabled();
+ super.permit(owner, spender, value, deadline, v, r, s);
+ }🤖 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/FactoryTokenV2.sol` around lines 40 - 41, Store the _permitEnabled
flag in the contract state (e.g., a public bool permitEnabled set in the
constructor) and explicitly disable EIP‑2612 by overriding the inherited
permit(...) function to revert (or require(permitEnabled)) when permitEnabled is
false; keep calling ERC20Permit(...) in the constructor for domain setup if you
want but rely on the stored permitEnabled flag and the overridden permit method
to actually prevent use when disabled, and expose the flag via a public getter
so callers can query whether permit was intended to be enabled.
| /// @notice Deploy with permit disabled. | ||
| function test_deployPermitDisabled() public { | ||
| vm.prank(deployer); | ||
| address token = factory.deployToken( | ||
| "NoPermit", "NOPM", DEFAULT_DECIMALS, 1_000 ether, 0, | ||
| false, DEFAULT_PAUSE | ||
| ); | ||
| FactoryTokenV2 t = FactoryTokenV2(token); | ||
| assertEq(t.name(), "NoPermit"); | ||
| assertEq(t.symbol(), "NOPM"); | ||
| // DOMAIN_SEPARATOR exists even when permit disabled | ||
| // (ERC20Permit is always inherited, just not actively used) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add a negative permit test for permitEnabled = false.
This test currently proves deployment only; it should also assert that permit() is rejected when the feature is disabled, so the toggle is behaviorally enforced.
🤖 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/TokenFactoryV2.t.sol` around lines 362 - 374, Extend
test_deployPermitDisabled to assert that permit() is rejected when permitEnabled
is false: after deploying via factory.deployToken and casting to FactoryTokenV2
(t), construct a permit call using t.permit(...) or the helper that signs and
call it via vm.prank(signer) and expectRevert (or vm.expectRevert) before
calling t.permit to ensure the transaction reverts when permitEnabled is false;
reference test_deployPermitDisabled, factory.deployToken, FactoryTokenV2, and
the permit() method so the negative assertion checks the feature toggle
behaviorally.
|
Thanks for the PR. I reviewed this locally and found a couple of blockers before this can be merged. Test result: Blockers:
_pauseEnabled = _pauseEnabled;This self-assigns the parameter, so the contract-level
Also, this PR appears to include the ERC-4626 vault changes from PR #36. Please rebase/split this after #36 is resolved so the TokenFactoryV2 diff stays focused. Suggested next steps:
Holding off on merge until these are resolved. |
2 participants
Summary
Implements TokenFactoryV2 as described in #30 — a companion factory (next to v1) that deploys extended ERC-20 tokens with three commonly-requested production features.
What's included
FactoryTokenV2.sol
Follows the same coding style as the existing codebase: BUSL-1.1, Solidity 0.8.24, OpenZeppelin v5.3.0 (already in deps).
TokenFactoryV2.sol
ITokenFactoryV2.sol
Tests
Out of scope (per issue)
Verification
forge test --match-contract TokenFactoryV2Test
Suite result: ok. 15 passed; 0 failed; 0 skipped
Closes #30
Summary by CodeRabbit
New Features
Tests