Skip to content

Commit

Permalink
additional safeguards and tests
Browse files Browse the repository at this point in the history
  • Loading branch information
@jtfirek
jtfirek committed Oct 9, 2024
1 parent 9d220ab commit 71f70ae
Show file tree
Hide file tree
Showing 3 changed files with 47 additions and 16 deletions.
1 change: 1 addition & 0 deletions src/LiquidityPool.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -423,6 +423,7 @@ contract LiquidityPool is Initializable, OwnableUpgradeable, UUPSUpgradeable, IL

emit Rebase(getTotalPooledEther(), eETH.totalShares());
}

/// @notice pay protocol fees including 5% to treaury, 5% to node operator and ethfund bnft holders
/// @param _protocolFees The amount of protocol fees to pay in ether
function payProtocolFees(uint128 _protocolFees) external {
Expand Down
42 changes: 28 additions & 14 deletions src/WithdrawRequestNFT.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,6 @@ import "./interfaces/IWithdrawRequestNFT.sol";
import "./interfaces/IMembershipManager.sol";
import "./RoleRegistry.sol";

import "forge-std/console.sol";

contract WithdrawRequestNFT is ERC721Upgradeable, UUPSUpgradeable, OwnableUpgradeable, IWithdrawRequestNFT {
//--------------------------------------------------------------------------------------
//--------------------------------- STATE-VARIABLES ----------------------------------
Expand Down Expand Up @@ -113,7 +111,7 @@ contract WithdrawRequestNFT is ERC721Upgradeable, UUPSUpgradeable, OwnableUpgrad

/// @notice called by the NFT owner of a finalized request to claim their ETH
/// @param requestId the id of the withdraw request and associated NFT
/// @param checkpointIndex the index of the `finalizationCheckpoints` that the request belongs to.
/// @param checkpointIndex the index of the `finalizationCheckpoints` that the request belongs to
/// can be found with `findCheckpointIndex(_requestIds, 1, getLastCheckpointIndex())`
function claimWithdraw(uint32 requestId, uint32 checkpointIndex) external {
return _claimWithdraw(requestId, ownerOf(requestId), checkpointIndex);
Expand Down Expand Up @@ -153,17 +151,28 @@ contract WithdrawRequestNFT is ERC721Upgradeable, UUPSUpgradeable, OwnableUpgrad

/// @notice finalizes a batch of requests and locks the corresponding ETH to be withdrawn
/// @dev called by the `EtherFiAdmin` contract to finalize a batch of requests based on the last oracle report
/// @param lastRequestId the id of the last request to finalize in this batch, will update `lastFinalizedRequestId` value
function finalizeRequests(uint32 lastRequestId) external {
if (!roleRegistry.hasRole(WITHDRAW_NFT_ADMIN_ROLE, msg.sender)) revert IncorrectRole();
require(lastRequestId >= lastFinalizedRequestId, "Invalid lastRequestId submitted");

// No new requests have been finalized since the last oracle report
if (lastRequestId == lastFinalizedRequestId) { return; }

uint256 totalAmount = uint256(calculateTotalPendingAmount(lastRequestId));
_finalizeRequests(lastRequestId, totalAmount);
}

/// @notice `finalizeRequests` with the ability to specify the total amount of ETH to be locked
/// @dev The oracle calculates the amount of ETH that is needed to fulfill the pending withdrawal off-chain
/// @param lastRequestId the id of the last request to finalize in this batch, will update `lastFinalizedRequestId` value
/// @param totalAmount the total amount of ETH to be locked for the requests in this batch
function finalizeRequests(uint256 lastRequestId, uint256 totalAmount) external {
if (!roleRegistry.hasRole(WITHDRAW_NFT_ADMIN_ROLE, msg.sender)) revert IncorrectRole();
require(lastRequestId >= lastFinalizedRequestId, "Invalid lastRequestId submitted");

// No new requests have been finalized since the last oracle report
if (lastRequestId == lastFinalizedRequestId) { return; }

_finalizeRequests(lastRequestId, totalAmount);
}
Expand Down Expand Up @@ -199,7 +208,7 @@ contract WithdrawRequestNFT is ERC721Upgradeable, UUPSUpgradeable, OwnableUpgrad
/// `checkpointIndex` can be found using `findCheckpointIndex()` function
/// @return uint256 the amount of ETH that can be claimed by the owner of the NFT
function getClaimableAmount(uint32 requestId, uint32 checkpointIndex) public view returns (uint256) {
require(isFinalized(tokenId), "Request is not finalized");
require(isFinalized(requestId), "Request is not finalized");
require(requestId < nextRequestId, "Request does not exist");
require(ownerOf(requestId) != address(0), "Already claimed");

Expand Down Expand Up @@ -235,7 +244,7 @@ contract WithdrawRequestNFT is ERC721Upgradeable, UUPSUpgradeable, OwnableUpgrad
function findCheckpointIndex(uint32 _requestId, uint32 _start, uint32 _end) public view returns (uint32) {
require(_requestId <= lastFinalizedRequestId, "Request is not finalized");
require(_start <= _end, "Invalid range");
require (_start != 0 && _end <= getLastCheckpointIndex(), "Range is out of bounds");
require(_start != 0 && _end <= getLastCheckpointIndex(), "Range is out of bounds");

// Left boundary
if (_requestId <= finalizationCheckpoints[_start].lastFinalizedRequestId) {
Expand Down Expand Up @@ -269,9 +278,9 @@ contract WithdrawRequestNFT is ERC721Upgradeable, UUPSUpgradeable, OwnableUpgrad
return uint32(max);
}

/// @notice The excess eETH balance of this contract beyond what is needed to fulfill withdrawal requests.
/// @notice The excess eETH balance of this contract beyond what is needed to fulfill withdrawal requests
/// This excess accumulates due to:
/// - eETH requested for withdrawal accruing staking rewards until the withdrawal is finalized.
/// - eETH requested for withdrawal accruing staking rewards until the withdrawal is finalized
/// Any remaining positive rebase rewards stay in the contract after finalization
/// - eETH balance calculation includes integer division, and there is a common case when the whole eETH
/// balance can't be transferred from the account while leaving the last 1-2 wei on the sender's account
Expand All @@ -286,10 +295,14 @@ contract WithdrawRequestNFT is ERC721Upgradeable, UUPSUpgradeable, OwnableUpgrad
/// @notice The amount of eETH that needed to fulfill the pending withdrawal requests up to and including `lastRequestId`
function calculateTotalPendingAmount(uint32 lastRequestId) public view returns (uint256) {
uint256 totalAmount = 0;
uint256 preciseSharePrice = liquidityPool.amountForShare(E27_PRECISION_BASE);

for (uint32 i = lastFinalizedRequestId + 1; i <= lastRequestId; i++) {

IWithdrawRequestNFT.WithdrawRequest memory request = _requests[i];
uint256 amountForShares = liquidityPool.amountForShare(request.shareOfEEth);

// Use the precise share price calculation to maintain conistency with how amount is calculated in `getClaimableAmount`
uint256 amountForShares = request.shareOfEEth * preciseSharePrice / E27_PRECISION_BASE;
uint256 amount = _min(request.amountOfEEth, amountForShares);

totalAmount += amount;
Expand Down Expand Up @@ -350,18 +363,19 @@ contract WithdrawRequestNFT is ERC721Upgradeable, UUPSUpgradeable, OwnableUpgrad

lastFinalizedRequestId = uint32(lastRequestId);

if (totalAmount > 0) {
liquidityPool.withdraw(address(this), totalAmount);
}
liquidityPool.withdraw(address(this), totalAmount);

emit UpdateFinalizedRequestId(uint32(lastRequestId), totalAmount);
}

// invalid NFTs is non-transferable except for the case they are being burnt by the owner via `seizeInvalidRequest`
function _beforeTokenTransfer(address /*from*/, address /*to*/, uint256 firstTokenId, uint256 batchSize) internal view override {
for (uint256 i = 0; i < batchSize; i++) {
uint256 tokenId = firstTokenId + i;
require(_requests[tokenId].isValid || msg.sender == owner(), "INVALID_REQUEST");
if (msg.sender != owner()) {
// if not called by the contract owner, only allow transfers of valid NFTs
for (uint256 i = 0; i < batchSize; i++) {
uint256 tokenId = firstTokenId + i;
require(_requests[tokenId].isValid, "INVALID_REQUEST");
}
}
}

Expand Down
20 changes: 18 additions & 2 deletions test/WithdrawRequestNFT.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -311,13 +311,15 @@ contract WithdrawRequestNFTTest is TestSetup {
assertEq(eETHInstance.balanceOf(address(withdrawRequestNFTInstance)), 8);
// Within `LP.requestWithdraw`
// - `share` is calculated by `sharesForAmount` as (9 * 98) / 100 = 8.82 ---> (rounded down to) 8


vm.prank(address(membershipManagerInstance));
liquidityPoolInstance.rebase(2);

_finalizeWithdrawalRequest(requestId);


vm.prank(bob);
withdrawRequestNFTInstance.claimWithdraw(requestId, 1);

// Within `claimWithdraw`,
// - `request.amountOfEEth` is 9
// - `amountForShares` is (8 * 100) / 98 = 8.16 ---> (rounded down to) 8
Expand Down Expand Up @@ -452,8 +454,22 @@ contract WithdrawRequestNFTTest is TestSetup {
liquidityPoolInstance.rebase(50 ether);

// finalize the requests in multiple batches

_finalizeWithdrawalRequest(5);

uint256 dustShares1 = withdrawRequestNFTInstance.getAccumulatedDustEEthAmount();

// no new NFTs where finalized during this period
_finalizeWithdrawalRequest(5);
// dust should remain the same amount
assertEq(dustShares1, withdrawRequestNFTInstance.getAccumulatedDustEEthAmount());

vm.expectRevert("Invalid lastRequestId submitted");
_finalizeWithdrawalRequest(4);

_finalizeWithdrawalRequest(11);
_finalizeWithdrawalRequest(12);
_finalizeWithdrawalRequest(13);
_finalizeWithdrawalRequest(17);
_finalizeWithdrawalRequest(23);
_finalizeWithdrawalRequest(withdrawRequestNFTInstance.nextRequestId() - 1);
Expand Down

0 comments on commit 71f70ae

Please sign in to comment.