Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Certora][H-02] NFT Withdrawal Price Finalization #158

Open
wants to merge 7 commits into
base: staging-2.5
Choose a base branch
from

Conversation

jtfirek
Copy link
Contributor

@jtfirek jtfirek commented Sep 4, 2024

Motivation

This pr addresses the concerns with the withdrawal flow raised by the certora team during the audit:

  • The withdrawal requests claim process is unfair as users are exposed to the ramifications of a negative rebase after their request is finalized
  • eETH requested for withdrawal still accrue staking rewards after requests are made. These positive rebase rewards remain in this contract after users claim their withdrawal, but there is currently no accounting or method to withdraw these funds as revenue

Features

  • A new mechanism to finalize the value of request when its corresponding batch of withdrawals is processed by the oracle. Note that users are still exposed to negative rebases that occur before finalization
  • Logic to query and claim the excess staking rewards mentioned above
  • Removal of unnecessary accounting like EthAmountLockedForWithdrawal in the new flow
  • Refactor the codebase to consistently use uint32 for withdrawal NFT id values

Migration

  • To make the existing unclaimed finalized withdrawals work with the new system, lastFinalizedRequestId will be reset to 0 and the oracle will re-finalize all withdrawals with the new logic. It will be much to expensive to call calculateTotalPendingAmount on 30,000ish withdrawals. We will have to calculate the amount needed to re-finalized all the withdrawals off-chain and pass it on-chain with finalizeRequests(uint256 lastRequestId, uint256 totalAmount). The ability to withdrawal will be temporary paused to maintain correct accounting

@jtfirek jtfirek marked this pull request as draft September 4, 2024 01:22
@jtfirek jtfirek changed the title WIP: NFT Withdraw Price Finalization WIP: NFT Withdrawal Price Finalization Sep 4, 2024
@jtfirek jtfirek marked this pull request as ready for review September 5, 2024 02:39
@jtfirek
Copy link
Contributor Author

jtfirek commented Sep 5, 2024

fyi, The failing unit tests on the last run are from Eigenpod changes from the PEPE upgrade

@jtfirek jtfirek changed the title WIP: NFT Withdrawal Price Finalization NFT Withdrawal Price Finalization Sep 5, 2024
/// Stores the cached share price at the time of finalization for each batch
/// For a checkpoint index i, this batch includes requests:
/// `finalizationCheckpoints[i-1].lastFinalizedRequestId < requestId <= finalizationCheckpoints[i].lastFinalizedRequestId`
FinalizationCheckpoint[] public finalizationCheckpoints;
Copy link
Contributor

@vvalecha519 vvalecha519 Sep 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can get rid of binary search and just use a map, m, of lastFinalizedRequestId -> finalizedPrice
and add lastFinalizedRequestId in WithdrawRequest. This might simplify everything. Let me know what you think and then I will re-review.

Ie. if someone wants to claim we find their WithdrawRequest get the lastFinalizedRequestId and then find the price in the map m to find the finalizedPrice.

Copy link
Contributor Author

@jtfirek jtfirek Sep 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is a situation that breaks this approach:

  • Let's say we have request A and request B created during the same oracle report period; they will be created with the same lastFinalizedRequestId value
  • Later request A is finalized as the last request in a batch and request B isn't finalized till the next batch
  • Both request A and B will incorrectly map to the same finalizedPrice

@jtfirek jtfirek changed the title NFT Withdrawal Price Finalization [H-02] NFT Withdrawal Price Finalization Sep 19, 2024
@seongyun-ko seongyun-ko changed the title [H-02] NFT Withdrawal Price Finalization [Certora][H-02] NFT Withdrawal Price Finalization Sep 24, 2024
@jtfirek jtfirek requested a review from vvalecha519 October 8, 2024 21:06
@jtfirek jtfirek force-pushed the jtdev/feat/withdraw-price-finalization branch from f979658 to 71f70ae Compare October 9, 2024 21:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants