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
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)
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.
| 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.
Missing address(0) guard causes ETH redemption revert
High Severity
getInstantLiquidityAmount unconditionally calls priorityWithdrawalQueue.ethAmountLockedForPriorityWithdrawal() without checking if priorityWithdrawalQueue is address(0). The LiquidityPool.withdraw properly guards with if (priorityWithdrawalQueue != address(0)), but the redemption manager lacks this guard. If the redemption manager is ever deployed with a zero-address priority queue (as done in the reaudit-fixes script), all ETH redemptions will revert.
| 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.
Unchecked subtraction may panic blocking withdrawals
Medium Severity
Two new subtraction operations in withdraw can underflow with a panic instead of a clean InsufficientLiquidity revert. totalValueInLp - ethAmountLockedForWithdrawal (priority queue path) and totalValueInLp - priorityLocked (NFT path) panic if the locked amounts exceed totalValueInLp, which is possible after a negative rebase or slashing event. This would block all priority queue or NFT withdrawals respectively.
Additional Locations (1)
5 participants
Note
High Risk
High risk because it introduces a new withdrawal path and changes core
LiquidityPool.withdrawauthorization/liquidity checks plusEtherFiRedemptionManagerliquidity accounting, which directly impacts redemption/withdrawal correctness and fund safety.Overview
Introduces a new upgradeable
PriorityWithdrawalQueuecontract for whitelisted “priority” withdrawals (request, fulfill/finalize, claim/cancel flows, remainder handling, pausing, and role-gated administration), plus theIPriorityWithdrawalQueueinterface.Integrates the queue into core contracts:
LiquidityPoolnow has an immutablepriorityWithdrawalQueue, allows it to callwithdraw/burnEEthShares, and adds cross-reserve liquidity checks so NFT withdrawals and priority withdrawals respect each other’s locked ETH;EtherFiRedemptionManageradds an immutable queue reference and subtractsethAmountLockedForPriorityWithdrawal()when calculating instant ETH liquidity.Adds mainnet ops artifacts and scripts to deploy the queue + new implementations via Create2, generate timelock upgrade/role-grant transactions, and set operation-parameter tags; updates
Utils.deployto be idempotent and extends constructor-arg JSON formatting foruint32/uint64/uint128. Updates tests and fork scripts to pass the new constructors and adds a largePriorityWithdrawalQueue.t.solsuite covering end-to-end flows and reserve-isolation.Written by Cursor Bugbot for commit a169db3. This will update automatically on new commits. Configure here.