From cfef9c85bf04c2cf0fb368d4d6b1f99f730c3481 Mon Sep 17 00:00:00 2001 From: Benjamin Date: Wed, 24 Sep 2025 12:31:57 +0200 Subject: [PATCH 1/4] add cancel withdrawal --- .../src/PufferWithdrawalManager.sol | 43 ++ .../interface/IPufferWithdrawalManager.sol | 29 ++ .../test/unit/PufferWithdrawalManager.t.sol | 401 ++++++++++++++++++ 3 files changed, 473 insertions(+) diff --git a/mainnet-contracts/src/PufferWithdrawalManager.sol b/mainnet-contracts/src/PufferWithdrawalManager.sol index b3a9ff96..836554a8 100644 --- a/mainnet-contracts/src/PufferWithdrawalManager.sol +++ b/mainnet-contracts/src/PufferWithdrawalManager.sol @@ -221,6 +221,48 @@ contract PufferWithdrawalManager is }); } + /** + * @inheritdoc IPufferWithdrawalManager + * @dev Allows users to cancel their withdrawal requests and receive back pufETH + */ + function cancelWithdrawal(uint256 withdrawalIdx) external { + WithdrawalManagerStorage storage $ = _getWithdrawalManagerStorage(); + + require(withdrawalIdx < $.withdrawals.length, WithdrawalDoesNotExist()); + + Withdrawal memory withdrawal = $.withdrawals[withdrawalIdx]; + + // Check if withdrawal has already been completed (recipient is set to address(0) when completed) + require(withdrawal.recipient != address(0), WithdrawalAlreadyCompleted()); + + // Check if the caller is the original recipient + require(withdrawal.recipient == msg.sender, NotWithdrawalOwner()); + + // Check if the withdrawal's batch has been finalized + uint256 batchIndex = withdrawalIdx / BATCH_SIZE; + require(batchIndex > $.finalizedWithdrawalBatch, WithdrawalAlreadyFinalized()); + + uint256 pufETHAmount = withdrawal.pufETHAmount; + address recipient = withdrawal.recipient; + + WithdrawalBatch storage batch = $.withdrawalBatches[batchIndex]; + + // Treat this canceled withdrawal as a claimed withdrawal to avoid issues in the `returnExcessETHToVault` function + ++batch.withdrawalsClaimed; + + uint256 expectedETHAmount = (pufETHAmount * withdrawal.pufETHToETHExchangeRate) / 1 ether; + batch.toBurn -= uint88(pufETHAmount); + batch.toTransfer -= uint96(expectedETHAmount); + + // Clear the withdrawal data + delete $.withdrawals[withdrawalIdx]; + + // Transfer pufETH back to the user + PUFFER_VAULT.transfer(recipient, pufETHAmount); + + emit WithdrawalCancelled({ withdrawalIdx: withdrawalIdx, pufETHAmount: pufETHAmount, recipient: recipient }); + } + /** * @inheritdoc IPufferWithdrawalManager * @dev Restricted access to ROLE_ID_OPERATIONS_MULTISIG @@ -230,6 +272,7 @@ contract PufferWithdrawalManager is uint256 totalExcessETH = 0; for (uint256 i = 0; i < batchIndices.length; ++i) { + require(batchIndices[i] <= $.finalizedWithdrawalBatch, NotFinalized()); WithdrawalBatch storage batch = $.withdrawalBatches[batchIndices[i]]; require(batch.withdrawalsClaimed == BATCH_SIZE, NotAllWithdrawalsClaimed()); diff --git a/mainnet-contracts/src/interface/IPufferWithdrawalManager.sol b/mainnet-contracts/src/interface/IPufferWithdrawalManager.sol index 57f750ac..abfeac0e 100644 --- a/mainnet-contracts/src/interface/IPufferWithdrawalManager.sol +++ b/mainnet-contracts/src/interface/IPufferWithdrawalManager.sol @@ -72,6 +72,21 @@ interface IPufferWithdrawalManager { */ error WithdrawalAmountTooHigh(); + /** + * @notice Thrown when attempting to cancel a withdrawal that has already been finalized + */ + error WithdrawalAlreadyFinalized(); + + /** + * @notice Thrown when attempting to cancel a withdrawal that doesn't exist + */ + error WithdrawalDoesNotExist(); + + /** + * @notice Thrown when attempting to cancel a withdrawal that you don't own + */ + error NotWithdrawalOwner(); + /** * @notice Emitted when a withdrawal is requested * @param withdrawalIdx The index of the requested withdrawal @@ -119,6 +134,14 @@ interface IPufferWithdrawalManager { */ event ExcessETHReturned(uint256[] batchIndices, uint256 totalExcessETH); + /** + * @notice Emitted when a withdrawal is cancelled + * @param withdrawalIdx The index of the cancelled withdrawal + * @param pufETHAmount The amount of pufETH returned to the user + * @param recipient The address that received the returned pufETH + */ + event WithdrawalCancelled(uint256 indexed withdrawalIdx, uint256 pufETHAmount, address indexed recipient); + /** * @notice Returns the address of the PufferVaultV5 contract * @return The address of the PufferVaultV5 contract @@ -154,6 +177,12 @@ interface IPufferWithdrawalManager { */ function completeQueuedWithdrawal(uint256 withdrawalIdx) external; + /** + * @notice Cancel a withdrawal request and receive back the pufETH + * @param withdrawalIdx The index of the withdrawal to cancel + */ + function cancelWithdrawal(uint256 withdrawalIdx) external; + /** * @notice Returns the excess ETH transferred from the Vault to the WithdrawalManager * This can happen if there is a discrepancy between the expected ETH amount and the actual ETH amount withdrawn because of the pufETH:ETH exchange rate. diff --git a/mainnet-contracts/test/unit/PufferWithdrawalManager.t.sol b/mainnet-contracts/test/unit/PufferWithdrawalManager.t.sol index c843ea86..e422a38a 100644 --- a/mainnet-contracts/test/unit/PufferWithdrawalManager.t.sol +++ b/mainnet-contracts/test/unit/PufferWithdrawalManager.t.sol @@ -783,6 +783,173 @@ contract PufferWithdrawalManagerTest is UnitTestHelper { assertEq(address(withdrawalManager).balance, 0, "WithdrawalManager should have 0 ETH"); } + function test_cancelWithdrawal() public withUnlimitedWithdrawalLimit { + uint256 depositAmount = 1 ether; + _givePufETH(depositAmount, alice); + + vm.startPrank(alice); + pufferVault.approve(address(withdrawalManager), depositAmount); + withdrawalManager.requestWithdrawal(uint128(depositAmount), alice); + vm.stopPrank(); + + uint256 withdrawalIdx = batchSize; // First withdrawal after the initial empty batch + uint256 aliceBalanceBefore = pufferVault.balanceOf(alice); + + // Cancel the withdrawal + vm.prank(alice); + vm.expectEmit(true, true, true, true); + emit IPufferWithdrawalManager.WithdrawalCancelled(withdrawalIdx, depositAmount, alice); + withdrawalManager.cancelWithdrawal(withdrawalIdx); + + // Check that pufETH was returned to alice + assertEq(pufferVault.balanceOf(alice), aliceBalanceBefore + depositAmount, "Alice should receive pufETH back"); + + // Check that withdrawal data was cleared + PufferWithdrawalManagerStorage.Withdrawal memory withdrawal = withdrawalManager.getWithdrawal(withdrawalIdx); + assertEq(withdrawal.recipient, address(0), "Withdrawal recipient should be cleared"); + assertEq(withdrawal.pufETHAmount, 0, "Withdrawal amount should be cleared"); + } + + function test_cancelWithdrawal_doesNotExist() public { + vm.prank(alice); + vm.expectRevert(IPufferWithdrawalManager.WithdrawalDoesNotExist.selector); + withdrawalManager.cancelWithdrawal(999); + } + + function test_cancelWithdrawal_alreadyCompleted() public withUnlimitedWithdrawalLimit { + uint256 depositAmount = 1 ether; + + // Fill the batch completely before finalizing + for (uint256 i = 0; i < batchSize; i++) { + address actor = actors[i % actors.length]; + _givePufETH(depositAmount, actor); + + vm.startPrank(actor); + pufferVault.approve(address(withdrawalManager), depositAmount); + withdrawalManager.requestWithdrawal(uint128(depositAmount), actor); + vm.stopPrank(); + } + + uint256 withdrawalIdx = batchSize; + + // Finalize and complete the withdrawal first + vm.prank(PAYMASTER); + withdrawalManager.finalizeWithdrawals(1); + + vm.prank(alice); + withdrawalManager.completeQueuedWithdrawal(withdrawalIdx); + + // Try to cancel the already completed withdrawal + vm.prank(alice); + vm.expectRevert(IPufferWithdrawalManager.WithdrawalAlreadyCompleted.selector); + withdrawalManager.cancelWithdrawal(withdrawalIdx); + } + + function test_cancelWithdrawal_alreadyFinalized() public withUnlimitedWithdrawalLimit { + uint256 depositAmount = 1 ether; + + // Fill the batch completely before finalizing + for (uint256 i = 0; i < batchSize; i++) { + address actor = actors[i % actors.length]; + _givePufETH(depositAmount, actor); + + vm.startPrank(actor); + pufferVault.approve(address(withdrawalManager), depositAmount); + withdrawalManager.requestWithdrawal(uint128(depositAmount), actor); + vm.stopPrank(); + } + + uint256 withdrawalIdx = batchSize; + + // Finalize the batch + vm.prank(PAYMASTER); + withdrawalManager.finalizeWithdrawals(1); + + // Try to cancel the finalized withdrawal + vm.prank(alice); + vm.expectRevert(IPufferWithdrawalManager.WithdrawalAlreadyFinalized.selector); + withdrawalManager.cancelWithdrawal(withdrawalIdx); + } + + function test_cancelWithdrawal_notOwner() public withUnlimitedWithdrawalLimit { + uint256 depositAmount = 1 ether; + _givePufETH(depositAmount, alice); + + vm.startPrank(alice); + pufferVault.approve(address(withdrawalManager), depositAmount); + withdrawalManager.requestWithdrawal(uint128(depositAmount), alice); + vm.stopPrank(); + + uint256 withdrawalIdx = batchSize; + + // Try to cancel someone else's withdrawal + vm.prank(bob); + vm.expectRevert(IPufferWithdrawalManager.NotWithdrawalOwner.selector); + withdrawalManager.cancelWithdrawal(withdrawalIdx); + } + + function test_cancelWithdrawal_updatesBatch() public withUnlimitedWithdrawalLimit { + uint256 depositAmount = 1 ether; + _givePufETH(depositAmount, alice); + + vm.startPrank(alice); + pufferVault.approve(address(withdrawalManager), depositAmount); + withdrawalManager.requestWithdrawal(uint128(depositAmount), alice); + vm.stopPrank(); + + uint256 withdrawalIdx = batchSize; + uint256 batchIdx = withdrawalIdx / batchSize; + + // Check initial batch state + PufferWithdrawalManagerStorage.WithdrawalBatch memory batchBefore = withdrawalManager.getBatch(batchIdx); + assertEq(batchBefore.toBurn, depositAmount, "Initial toBurn should be depositAmount"); + assertGt(batchBefore.toTransfer, 0, "Initial toTransfer should be > 0"); + + // Cancel the withdrawal + vm.prank(alice); + withdrawalManager.cancelWithdrawal(withdrawalIdx); + + // Check that batch was updated + PufferWithdrawalManagerStorage.WithdrawalBatch memory batchAfter = withdrawalManager.getBatch(batchIdx); + assertEq(batchAfter.toBurn, 0, "toBurn should be 0 after cancellation"); + assertEq(batchAfter.toTransfer, 0, "toTransfer should be 0 after cancellation"); + } + + function test_cancelWithdrawal_multipleInBatch() public withUnlimitedWithdrawalLimit { + uint256 depositAmount = 1 ether; + + // Create multiple withdrawals in the same batch + for (uint256 i = 0; i < 3; i++) { + address actor = actors[i]; + _givePufETH(depositAmount, actor); + + vm.startPrank(actor); + pufferVault.approve(address(withdrawalManager), depositAmount); + withdrawalManager.requestWithdrawal(uint128(depositAmount), actor); + vm.stopPrank(); + } + + uint256 batchIdx = batchSize / batchSize; // Should be 1 + + // Check initial batch state + PufferWithdrawalManagerStorage.WithdrawalBatch memory batchBefore = withdrawalManager.getBatch(batchIdx); + assertEq(batchBefore.toBurn, depositAmount * 3, "Initial toBurn should be 3 * depositAmount"); + + // Cancel two withdrawals + vm.prank(actors[0]); + withdrawalManager.cancelWithdrawal(batchSize); + + vm.prank(actors[1]); + withdrawalManager.cancelWithdrawal(batchSize + 1); + + // Check that batch was updated correctly + PufferWithdrawalManagerStorage.WithdrawalBatch memory batchAfter = withdrawalManager.getBatch(batchIdx); + assertEq(batchAfter.toBurn, depositAmount, "toBurn should be depositAmount after cancelling 2 withdrawals"); + assertEq( + batchAfter.toTransfer, depositAmount, "toTransfer should be depositAmount after cancelling 2 withdrawals" + ); + } + function _givePufETH(uint256 ethAmount, address recipient) internal returns (uint256) { vm.deal(address(this), ethAmount); @@ -801,4 +968,238 @@ contract PufferWithdrawalManagerTest is UnitTestHelper { withdrawalManager.requestWithdrawal(uint128(pufETHAmount), actor); vm.stopPrank(); } + + /** + * @dev Test constructor with batchSize = 0 to cover uncovered constructor line + */ + function test_constructor_batchSizeZero() public { + // This should work fine, but we need to test the constructor path + PufferWithdrawalManager impl = new PufferWithdrawalManager(0, pufferVault, weth); + assertEq(impl.BATCH_SIZE(), 0); + } + + /** + * @dev Test oneWithdrawalRequestAllowed modifier revert path + */ + function test_oneWithdrawalRequestAllowed_revert() public withUnlimitedWithdrawalLimit { + // Upgrade to the real implementation to test the modifier + address newImpl = address( + new PufferWithdrawalManager(batchSize, PufferVaultV5(payable(address(pufferVault))), IWETH(address(weth))) + ); + vm.prank(timelock); + withdrawalManager.upgradeToAndCall(newImpl, ""); + + _givePufETH(200 ether, alice); + + vm.startPrank(alice); + pufferVault.approve(address(withdrawalManager), type(uint256).max); + + withdrawalManager.requestWithdrawal(uint128(100 ether), alice); + + // This should revert due to the modifier + vm.expectRevert(abi.encodeWithSelector(IPufferWithdrawalManager.MultipleWithdrawalsAreForbidden.selector)); + withdrawalManager.requestWithdrawal(uint128(100 ether), alice); + vm.stopPrank(); + } + + /** + * @dev Test finalizeWithdrawals edge cases for uncovered branches + */ + function test_finalizeWithdrawals_edgeCases() public withUnlimitedWithdrawalLimit { + // Test finalizing batch 0 (should revert) + vm.startPrank(PAYMASTER); + vm.expectRevert(abi.encodeWithSelector(IPufferWithdrawalManager.BatchAlreadyFinalized.selector, 0)); + withdrawalManager.finalizeWithdrawals(0); + vm.stopPrank(); + + // Test finalizing when no batches are full + vm.startPrank(PAYMASTER); + vm.expectRevert(IPufferWithdrawalManager.BatchesAreNotFull.selector); + withdrawalManager.finalizeWithdrawals(1); + vm.stopPrank(); + } + + /** + * @dev Test completeQueuedWithdrawal edge cases for uncovered branches + */ + function test_completeQueuedWithdrawal_edgeCases() public withUnlimitedWithdrawalLimit { + uint256 depositAmount = 1 ether; + + // Fill the batch + for (uint256 i = 0; i < batchSize; i++) { + address actor = actors[i]; + _givePufETH(depositAmount, actor); + vm.prank(actor); + pufferVault.approve(address(withdrawalManager), depositAmount); + vm.prank(actor); + withdrawalManager.requestWithdrawal(uint128(depositAmount), actor); + } + + // Test completing withdrawal from unfinalized batch + vm.expectRevert(IPufferWithdrawalManager.NotFinalized.selector); + withdrawalManager.completeQueuedWithdrawal(batchSize); + + // Finalize the batch + vm.prank(PAYMASTER); + withdrawalManager.finalizeWithdrawals(1); + + // Test completing already completed withdrawal + vm.prank(actors[0]); + withdrawalManager.completeQueuedWithdrawal(batchSize); + + vm.prank(actors[0]); + vm.expectRevert(IPufferWithdrawalManager.WithdrawalAlreadyCompleted.selector); + withdrawalManager.completeQueuedWithdrawal(batchSize); + } + + /** + * @dev Test _processWithdrawalRequest edge cases for uncovered branches + */ + function test_processWithdrawalRequest_edgeCases() public withUnlimitedWithdrawalLimit { + // Test withdrawal amount too high + vm.startPrank(DAO); + withdrawalManager.changeMaxWithdrawalAmount(0.1 ether); + vm.stopPrank(); + + _givePufETH(1 ether, alice); + vm.startPrank(alice); + pufferVault.approve(address(withdrawalManager), 1 ether); + vm.expectRevert(IPufferWithdrawalManager.WithdrawalAmountTooHigh.selector); + withdrawalManager.requestWithdrawal(uint128(1 ether), alice); + vm.stopPrank(); + } + + /** + * @dev Test _authorizeUpgrade edge cases for uncovered branches + */ + function test_authorizeUpgrade_edgeCases() public { + // Test upgrade with different batch size + address newImpl = address(new PufferWithdrawalManager(999, pufferVault, weth)); + + vm.startPrank(timelock); + vm.expectRevert(IPufferWithdrawalManager.BatchSizeCannotChange.selector); + withdrawalManager.upgradeToAndCall(newImpl, ""); + vm.stopPrank(); + } + + /** + * @dev Test receive function + */ + function test_receive() public { + uint256 amount = 1 ether; + vm.deal(address(this), amount); + + (bool success,) = address(withdrawalManager).call{ value: amount }(""); + assertTrue(success); + assertEq(address(withdrawalManager).balance, amount); + } + + /** + * @dev Test finalizing batch with one canceled request and claiming other requests + */ + function test_finalizeBatchWithCanceledRequest() public withUnlimitedWithdrawalLimit { + uint256 depositAmount = 1 ether; + + // Create withdrawals to fill a batch (batchSize = 10) + for (uint256 i = 0; i < batchSize; i++) { + address actor = actors[i % actors.length]; + _givePufETH(depositAmount, actor); + + vm.startPrank(actor); + pufferVault.approve(address(withdrawalManager), depositAmount); + withdrawalManager.requestWithdrawal(uint128(depositAmount), actor); + vm.stopPrank(); + } + + uint256 batchIdx = 1; // First non-zero batch + uint256 canceledWithdrawalIdx = batchSize; // First withdrawal in the batch + + // Check initial batch state + PufferWithdrawalManagerStorage.WithdrawalBatch memory batchBefore = withdrawalManager.getBatch(batchIdx); + assertEq(batchBefore.toBurn, batchSize * depositAmount, "Initial toBurn should be batchSize * depositAmount"); + assertEq( + batchBefore.toTransfer, batchSize * depositAmount, "Initial toTransfer should be batchSize * depositAmount" + ); + assertEq(batchBefore.withdrawalsClaimed, 0, "Initial withdrawalsClaimed should be 0"); + + // Cancel one withdrawal (Alice's withdrawal) + vm.prank(alice); + vm.expectEmit(true, true, true, true); + emit IPufferWithdrawalManager.WithdrawalCancelled(canceledWithdrawalIdx, depositAmount, alice); + withdrawalManager.cancelWithdrawal(canceledWithdrawalIdx); + + // Check that batch was updated after cancellation + PufferWithdrawalManagerStorage.WithdrawalBatch memory batchAfterCancel = withdrawalManager.getBatch(batchIdx); + assertEq(batchAfterCancel.toBurn, (batchSize - 1) * depositAmount, "toBurn should be reduced by depositAmount"); + assertEq( + batchAfterCancel.toTransfer, + (batchSize - 1) * depositAmount, + "toTransfer should be reduced by depositAmount" + ); + + // Verify Alice got her pufETH back + assertEq(pufferVault.balanceOf(alice), depositAmount, "Alice should have her pufETH back"); + + // Verify canceled withdrawal data was cleared + PufferWithdrawalManagerStorage.Withdrawal memory canceledWithdrawal = + withdrawalManager.getWithdrawal(canceledWithdrawalIdx); + assertEq(canceledWithdrawal.recipient, address(0), "Canceled withdrawal recipient should be cleared"); + assertEq(canceledWithdrawal.pufETHAmount, 0, "Canceled withdrawal amount should be cleared"); + + // Finalize the batch + vm.prank(PAYMASTER); + vm.expectEmit(true, true, true, true); + emit IPufferWithdrawalManager.BatchFinalized( + batchIdx, + (batchSize - 1) * depositAmount, // expectedETHAmount + (batchSize - 1) * depositAmount, // actualEthAmount + (batchSize - 1) * depositAmount // pufETHBurnAmount + ); + withdrawalManager.finalizeWithdrawals(1); + + // Check batch state after finalization + PufferWithdrawalManagerStorage.WithdrawalBatch memory batchAfterFinalize = withdrawalManager.getBatch(batchIdx); + assertEq(batchAfterFinalize.toBurn, (batchSize - 1) * depositAmount, "toBurn should remain the same"); + assertEq(batchAfterFinalize.toTransfer, (batchSize - 1) * depositAmount, "toTransfer should remain the same"); + assertEq(batchAfterFinalize.withdrawalsClaimed, 1, "withdrawalsClaimed should be 1 (the canceled withdrawal)"); + + // Test claiming other requests in the batch (skip the canceled one) + for (uint256 i = 1; i < batchSize; i++) { + uint256 withdrawalIdx = batchSize + i; + address actor = actors[i % actors.length]; + uint256 actorBalanceBefore = weth.balanceOf(actor); + + vm.prank(actor); + vm.expectEmit(true, true, true, true); + emit IPufferWithdrawalManager.WithdrawalCompleted(withdrawalIdx, depositAmount, 1 ether, actor); + withdrawalManager.completeQueuedWithdrawal(withdrawalIdx); + + uint256 actorBalanceAfter = weth.balanceOf(actor); + assertEq(actorBalanceAfter - actorBalanceBefore, depositAmount, "Actor should receive depositAmount in ETH"); + } + + // Check final batch state + PufferWithdrawalManagerStorage.WithdrawalBatch memory batchFinal = withdrawalManager.getBatch(batchIdx); + assertEq( + batchFinal.withdrawalsClaimed, + batchSize, + "withdrawalsClaimed should be batchSize (1 canceled + 9 completed)" + ); + assertEq( + batchFinal.amountClaimed, + (batchSize - 1) * depositAmount, + "amountClaimed should be (batchSize - 1) * depositAmount" + ); + + // Verify Alice cannot claim the canceled withdrawal + vm.prank(alice); + vm.expectRevert(IPufferWithdrawalManager.WithdrawalAlreadyCompleted.selector); + withdrawalManager.completeQueuedWithdrawal(canceledWithdrawalIdx); + + // Verify the canceled withdrawal slot is empty + PufferWithdrawalManagerStorage.Withdrawal memory emptyWithdrawal = + withdrawalManager.getWithdrawal(canceledWithdrawalIdx); + assertEq(emptyWithdrawal.recipient, address(0), "Canceled withdrawal should remain empty"); + assertEq(emptyWithdrawal.pufETHAmount, 0, "Canceled withdrawal amount should remain 0"); + } } From 6cdb396df4c7a1b272f06757070b13053f5563c1 Mon Sep 17 00:00:00 2001 From: Benjamin Date: Thu, 25 Sep 2025 08:45:11 +0200 Subject: [PATCH 2/4] rearange variables --- mainnet-contracts/src/PufferWithdrawalManager.sol | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/mainnet-contracts/src/PufferWithdrawalManager.sol b/mainnet-contracts/src/PufferWithdrawalManager.sol index 836554a8..db266491 100644 --- a/mainnet-contracts/src/PufferWithdrawalManager.sol +++ b/mainnet-contracts/src/PufferWithdrawalManager.sol @@ -231,25 +231,25 @@ contract PufferWithdrawalManager is require(withdrawalIdx < $.withdrawals.length, WithdrawalDoesNotExist()); Withdrawal memory withdrawal = $.withdrawals[withdrawalIdx]; + address recipient = withdrawal.recipient; // Check if withdrawal has already been completed (recipient is set to address(0) when completed) - require(withdrawal.recipient != address(0), WithdrawalAlreadyCompleted()); + require(recipient != address(0), WithdrawalAlreadyCompleted()); // Check if the caller is the original recipient - require(withdrawal.recipient == msg.sender, NotWithdrawalOwner()); + require(recipient == msg.sender, NotWithdrawalOwner()); // Check if the withdrawal's batch has been finalized uint256 batchIndex = withdrawalIdx / BATCH_SIZE; require(batchIndex > $.finalizedWithdrawalBatch, WithdrawalAlreadyFinalized()); - uint256 pufETHAmount = withdrawal.pufETHAmount; - address recipient = withdrawal.recipient; - WithdrawalBatch storage batch = $.withdrawalBatches[batchIndex]; // Treat this canceled withdrawal as a claimed withdrawal to avoid issues in the `returnExcessETHToVault` function ++batch.withdrawalsClaimed; + uint256 pufETHAmount = withdrawal.pufETHAmount; + uint256 expectedETHAmount = (pufETHAmount * withdrawal.pufETHToETHExchangeRate) / 1 ether; batch.toBurn -= uint88(pufETHAmount); batch.toTransfer -= uint96(expectedETHAmount); From dd419f8c8bc07083f447a3c635a1faddcb2382c2 Mon Sep 17 00:00:00 2001 From: Benjamin Date: Thu, 25 Sep 2025 10:23:37 +0200 Subject: [PATCH 3/4] cover some uncovered lines --- .../test/unit/PufferWithdrawalManager.t.sol | 45 +++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/mainnet-contracts/test/unit/PufferWithdrawalManager.t.sol b/mainnet-contracts/test/unit/PufferWithdrawalManager.t.sol index e422a38a..ab15c10a 100644 --- a/mainnet-contracts/test/unit/PufferWithdrawalManager.t.sol +++ b/mainnet-contracts/test/unit/PufferWithdrawalManager.t.sol @@ -1202,4 +1202,49 @@ contract PufferWithdrawalManagerTest is UnitTestHelper { assertEq(emptyWithdrawal.recipient, address(0), "Canceled withdrawal should remain empty"); assertEq(emptyWithdrawal.pufETHAmount, 0, "Canceled withdrawal amount should remain 0"); } + + function test_constructor_disableInitializers() public { + // Test that the constructor properly calls _disableInitializers() + // This is covered by the existing constructor tests, but we need to ensure coverage + PufferWithdrawalManager impl = new PufferWithdrawalManager(5, pufferVault, weth); + assertEq(impl.BATCH_SIZE(), 5); + } + + function test_getWithdrawal_uncoveredBranches() public view { + // Test getting withdrawal with invalid index + PufferWithdrawalManagerStorage.Withdrawal memory withdrawal = withdrawalManager.getWithdrawal(999); + assertEq(withdrawal.recipient, address(0), "Invalid withdrawal should return empty"); + assertEq(withdrawal.pufETHAmount, 0, "Invalid withdrawal amount should be 0"); + } + + function test_getBatch_uncoveredBranches() public view { + // Test getting batch with invalid index + PufferWithdrawalManagerStorage.WithdrawalBatch memory batch = withdrawalManager.getBatch(999); + assertEq(batch.toBurn, 0, "Invalid batch toBurn should be 0"); + assertEq(batch.toTransfer, 0, "Invalid batch toTransfer should be 0"); + assertEq(batch.withdrawalsClaimed, 0, "Invalid batch withdrawalsClaimed should be 0"); + assertEq(batch.amountClaimed, 0, "Invalid batch amountClaimed should be 0"); + } + + function test_requestWithdrawalWithPermit_failedPermit() public withUnlimitedWithdrawalLimit { + _givePufETH(1 ether, alice); + + // Approve the withdrawal manager to spend pufETH + vm.prank(alice); + pufferVault.approve(address(withdrawalManager), 1 ether); + + Permit memory permit = _signPermit( + _testTemps("alice", address(withdrawalManager), 1 ether, block.timestamp), pufferVault.DOMAIN_SEPARATOR() + ); + + // Corrupt the permit signature to make it fail + permit.v = 15; + + vm.prank(alice); + // This should still work because the permit failure is caught and ignored + withdrawalManager.requestWithdrawalWithPermit(permit, alice); + + // Verify withdrawal was created despite failed permit + assertEq(withdrawalManager.getWithdrawal(batchSize).pufETHAmount, 1 ether, "Withdrawal should be created"); + } } From 0384aeaaab78ed79fead269b306d0a2a2085c41e Mon Sep 17 00:00:00 2001 From: Benjamin Date: Thu, 25 Sep 2025 17:20:33 +0200 Subject: [PATCH 4/4] increase coverage --- .../test/unit/PufferWithdrawalManager.t.sol | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/mainnet-contracts/test/unit/PufferWithdrawalManager.t.sol b/mainnet-contracts/test/unit/PufferWithdrawalManager.t.sol index ab15c10a..08d69d08 100644 --- a/mainnet-contracts/test/unit/PufferWithdrawalManager.t.sol +++ b/mainnet-contracts/test/unit/PufferWithdrawalManager.t.sol @@ -1247,4 +1247,40 @@ contract PufferWithdrawalManagerTest is UnitTestHelper { // Verify withdrawal was created despite failed permit assertEq(withdrawalManager.getWithdrawal(batchSize).pufETHAmount, 1 ether, "Withdrawal should be created"); } + + function test_cancelNonExistantWithdrawal() public { + vm.prank(alice); + vm.expectRevert(IPufferWithdrawalManager.WithdrawalDoesNotExist.selector); + withdrawalManager.cancelWithdrawal(999); + } + + function test_cancel_OtherRecipientWithdrawal() public withUnlimitedWithdrawalLimit { + uint256 depositAmount = 1 ether; + _givePufETH(depositAmount, alice); + + vm.startPrank(alice); + pufferVault.approve(address(withdrawalManager), depositAmount); + withdrawalManager.requestWithdrawal(uint128(depositAmount), alice); + + // Bob tries to cancel Alice's withdrawal + vm.startPrank(bob); + vm.expectRevert(IPufferWithdrawalManager.NotWithdrawalOwner.selector); + withdrawalManager.cancelWithdrawal(batchSize); + } + + function test_cancel_alreadyFinalizedWithdrawal() public withUnlimitedWithdrawalLimit { + uint256 depositAmount = 1 ether; + _givePufETH(depositAmount, alice); + + vm.startPrank(alice); + pufferVault.approve(address(withdrawalManager), depositAmount); + withdrawalManager.requestWithdrawal(uint128(depositAmount), alice); + + // First time it works + withdrawalManager.cancelWithdrawal(batchSize); + + // Second time it reverts + vm.expectRevert(IPufferWithdrawalManager.WithdrawalAlreadyCompleted.selector); + withdrawalManager.cancelWithdrawal(batchSize); + } }