Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -44,3 +44,6 @@ build/deployments-fork*.json

# Other
.DS_Store

# Cursor solidity plugin
remappings.txt
17 changes: 16 additions & 1 deletion src/contracts/adapters/EtherFiAssetAdapter.sol
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,12 @@ contract EtherFiAssetAdapter is Initializable, IAssetAdapter, IERC721Receiver {
uint256[] internal pendingRequestIds;
uint256 internal nextPendingIndex;

/// @dev True only while the adapter is actively claiming Ether.fi withdrawal NFTs.
bool internal claimingEtherFi;

/// @notice Thrown when Ether.fi claim proceeds arrive without an adapter-initiated claim.
error UnauthorizedEtherFiClaim(); // 0x7b2b45a4

modifier onlyARM() {
require(msg.sender == arm, "Adapter: only ARM");
_;
Expand Down Expand Up @@ -138,7 +144,9 @@ contract EtherFiAssetAdapter is Initializable, IAssetAdapter, IERC721Receiver {
}
nextPendingIndex = cursor + claimCount;

claimingEtherFi = true;
etherfiWithdrawalNFT.batchClaimWithdraw(requestIds);
claimingEtherFi = false;

uint256 ethBalance = address(this).balance;
if (ethBalance > 0) weth.deposit{value: ethBalance}();
Expand All @@ -158,7 +166,14 @@ contract EtherFiAssetAdapter is Initializable, IAssetAdapter, IERC721Receiver {
return pendingRequestIds[index];
}

receive() external payable {}
/// @notice Accepts ETH only while this adapter is claiming Ether.fi withdrawals.
/// @dev ETH arriving outside an adapter-initiated claim (e.g. a permissionless Ether.fi claim) is
/// rejected. Ether.fi reverts the entire claim, including the NFT burn, when this transfer fails.
receive() external payable {
if (!claimingEtherFi) {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@clement-ux is making a point that this check could be:

 if (msg.sender == address(etherfiWithdrawalNFT) && !claimingEtherFi) {

There are pros/cons to it:
🟢 code change would allow sending ETH to adapter in case there was a shortfall
🔴 code change would allow a donation attack

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I lean towards not allowing for a donation attack and accounting for everything. In case we ever need to infuse the adapter/ARM with funds I would rather we make a deliberate function for it

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You can always send weth

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

After some thought, I agree that we should keep the first implementation (msg.sender == address(etherfiWithdrawalNFT) && !claimingEtherFi).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

nit: you probably ment this by the first implementation:

if (!claimingEtherFi) {

revert UnauthorizedEtherFiClaim();
}
}

/// @notice Accepts Ether.fi withdrawal NFTs minted to this adapter.
function onERC721Received(address, address, uint256, bytes calldata) external pure returns (bytes4) {
Expand Down
17 changes: 16 additions & 1 deletion src/contracts/adapters/WeETHAssetAdapter.sol
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,12 @@ contract WeETHAssetAdapter is Initializable, IAssetAdapter, IERC721Receiver {
uint256[] internal pendingRequestIds;
uint256 internal nextPendingIndex;

/// @dev True only while the adapter is actively claiming Ether.fi withdrawal NFTs.
bool internal claimingEtherFi;

/// @notice Thrown when Ether.fi claim proceeds arrive without an adapter-initiated claim.
error UnauthorizedEtherFiClaim(); // 0x7b2b45a4

modifier onlyARM() {
require(msg.sender == arm, "Adapter: only ARM");
_;
Expand Down Expand Up @@ -148,7 +154,9 @@ contract WeETHAssetAdapter is Initializable, IAssetAdapter, IERC721Receiver {
}
nextPendingIndex = cursor + claimCount;

claimingEtherFi = true;
etherfiWithdrawalNFT.batchClaimWithdraw(requestIds);
claimingEtherFi = false;

uint256 ethBalance = address(this).balance;
if (ethBalance > 0) weth.deposit{value: ethBalance}();
Expand All @@ -168,7 +176,14 @@ contract WeETHAssetAdapter is Initializable, IAssetAdapter, IERC721Receiver {
return pendingRequestIds[index];
}

receive() external payable {}
/// @notice Accepts ETH only while this adapter is claiming Ether.fi withdrawals.
/// @dev ETH arriving outside an adapter-initiated claim (e.g. a permissionless Ether.fi claim) is
/// rejected. Ether.fi reverts the entire claim, including the NFT burn, when this transfer fails.
receive() external payable {
if (!claimingEtherFi) {
revert UnauthorizedEtherFiClaim();
}
}

/// @notice Accepts Ether.fi withdrawal NFTs minted to this adapter.
function onERC721Received(address, address, uint256, bytes calldata) external pure returns (bytes4) {
Expand Down
63 changes: 59 additions & 4 deletions test/unit/adapters/concrete/EtherFiAssetAdapter.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -280,10 +280,65 @@ contract Unit_EtherFiAssetAdapter_Test is Test {
);
}

function test_Receive_AcceptsEth() public {
function test_Receive_RevertWhen_EthArrivesOutsideClaim() public {
// Any ETH — not just NFT-forwarded claim proceeds — is refused outside an adapter-initiated claim.
vm.deal(address(this), 1 ether);
(bool ok,) = address(adapter).call{value: 1 ether}("");
assertTrue(ok, "adapter accepts ETH");
assertEq(address(adapter).balance, 1 ether, "adapter eth balance");
(bool ok, bytes memory ret) = address(adapter).call{value: 1 ether}("");

assertFalse(ok, "adapter rejects ETH outside a claim");
assertEq(bytes4(ret), EtherFiAssetAdapter.UnauthorizedEtherFiClaim.selector, "revert selector");
assertEq(address(adapter).balance, 0, "no ETH retained");
}

//////////////////////////////////////////////////////
/// --- permissionless Ether.fi claim gate (Immunefi: NAV double-count)
//////////////////////////////////////////////////////

/// @notice ETH forwarded by the Ether.fi NFT contract outside an adapter-initiated claim is rejected
/// with `UnauthorizedEtherFiClaim`. This is the exact transfer EtherFi makes to the NFT owner
/// during a permissionless `claimWithdraw`; rejecting it (and thus reverting the claim) keeps the
/// withdrawal request — and the ARM's pending-redeem accounting — in sync.
function test_Receive_RejectsNftForwardedEthOutsideClaim() public {
vm.deal(address(etherfi), 1 ether);
vm.prank(address(etherfi));
(bool ok, bytes memory ret) = address(adapter).call{value: 1 ether}("");

assertFalse(ok, "NFT-forwarded ETH must be rejected outside a claim");
assertEq(bytes4(ret), EtherFiAssetAdapter.UnauthorizedEtherFiClaim.selector, "revert selector");
assertEq(address(adapter).balance, 0, "no ETH retained");
}

/// @notice A third party cannot claim an adapter-owned Ether.fi withdrawal NFT out-of-band. EtherFi
/// forwards proceeds to the NFT owner (the adapter) and reverts the whole claim (incl. the NFT burn)
/// if that transfer fails. The adapter rejects proceeds it did not initiate, so the permissionless
/// claim reverts and the request stays pending — the adapter's own claim still works afterward.
/// Without the gate this path would drop ETH into the adapter while the pending request survived,
/// double-counting it in the ARM's NAV (the reported vulnerability).
function test_ExternalClaim_RevertWhen_NotAdapterInitiated() public {
uint256 shares = 500 ether;

vm.prank(arm);
adapter.requestRedeem(shares);
uint256 id = adapter.pendingRequestId(0);

// Permissionless third-party claim: the mock (standing in for EtherFi's WithdrawRequestNFT)
// forwards ETH to the owner, whose receive() rejects it; the mock bubbles that reason (EtherFi
// would surface its own EthTransferFailed), so the whole claim reverts on the adapter's gate.
vm.prank(alice);
vm.expectRevert(EtherFiAssetAdapter.UnauthorizedEtherFiClaim.selector);
etherfi.claimWithdraw(id);

// Request survives untouched: nothing claimed, share accounting intact, no stray ETH.
(,,, bool claimed) = etherfi.requests(id);
assertFalse(claimed, "request must stay unclaimed");
assertEq(adapter.requestShares(id), shares, "requestShares intact");
assertEq(address(adapter).balance, 0, "adapter holds no ETH");

// The adapter's own claim path still works and delivers WETH to the ARM.
uint256 armWethBefore = weth.balanceOf(arm);
vm.prank(arm);
(uint256 sharesClaimed,, uint256 assetsReceived) = adapter.redeem(shares);
assertEq(sharesClaimed, shares, "adapter redeem claims the request");
assertEq(weth.balanceOf(arm), armWethBefore + assetsReceived, "ARM received WETH");
}
}
63 changes: 59 additions & 4 deletions test/unit/adapters/concrete/WeETHAssetAdapter.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -264,10 +264,65 @@ contract Unit_WeETHAssetAdapter_Test is Test {
);
}

function test_Receive_AcceptsEth() public {
function test_Receive_RevertWhen_EthArrivesOutsideClaim() public {
// Any ETH — not just NFT-forwarded claim proceeds — is refused outside an adapter-initiated claim.
vm.deal(address(this), 1 ether);
(bool ok,) = address(adapter).call{value: 1 ether}("");
assertTrue(ok, "adapter accepts ETH");
assertEq(address(adapter).balance, 1 ether, "adapter eth balance");
(bool ok, bytes memory ret) = address(adapter).call{value: 1 ether}("");

assertFalse(ok, "adapter rejects ETH outside a claim");
assertEq(bytes4(ret), WeETHAssetAdapter.UnauthorizedEtherFiClaim.selector, "revert selector");
assertEq(address(adapter).balance, 0, "no ETH retained");
}

//////////////////////////////////////////////////////
/// --- permissionless Ether.fi claim gate (Immunefi: NAV double-count)
//////////////////////////////////////////////////////

/// @notice ETH forwarded by the Ether.fi NFT contract outside an adapter-initiated claim is rejected
/// with `UnauthorizedEtherFiClaim`. This is the exact transfer EtherFi makes to the NFT owner
/// during a permissionless `claimWithdraw`; rejecting it (and thus reverting the claim) keeps the
/// withdrawal request — and the ARM's pending-redeem accounting — in sync.
function test_Receive_RejectsNftForwardedEthOutsideClaim() public {
vm.deal(address(etherfi), 1 ether);
vm.prank(address(etherfi));
(bool ok, bytes memory ret) = address(adapter).call{value: 1 ether}("");

assertFalse(ok, "NFT-forwarded ETH must be rejected outside a claim");
assertEq(bytes4(ret), WeETHAssetAdapter.UnauthorizedEtherFiClaim.selector, "revert selector");
assertEq(address(adapter).balance, 0, "no ETH retained");
}

/// @notice A third party cannot claim an adapter-owned Ether.fi withdrawal NFT out-of-band. EtherFi
/// forwards proceeds to the NFT owner (the adapter) and reverts the whole claim (incl. the NFT burn)
/// if that transfer fails. The adapter rejects proceeds it did not initiate, so the permissionless
/// claim reverts and the request stays pending — the adapter's own claim still works afterward.
/// Without the gate this path would drop ETH into the adapter while the pending request survived,
/// double-counting it in the ARM's NAV (the reported vulnerability).
function test_ExternalClaim_RevertWhen_NotAdapterInitiated() public {
uint256 shares = 500 ether;

vm.prank(arm);
adapter.requestRedeem(shares);
uint256 id = adapter.pendingRequestId(0);

// Permissionless third-party claim: the mock (standing in for EtherFi's WithdrawRequestNFT)
// forwards ETH to the owner, whose receive() rejects it; the mock bubbles that reason (EtherFi
// would surface its own EthTransferFailed), so the whole claim reverts on the adapter's gate.
vm.prank(alice);
vm.expectRevert(WeETHAssetAdapter.UnauthorizedEtherFiClaim.selector);
etherfi.claimWithdraw(id);

// Request survives untouched: nothing claimed, share accounting intact, no stray ETH.
(,,, bool claimed) = etherfi.requests(id);
assertFalse(claimed, "request must stay unclaimed");
assertEq(adapter.requestShares(id), shares, "requestShares intact");
assertEq(address(adapter).balance, 0, "adapter holds no ETH");

// The adapter's own claim path still works and delivers WETH to the ARM.
uint256 armWethBefore = weth.balanceOf(arm);
vm.prank(arm);
(uint256 sharesClaimed,, uint256 assetsReceived) = adapter.redeem(shares);
assertEq(sharesClaimed, shares, "adapter redeem claims the request");
assertEq(weth.balanceOf(arm), armWethBefore + assetsReceived, "ARM received WETH");
}
}
17 changes: 13 additions & 4 deletions test/unit/adapters/mocks/MockEtherFiWithdraw.sol
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@ import {ERC20} from "@solmate/tokens/ERC20.sol";
/// @notice Test double for Ether.fi's withdrawal queue and withdrawal NFT, combined into one contract.
/// Implements the subset used by `EtherFiAssetAdapter` / `WeETHAssetAdapter`:
/// `requestWithdraw` (pulls eETH from the caller and opens a request) and
/// `batchClaimWithdraw` / `claimWithdraw` (sends ETH to the claimer). Requests are finalized
/// `batchClaimWithdraw` / `claimWithdraw` (sends ETH to the request recipient, i.e. the NFT
/// owner, as EtherFi does — the claim is permissionless but proceeds go to the owner). Requests are finalized
/// on creation; `mock_*` setters drive the adapter's un-finalized / claimed edge-case branches.
/// The mock must be pre-funded with ETH so claims can pay out.
contract MockEtherFiWithdraw {
Expand Down Expand Up @@ -37,7 +38,7 @@ contract MockEtherFiWithdraw {
requests[requestId] = Request({recipient: recipient, amount: amount, finalized: true, claimed: false});
}

/// @dev Claims finalized requests in batch, sending 1:1 ETH to the caller (the adapter / NFT holder).
/// @dev Claims finalized requests in batch, sending 1:1 ETH to each request's recipient (the NFT owner).
function batchClaimWithdraw(uint256[] calldata requestIds) external {
for (uint256 i = 0; i < requestIds.length; ++i) {
_claim(requestIds[i]);
Expand Down Expand Up @@ -66,7 +67,15 @@ contract MockEtherFiWithdraw {
require(!request.claimed, "Mock EF: already claimed");
request.claimed = true;

(bool ok,) = msg.sender.call{value: request.amount}("");
require(ok, "Mock EF: eth transfer failed");
// EtherFi pays the NFT owner (the recorded recipient), not the caller, and reverts the whole
// claim — including the NFT burn — if that transfer fails. Real EtherFi masks the failure as
// `EthTransferFailed()`; the mock bubbles the recipient's revert reason instead so tests can
// assert the adapter's gate (`UnauthorizedEtherFiClaim`) is what blocks an out-of-band claim.
(bool ok, bytes memory ret) = request.recipient.call{value: request.amount}("");
if (!ok) {
assembly {
revert(add(ret, 0x20), mload(ret))
}
}
}
}
Loading