Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
49 changes: 49 additions & 0 deletions script/deploy/mainnet/041_RedeployEtherFiAdaptersScript.s.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
// SPDX-License-Identifier: MIT
pragma solidity 0.8.23;

// Contracts
import {Proxy} from "contracts/Proxy.sol";
import {Mainnet} from "contracts/utils/Addresses.sol";
import {EtherFiAssetAdapter} from "contracts/adapters/EtherFiAssetAdapter.sol";
import {WeETHAssetAdapter} from "contracts/adapters/WeETHAssetAdapter.sol";

// Deployment
import {AbstractDeployScript} from "script/deploy/helpers/AbstractDeployScript.s.sol";

/// @title Redeploy the WETH ARM Ether.fi adapters
/// @notice Deploys new implementations for the eETH and weETH adapters, then upgrades their existing
/// proxies. The proxies are owned directly by the mainnet 2/8 multisig, so the upgrades do not
/// require a governance proposal.
/// @dev Reusing the existing proxies preserves their addresses and withdrawal-request storage.
contract $041_RedeployEtherFiAdaptersScript is AbstractDeployScript("041_RedeployEtherFiAdaptersScript") {
function _execute() internal override {
address arm = resolver.resolve("WETH_ARM");

EtherFiAssetAdapter eethAdapterImpl = new EtherFiAssetAdapter(
arm, Mainnet.EETH, Mainnet.WETH, Mainnet.ETHERFI_WITHDRAWAL, Mainnet.ETHERFI_WITHDRAWAL_NFT
);
_recordDeployment("WETH_ARM_EETH_ADAPTER_IMPL", address(eethAdapterImpl));

WeETHAssetAdapter weethAdapterImpl = new WeETHAssetAdapter(
arm, Mainnet.WEETH, Mainnet.EETH, Mainnet.WETH, Mainnet.ETHERFI_WITHDRAWAL, Mainnet.ETHERFI_WITHDRAWAL_NFT
);
_recordDeployment("WETH_ARM_WEETH_ADAPTER_IMPL", address(weethAdapterImpl));
}

function _fork() internal override {
_upgradeAdapter("WETH_ARM_EETH_ADAPTER", "WETH_ARM_EETH_ADAPTER_IMPL");
_upgradeAdapter("WETH_ARM_WEETH_ADAPTER", "WETH_ARM_WEETH_ADAPTER_IMPL");
}

function _upgradeAdapter(string memory proxyName, string memory implementationName) internal {
Proxy adapterProxy = Proxy(payable(resolver.resolve(proxyName)));
address adapterImpl = resolver.resolve(implementationName);

// Idempotent: the deployment runner can replay pending multisig actions on forks.
if (adapterProxy.implementation() == adapterImpl) return;

require(adapterProxy.owner() == Mainnet.MULTISIG_2_OF_8, "Unexpected adapter owner");
vm.prank(Mainnet.MULTISIG_2_OF_8);
adapterProxy.upgradeTo(adapterImpl);
}
}
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
17 changes: 17 additions & 0 deletions test/smoke/WETHARMSmokeTest.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import {AbstractSmokeTest} from "./AbstractSmokeTest.sol";

import {MultiAssetARM} from "contracts/MultiAssetARM.sol";
import {CapManager} from "contracts/CapManager.sol";
import {Proxy} from "contracts/Proxy.sol";
import {Mainnet} from "contracts/utils/Addresses.sol";

contract Fork_WETHARM_Smoke_Test is AbstractSmokeTest {
Expand Down Expand Up @@ -50,6 +51,22 @@ contract Fork_WETHARM_Smoke_Test is AbstractSmokeTest {
_assertBaseAssetConfig(Mainnet.WEETH, "WETH_ARM_WEETH_ADAPTER", false);
}

function test_EtherFiAdapterUpgrades() external view {
Proxy eethAdapter = Proxy(payable(resolver.resolve("WETH_ARM_EETH_ADAPTER")));
Proxy weethAdapter = Proxy(payable(resolver.resolve("WETH_ARM_WEETH_ADAPTER")));

assertEq(eethAdapter.owner(), Mainnet.MULTISIG_2_OF_8, "eETH adapter owner");
assertEq(weethAdapter.owner(), Mainnet.MULTISIG_2_OF_8, "weETH adapter owner");
assertEq(
eethAdapter.implementation(), resolver.resolve("WETH_ARM_EETH_ADAPTER_IMPL"), "eETH adapter implementation"
);
assertEq(
weethAdapter.implementation(),
resolver.resolve("WETH_ARM_WEETH_ADAPTER_IMPL"),
"weETH adapter implementation"
);
}

function _assertBaseAssetConfig(address baseAsset, string memory adapterName, bool pegged) internal view {
(
uint128 buyPrice,
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