Conversation
…al request handling and treasury fee distribution - Rename and adjust constants for clarity - Introduce immutable treasury address and share remainder split to treasury - Simplify withdrawal request structure by removing unnecessary parameters - Enhance request management with new roles and functions for invalidating requests - Update event emissions and error handling for better clarity and functionality
…drawalQueue and remove setter function - Change priorityWithdrawalQueue to an immutable variable initialized in the constructor - Remove the setPriorityWithdrawalQueue function to enhance security and restrict modifications - Adjust reduceEthAmountLockedForPriorityWithdrawal to allow only the priorityWithdrawalQueue to call it
…improved initialization and request handling
- Change nonce and minDelay types to uint32 for better compatibility - Adjust withdrawal request structure to use consistent data types - Update event emissions and function signatures to reflect new types - Enhance role management for whitelist operations
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
seongyun-ko
left a comment
seongyun-ko
left a comment
There was a problem hiding this comment.
Nice work! :)
-
why do we need 'withdrawCapacity'? we can just let them submit whatever they want and handle from our end?
-
let's put Reentrancy Protection to (requestWithdraw, cancelWithdraw, claimWithdraw)
-
let's put guards so that (requestWithdraw) and (cancelWithdraw, claimWithdraw) can't happen within the same block in any case. preveting any possible loop. saw 'minDelay' config.
-
as a post-hook, let's add balance check of (LiquidityPool/PriorityWithdrawalQueue), so that it reverts if unexpected balance change occurs
…ved withdrawal handling - Update burnEEthShares function to include priorityWithdrawalQueue as a valid caller - Refactor withdrawal logic to calculate and return the lesser of original amount or current share value, addressing negative rebase scenarios - Adjust event emissions to reflect updated withdrawal amounts and shares transferred
- Remove withdrawal configuration struct and related functions for clarity - Introduce constants for minimum delay and minimum withdrawal amount - Enhance request handling with new post-condition verification methods - Update tests to reflect changes in withdrawal logic and configuration
pankajjagtapp
force-pushed
the
pankaj/feat/priority-withdrawal-queue
branch
from
0b80857 to
f199f40
Compare
- Replace constant MIN_DELAY with an immutable variable initialized in the constructor - Adjust withdrawal request handling to check for MIN_DELAY in a more efficient manner - Update tests to reflect changes in constructor parameters and request validation logic
vvalecha519
left a comment
vvalecha519
left a comment
There was a problem hiding this comment.
-
U will need to make changes to EtherFiRedemptionManager (ie. getInstantLiquidityAmount needs to get eth locked by priority queue as well)
-
Maybe add easy way to query a users pending wds... pain-point with veda's on chain queue?
…dant comments and simplifying logic - Removed unnecessary comments and documentation for clarity - Streamlined canceling withdrawal request handling by directly using request parameters - Enhanced event emissions to reflect accurate withdrawal amounts and shares
…ixes Priority queue Audit fixes
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Signed-off-by: Pankaj Jagtap <jagtap.pankaj2000@gmail.com>
📊 Forge Coverage ReportGenerated by workflow run #667 |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
pankajjagtapp
force-pushed
the
pankaj/feat/priority-withdrawal-queue
branch
from
edf486a to
0ba348c
Compare
…ality in PriorityWithdrawalQueue tests
…modify treasury references
…rt-for-priority-queue Pankaj/feat/we eth support for priority queue
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
…ences in deployment scripts
…yPool, PriorityWithdrawalQueue, and UUPSProxy
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| if (priorityWithdrawalQueue != address(0)) { | ||
| uint128 priorityLocked = uint128(IPriorityWithdrawalQueue(priorityWithdrawalQueue).ethAmountLockedForPriorityWithdrawal()); | ||
| if (totalValueInLp - priorityLocked < _amount) revert InsufficientLiquidity(); | ||
| } |
There was a problem hiding this comment.
NFT withdrawal cross-reserve check is too weak
Medium Severity
The NFT withdrawal path checks totalValueInLp - priorityLocked >= _amount, but the correct invariant to maintain is totalValueInLp >= ethAmountLockedForWithdrawal + priorityLocked. When _amount is smaller than ethAmountLockedForWithdrawal (a partial claim of one NFT from many), the check is too lenient and allows withdrawals that leave the LP unable to serve remaining NFT and priority obligations combined. This breaks the cross-reserve isolation that this PR intends to enforce.
| function getInstantLiquidityAmount(address token) public view returns (uint256) { | ||
| if(token == ETH_ADDRESS) { | ||
| return address(liquidityPool).balance - liquidityPool.ethAmountLockedForWithdrawal(); | ||
| return address(liquidityPool).balance - liquidityPool.ethAmountLockedForWithdrawal() - priorityWithdrawalQueue.ethAmountLockedForPriorityWithdrawal(); |
There was a problem hiding this comment.
Potential underflow in getInstantLiquidityAmount breaks redemptions
Medium Severity
getInstantLiquidityAmount subtracts both ethAmountLockedForWithdrawal() and ethAmountLockedForPriorityWithdrawal() from the LP balance. Since these two locks are managed by independent actors (EtherFiAdmin and the priority queue request manager) with no combined cap enforcement, their sum can exceed address(liquidityPool).balance, causing an underflow revert. This would break canRedeem, totalRedeemableAmount, and redeem for ETH entirely—not gracefully returning zero but reverting.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| if (_amount > type(uint128).max || _amount == 0 || share == 0) revert InvalidAmount(); | ||
|
|
||
| totalValueInLp -= uint128(_amount); | ||
| if (msg.sender == priorityWithdrawalQueue && (totalValueInLp - ethAmountLockedForWithdrawal < _amount)) revert InsufficientLiquidity(); |
There was a problem hiding this comment.
Arithmetic underflow in LP withdraw lock checks
High Severity
The subtraction totalValueInLp - ethAmountLockedForWithdrawal (line 225) and totalValueInLp - priorityLocked (line 231) can underflow, causing a panic revert instead of the intended InsufficientLiquidity error. Since membershipManager and etherFiRedemptionManager callers pass through withdraw() without respecting either lock, they can drain totalValueInLp below the lock values. This would then DoS subsequent NFT and priority queue withdrawals with an unexpected panic revert.
Additional Locations (1)
| function getInstantLiquidityAmount(address token) public view returns (uint256) { | ||
| if(token == ETH_ADDRESS) { | ||
| return address(liquidityPool).balance - liquidityPool.ethAmountLockedForWithdrawal(); | ||
| return address(liquidityPool).balance - liquidityPool.ethAmountLockedForWithdrawal() - priorityWithdrawalQueue.ethAmountLockedForPriorityWithdrawal(); |
There was a problem hiding this comment.
Underflow in getInstantLiquidityAmount breaks instant redemptions
Medium Severity
The expression address(liquidityPool).balance - liquidityPool.ethAmountLockedForWithdrawal() - priorityWithdrawalQueue.ethAmountLockedForPrior ityWithdrawal() can underflow and revert if the LP's actual ETH balance is less than the sum of both lock amounts. Since membershipManager withdrawals reduce the LP balance without respecting either lock, this view function can become permanently reverting, blocking all instant redemptions via totalRedeemableAmount.
| emit WithdrawRequestFinalized(requestId, request.user, request.amountOfEEth, request.shareOfEEth, request.nonce, uint32(block.timestamp)); | ||
| } | ||
|
|
||
| ethAmountLockedForPriorityWithdrawal += uint128(totalAmountToLock); |
There was a problem hiding this comment.
Fulfills lock amountOfEEth but only withdraws amountWithFee
Medium Severity
fulfillRequests locks request.amountOfEEth in ethAmountLockedForPriorityWithdrawal, but _claimWithdraw only actually withdraws request.amountWithFee (which can be significantly less due to fees). The lock is then decremented by amountOfEEth on claim, so it balances out per-request, but while requests are pending, the over-locked amount unnecessarily restricts NFT queue withdrawals and instant redemptions that check this value.
Additional Locations (1)
5 participants
Note
High Risk
High risk because it introduces a new upgradeable withdrawal flow that can lock ETH and changes
LiquidityPool.withdraw/EtherFiRedemptionManagerliquidity calculations, directly impacting user withdrawals and redemption availability.Overview
Adds a new upgradeable
PriorityWithdrawalQueuecontract for whitelisted “priority” withdrawals (request/finalize/claim, permit + weETH support, pausing, role-gated whitelist and request management, and remainder-share handling).Integrates the queue into core withdrawal paths by upgrading
LiquidityPoolandEtherFiRedemptionManager: both gain apriorityWithdrawalQueueimmutable,LiquidityPool.withdraw/burnEEthSharesaccept calls from the queue and enforce separate liquidity reservations, andEtherFiRedemptionManager.getInstantLiquidityAmount(ETH)now subtracts priority-locked ETH.Adds deployment/upgrade tooling and artifacts (Create2 deploy script + timelock transaction generator, operation-parameter update script + Gnosis JSON, new mainnet address constant, deployment logs), updates create2 deploy helper to be idempotent, and adds extensive tests for the priority queue and updated
LiquidityPoolconstructor usage.Written by Cursor Bugbot for commit 3c5d76b. This will update automatically on new commits. Configure here.