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
📊 Forge Coverage ReportGenerated by workflow run #611 |
![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
…llow any user to claim on behalf of the request user - Updated handling of canceling of finalized requests
- Updated user reference in cancel withdrawal request handling to use request.user instead of msg.sender - Simplified request validation by using logical OR in the RequestNotFound check - Enhanced post-condition verification to include the user who cancelled the request
- Introduced minAmountOut parameter to requestWithdraw and requestWithdrawWithPermit functions for slippage protection. - Updated WithdrawRequest struct to include minAmountOut for better tracking of withdrawal conditions. - Enhanced error handling to revert transactions if the output amount is insufficient. - Modified tests to cover new functionality and ensure proper behavior with minAmountOut.
… functions from LiquidityPool to PriorityWithdrawalQueue
- Modified getInstantLiquidityAmount function to account for ethAmountLockedForPriorityWithdrawal. - Refactored PriorityWithdrawalQueue to rename ethAmountLockedForWithdrawal to ethAmountLockedForPriorityWithdrawal, ensuring consistency across contracts. - Updated tests to reflect changes in the PriorityWithdrawalQueue implementation and its interactions.
…or when permit fails due to low allowance. - Added new tests to validate withdrawal requests using permits, including scenarios for invalid permits, expired deadlines, and replay attacks.
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.
pankajjagtapp
force-pushed
the
pankaj/feat/priority-withdrawal-queue
branch
from
8af79fd to
80b99a3
Compare
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.
| } | ||
|
|
||
| uint256 totalAmountToLock = liquidityPool.amountForShare(totalSharesToFinalize); | ||
| ethAmountLockedForPriorityWithdrawal += uint128(totalAmountToLock); |
There was a problem hiding this comment.
Locked amount accounting mismatch causes phantom locks
Medium Severity
In fulfillRequests, ethAmountLockedForPriorityWithdrawal is incremented using liquidityPool.amountForShare(totalSharesToFinalize) without capping at the original request amounts. However, in _claimWithdraw, the unlock amount is capped at request.amountOfEEth. When share price increases between request and fulfill (expected for a staking token), more ETH is locked than will ever be unlocked. Over time, this causes ethAmountLockedForPriorityWithdrawal to accumulate phantom locked amounts, potentially causing getInstantLiquidityAmount() in EtherFiRedemptionManager to underflow and block legitimate redemptions.
Additional Locations (1)
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.
| 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 null check causes revert when priorityWithdrawalQueue is zero
High Severity
The getInstantLiquidityAmount function calls priorityWithdrawalQueue.ethAmountLockedForPriorityWithdrawal() without checking if priorityWithdrawalQueue is address(0). The reaudit-fixes script deploys EtherFiRedemptionManager with address(0x0) for the priority queue parameter. When getInstantLiquidityAmount(ETH_ADDRESS) is called on such a deployment, it will revert because calling a function on address(0) fails. This breaks totalRedeemableAmount, canRedeem, and redemption functionality.
Additional Locations (1)
4 participants
Note
High Risk
High risk because it changes core withdrawal/redemption liquidity accounting and adds a new privileged caller path into
LiquidityPool.withdraw, impacting funds flow and upgrade/deploy procedures.Overview
Adds a new upgradeable
PriorityWithdrawalQueue(plusIPriorityWithdrawalQueue) to support whitelisted, delayed withdrawal requests that are finalized by a request manager, lock liquidity, and later claim ETH fromLiquidityPool.Integrates the queue into core contracts:
LiquidityPoolnow takes an immutablepriorityWithdrawalQueuein its constructor and allows it to callwithdraw/burnEEthShares, whileEtherFiRedemptionManageradds a constructor param for the queue and subtractsethAmountLockedForPriorityWithdrawal()from instant ETH liquidity.Updates deployment/upgrade automation with new scripts for CREATE2 deployment and timelock batch upgrades/role grants, adjusts bytecode-verification scripts for the new constructors, extends
Utilsconstructor-arg decoding foruint32/uint64/uint128, and adds extensive tests plus fixture changes to use the new constructors.Written by Cursor Bugbot for commit 79386cd. This will update automatically on new commits. Configure here.