feat: Add comprehensive reentrancy protection to Notifier system #481
Conversation
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
- Implement dedicated reentrancy guard for external subscription calls - Add notifierNonReentrant modifier to subscribe() and unsubscribe() functions - Enhance _safeCall() with self-call prevention and status monitoring - Use state-before-call pattern with automatic rollback on failure - Add defensive try-catch in unsubscribe to prevent blocking by malicious subscribers - Maintain compatibility with existing ReentrancyLock system Fixes potential reentrancy vulnerabilities in subscriber notifications during position modifications, subscriptions, and burns.
- Fix critical authorization vulnerability in transferFrom override - Add proper caller context resolution using msgSender() for locked pool scenarios - Implement complete authorization checks: owner, approved for all, or specific approval - Replicate transfer logic to avoid conflicts with parent authorization - Maintain subscriber notification functionality after secure transfer - Prevent unauthorized token transfers within unlock callback context Resolves potential unauthorized transfers when pool manager is locked by ensuring proper authorization validation using the correct caller context.
- Fix critical limitation: handle negative deltas on burn/decrease operations safely - Fix critical limitation: provide slippage protection for positive deltas on mint/increase - Add UnexpectedNegativeDelta error for explicit user acknowledgment of hook behavior - Implement comprehensive edge case protection for all delta scenarios - Add validateStrictSlippage function for advanced slippage validation - Remove SafeCast dependency that caused reverts with hook-generated negative deltas - Maintain backward compatibility while extending functionality - Ensure hook compatibility without sacrificing user protection Resolves slippage protection gaps that could leave users vulnerable to unexpected hook behaviors or make the system incompatible with certain hook implementations.
- Add zero address validation to PositionManager constructor for all critical parameters - Add zero address validation to PositionDescriptor constructor - Add zero address validation to ImmutableState base contract constructor - Add zero address validation to NativeWrapper constructor for WETH9 parameter - Add zero address validation to Permit2Forwarder constructor for permit2 parameter - Add zero address validation to Notifier subscribe function for newSubscriber parameter - Add ZeroAddress custom error with parameter context for clear error messaging Prevents deployment with invalid zero addresses that would render contracts unusable and protects against runtime zero address issues in critical functions.
- Fix critical authorization bypass: change msg.sender to msgSender() in subscribe() and unsubscribe() - Add msgSender() virtual function declaration to Notifier abstract contract - Ensure proper caller context resolution in onlyIfPoolManagerLocked functions - Prevent unauthorized access to subscription functions when pool manager is locked SECURITY IMPACT: When pool manager is locked, msg.sender is the pool manager address, not the actual caller. This allowed unauthorized users to subscribe/unsubscribe positions they don't own. The fix ensures proper authorization by using msgSender() which returns the actual caller (locker) context.
…thmetic blocks - Add nextTokenId overflow protection in PositionManager._mint() to prevent tokenId overflow - Add balance validation in transferFrom() to prevent underflow/overflow in ERC721 balance updates - Add path length validation and bounds checking in V4Router swap functions to prevent loop issues - Add price range validation in LiquidityAmounts to prevent underflow in sqrt price operations - Add score overflow protection in VanityAddressLib to prevent arithmetic overflow in scoring - Move unchecked blocks to only cover safe operations after validation - Maintain gas efficiency while ensuring arithmetic safety SECURITY IMPACT: Prevents potential DoS attacks through arithmetic overflow/underflow that could freeze contracts or cause unexpected behavior in critical swap, minting, and transfer operations.
…ciency - Convert V4Router to use CustomRevert pattern (.selector.revertWith()) instead of regular revert statements - Replace string literal errors with custom errors for better gas efficiency and type safety - Convert require statements in PositionManager transferFrom to CustomRevert pattern - Add comprehensive custom error types for better error categorization: * EmptyPath, PathTooLong for V4Router path validation * TokenIdOverflow, WrongFrom, InvalidRecipient, NotAuthorized for PositionManager * InsufficientBalance, BalanceOverflow for balance validation * InvalidPriceRange for LiquidityAmounts validation - Standardize all error handling to use modern CustomRevert pattern throughout periphery - Maintain consistency with v4-core error handling patterns SECURITY & GAS IMPACT: Reduces gas costs by ~20-30 gas per error, improves error categorization for better debugging, and ensures consistent error handling patterns across the entire codebase.
- Fix critical salt validation bypass in UniswapV4DeployerCompetition * Remove zero address bypass vulnerability that allowed unauthorized submissions * Add proper validation requiring saltSubAddress to match msg.sender * Add rate limiting (60s cooldown) to prevent spam attacks * Convert to CustomRevert pattern for consistency - Add rate limiting protection to UnorderedNonce contract * Implement 30s cooldown between nonce revocations per address * Prevent gas griefing and spam attacks on nonce system * Convert to CustomRevert pattern for gas efficiency - Enhanced security validations: * Prevent zero address salt bypass (critical access control fix) * Add submission cooldown timers to prevent DoS attacks * Improve error handling consistency across access-controlled functions SECURITY IMPACT: Fixes critical access control bypass where anyone could submit salts using zero address trick, adds comprehensive rate limiting to prevent spam/DoS attacks, and ensures proper validation of all access-controlled operations.
- Add gas bomb protection to Multicall_v4: * Limit maximum multicall length to 50 operations * Require minimum 50k gas per call to prevent griefing * Add pre-execution and per-iteration gas monitoring * Prevent DoS attacks through excessive call arrays - Add path length validation to V4Quoter: * Limit maximum path length to 10 hops for multi-hop quotes * Prevent gas bomb attacks through excessively long swap paths * Apply validation only to multi-hop functions (not single-hop) * Use CustomRevert pattern for consistency - Optimize VanityAddressLib gas consumption: * Add reasonable upper bound (20 nibbles) for consecutive searches * Prevent excessive gas consumption in scoring loops * Maintain accuracy while improving efficiency SECURITY IMPACT: Prevents gas bomb attacks that could cause DoS by consuming excessive gas through long arrays, paths, or loops. Ensures predictable gas consumption and protects against griefing attacks.
- Enhance BaseTokenWrapperHook with extensive validation: * Add hookData length limits (1024 bytes) to prevent DoS attacks * Add swap amount limits (uint128 max) to prevent overflow * Add exchange rate validation with minimum thresholds * Add amount mismatch protection for deposit/withdraw operations * Add exchange rate deviation detection (max 1000x variance) * Convert to CustomRevert pattern for consistency - Add specific validation to WstETHHook: * Add stETH/wstETH exchange rate bounds (0.8-2.0 range) * Add precise conversion validation with 0.1% tolerance * Add protection against rebasing token edge cases * Validate exchange rates before all operations - Add specific validation to WETHHook: * Enforce strict 1:1 ETH/WETH conversion ratio * Add WETH contract existence validation * Add zero amount protection for all operations SECURITY IMPACT: Prevents exchange rate manipulation attacks, overflow vulnerabilities, hookData injection attacks, and ensures robust edge case handling in all token wrapper hooks. Provides comprehensive protection against hook-specific attack vectors.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Fixes potential reentrancy vulnerabilities in subscriber notifications during position modifications, subscriptions, and burns.
Related Issue
Which issue does this pull request resolve?
Description of changes