yash/fix/nethermind-audit#419
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| // valdate finalized request id | ||
| uint32 lastFinalizedRequestId = withdrawRequestNft.lastFinalizedRequestId(); | ||
| if (_report.lastFinalizedWithdrawalRequestId < lastFinalizedRequestId) return (false, "EtherFiAdmin: finalized withdrawal request id is less than last finalized request id"); | ||
| if (_report.lastFinalizedWithdrawalRequestId - lastFinalizedRequestId > maxNumberOfRequestsToFinalizePerReport) return (false, "EtherFiAdmin: number of requests to finalize exceeds max"); |
There was a problem hiding this comment.
what is this for? is this useful?
There was a problem hiding this comment.
to prevent a griefing scenario. in oracle sanity checks, added a loop to confirm amount in each request sum equals total amount, need to ensure each reprot doesnt have too many requests else can be gas griefed. practical value could something like 2-5k
There was a problem hiding this comment.
why would the attack work? do we have O(N) component in withdrawal processing? are we scanning all requests?
There was a problem hiding this comment.
at time of finalization we loop over all requests to finalize (to sanity check individual balances sum equal total withdraw amount to finalize), that adds O(N) complexity so need this check to prevent griefing.
|
|
||
| // liquidity pool supplies 1 eth per validator | ||
| uint256 outboundEthAmountFromLp = 1 ether * _bidIds.length; | ||
| uint256 outboundEthAmountFromLp = LP_INITIAL_VALIDATOR_DEPOSIT * _bidIds.length; |
There was a problem hiding this comment.
variable naming, INITIAL_VALIDATOR_DEPOSIT; remove LP_
|
|
||
| // we have already deposited the initial amount to create the validator on the beacon chain | ||
| uint256 remainingEthPerValidator = validatorSizeWei - stakingManager.initialDepositAmount(); | ||
| uint256 remainingEthPerValidator = validatorSizeWei - stakingManager.INITIAL_DEPOSIT_AMOUNT(); |
There was a problem hiding this comment.
Do both LP and StakingMgr contracts have their own INITIAL DEPOSIT AMOUNT variables?
why not use only one as a single source of truth
|
|
||
| uint256 amountForShares = liquidityPool.amountForShare(request.shareOfEEth); | ||
| if (amountForShares < request.amountWithFee) revert InvalidOutputAmount(); | ||
| if (amountForShares + _TOLERANCE_BUFFER < request.amountWithFee) revert InvalidOutputAmount(); |
There was a problem hiding this comment.
If there was negative rebasing, what happens here?
if negative rebasing is happening, we shuold not finalize any PWQ request? should put that to Oracle bot?
There was a problem hiding this comment.
Yes, decided with nonce team that it;s fine to not handle negative rebase case for priority queue. will emsure relevant changes in BE.
| uint32 feeGwei = uint32(fee / 1 gwei); | ||
|
|
||
| _requests[requestId] = IWithdrawRequestNFT.WithdrawRequest(amountOfEEth, shareOfEEth, true, feeGwei); | ||
| _requests[requestId] = IWithdrawRequestNFT.WithdrawRequest(amountOfEEth, shareOfEEth, true, 0); |
There was a problem hiding this comment.
why do we do this change?
i know we dont have the fee on withdrawal.
let's keep the function as it is though
There was a problem hiding this comment.
using the difference between balance and eth amount locked to get stranded ETH to treasury in case of negative rebase. earlier this was used for fee path which we're not using (and confirmed with jv we have no plans for this in future), also fee was hardcoded to 0 so changing logic would've require upgrade anyway.
I implemented a different version previously: a82e295
but discussed to use current approach instead: a82e295#r3285119385
There was a problem hiding this comment.
ok. can we leave dev note on the function signature? there is no fee and the input param will be ingored? OR we shuold just revert if non zero
There was a problem hiding this comment.
removed accepting the fee from all places in contracts, functions were callable within protocol so no external function signature changes required.
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.
Reviewed by Cursor Bugbot for commit 449fa98. Configure here.
3 participants
Note
High Risk
Touches core protocol paths (oracle report handling, withdrawal finalization/claiming, and liquidity accounting) and changes access control for asset recovery, so mistakes could impact redemption safety or governance permissions despite added guardrails/tests.
Overview
Hardens several core protocol flows around oracle reporting and withdrawals.
EtherFiAdmin.finalizeWithdrawalsWhenStalenow keys offEtherFiOracle.lastPublishedReportRefBlock(), adds a cooldown, caps the number of requests finalized per call/report, and switches liquidity checks toliquidityPool.totalValueInLp(); report validation also adds the same request-range cap and updates math to useMathhelpers.Withdrawal mechanics are adjusted for the escrow-migration model:
LiquidityPoolreplaces fixed MIN/MAX withdrawal constants with operator-setminWithdrawAmount/maxWithdrawAmount, andWithdrawRequestNFTremoves fee handling from requests/claims, tightens escrow accounting, adds a small tolerance buffer when validating outputs, gatesfulfillRequestsonescrowMigrationCompleted, and restrictsseizeInvalidRequestto the upgrade timelock.Across the codebase, ERC20 interactions are made safer via
SafeERC20(e.g.,DepositAdapter,WeETH,MembershipManager,EtherFiRestaker), multiple ETH forwarding sites increase gas stipends to a shared no-grief constant, andBucketRateLimiterswitches toceilDivwith an explicitRATE_PRECISION. Finally,CumulativeMerkleRewardsDistributorgainsreceive()plus admin-onlyrecoverETH/recoverERC20, andEETH/WeETHrecovery functions are tightened from operations to admin; migration-only initializers are removed fromAuctionManagerandNodeOperatorManager.Reviewed by Cursor Bugbot for commit 449fa98. Bugbot is set up for automated code reviews on this repo. Configure here.