New ARM features before Ethena audit#208
Conversation
…hdraw from the lending market if not enough liquidity in the ARM
…the swap functions
naddison36
changed the title
naddison36
changed the title
| buyLiquidityRemaining = buyAmount; | ||
| sellLiquidityRemaining = sellAmount; | ||
|
|
||
| emit TraderateChanged(traderate0, traderate1); |
There was a problem hiding this comment.
If we have enough space on contract size, it could be worth for integrator/MEV to emit the buyLiquidityRemaining and sellLiquidityRemaining. WDYT?
There was a problem hiding this comment.
in the past we have avoided emitted events in the swaps to save gas. But gas is becoming less important. We could add a new Swap event that includes the remaining liquidity.
There was a problem hiding this comment.
I meant it for setPrices() not for swap, but it could make sense too.
There was a problem hiding this comment.
Good point. I've added in commit 28d4fb6
* Using swap multiplier to calculate accrued fee * Removed Maths lib to get contract size down Removed lastAvailableAssets() function * Improved gas usage of _accrueSwapFee by removing if swapFeeMultiplier is zero * Fixed gap after storage variables were compacted * Removed outToken from _accrueSwapFee * Simplified _updateSwapFeeMultiplier * More gas savings Allow price liquidity cap to be unlimited * Moved state changes before token transfers * Update swap gas compare test
* WIP multi base assets * - Move reusable multi-base asset config and swap accounting into AbstractARM - Replace protocol-specific ARM redemption logic with generic asset adapters - removed legacy max-cap sentinel handling - Upgrade all ARMs to multi base assets - Added wstETH to the Lido ARM - Added weETH to EtherFi ARM * Updated AI context * Added wOETH to OETH ARM * Updated ARM contract dependencies * Updated Hardhat tasks * Proxied the adapters * Lido ARM upgrade checks * Added Ethena upgrade guard * Updated migration error * More cleanup of legacy immutables * fmt * Update EtherFi ABI * Update claimEthenaWithdrawals Hardhat task * Fixed fork tests * Fixed smoke tests * Fix invariant tests * Updated contract diagrams * Renamed the ARM adapter-facing methods to requestBaseAssetRedeem and claimBaseAssetRedeem * Used modifiers on adapters * Pro-rata losses to redeemers and remaining LPs (#223) * Pro-rata losses to redeemers and remaining LPs * Calculate swap fee against cross price rather than par value (#224)
| function migrateLegacyWithdrawQueue() external onlyOwner { | ||
| require(withdrawsQueuedShares == 0 && withdrawsClaimedShares == 0, "ARM: already migrated"); | ||
|
|
||
| uint256 packedLegacyQueue = reservedWithdrawLiquidity; | ||
| uint128 legacyQueued = uint128(packedLegacyQueue); | ||
| uint128 legacyClaimed = uint128(packedLegacyQueue >> 128); | ||
| require(legacyQueued == legacyClaimed, "ARM: legacy withdrawals pending"); | ||
|
|
||
| reservedWithdrawLiquidity = 0; | ||
| } |
There was a problem hiding this comment.
🔴 This function is not called as part of the deploy script. But reservedWithdrawLiquidity is used as part of requestRedeem, getReserves and claimRedeem, all those methods seem to have no checks to ensure that migration has happened. if there's a pending withdrawal, migration wouldn't work and claim wouldn't work either since it's overriding the slots, am I right?
Here's what Claude has to say when I asked it this question:
You're essentially right, but with some nuance worth tracing precisely. Let me walk through what actually happens.
The packed legacy slot
Pre-upgrade, the slot holds withdrawsQueued | (withdrawsClaimed << 128). Cases:
- Fresh ARM, never had a queue: slot = 0. Migration trivially passes.
- ARM with pending requests, none claimed yet: low 128 = queued, high 128 = 0. Not bloated — reservedWithdrawLiquidity reads as just queued. The new code semi-coincidentally treats this as "amount reserved for withdrawals" — semantically
similar to the old meaning.
- ARM with claim history: high 128 = cumulative claimed. Bloated. A real Lido ARM with months of claims will sit somewhere in the 10⁵⁰+ range.
For the deployed Lido / EtherFi / Ethena ARMs that have been operating, you should assume bloated.
Your two claims
"migration wouldn't work" — correct
migrateLegacyWithdrawQueue requires legacyQueued == legacyClaimed (i.e. the packed slot must be X | X<<128). Any pending pre-upgrade LP request makes legacyQueued > legacyClaimed, and migration reverts.
The upgrade scripts (028 / 029 / 030 / 006) only check the protocol queue (checkNoLegacyLidoWithdrawalRequests checks _deprecatedLidoWithdrawalQueueAmount — the Lido NFT queue, not the LP queue). Nothing in the deploy data calls
migrateLegacyWithdrawQueue, and nothing prevents the upgrade going through with LP queue pending.
"claim wouldn't work" — partially right, mechanism is more subtle
The legacy claim function is gone (old claimRedeem on LidoARM/etc is removed). Legacy entries can only be claimed through the new claimRedeem. Here's what happens for a legacy request post-upgrade:
- The struct grew (added uint128 shares). Legacy entries occupy slots 0–1; slot 2 (shares) reads as 0 — uninitialized.
- claimRedeem line 750: assetsAtClaim = request.shares > 0 ? convertToAssets(request.shares) : request.assets → falls back to request.assets. OK.
- _burn(address(this), request.shares) → burn 0. OK.
- request.queued <= claimable() — legacy queued was in liquidity-asset terms, new claimable() is in share terms. For 1:1 share-asset ratios this passes numerically by coincidence; for non-1:1 (e.g. after gains/losses) the comparison is
semantically meaningless and could pass or fail wrongly.
- reservedWithdrawLiquidity -= request.assets — subtracts a small number from the bloated value. Slot is still bloated.
So the LP actually gets their funds — claim "works" in the sense that they're paid. The catastrophe is elsewhere.
The actual catastrophic consequence
reservedWithdrawLiquidity being bloated (~10⁵⁰+) breaks four call sites:
1. _ensureLiquidityAvailableForSwap (line 444): requiredLiquidity = amount + reservedWithdrawLiquidity overflows past any plausible liquidityBalance, falls into the market-withdraw path, tries to pull 10⁵⁰ from the market, reverts. All
buy-side swaps are dead. Sell-side swaps don't call this, so they still work.
2. _allocate line 1051: currentArmLiquidity = balance - reservedWithdrawLiquidity is hugely negative → _allocate tries to withdraw an absurd amount from the market every time, reverts.
3. _requireLiquidityAvailable (line 838): any positive amount fails → collectFees is dead.
4. getReserves (line 821): reservedWithdrawLiquidity > liquidityAssets, so returns 0 → off-chain price routers see zero liquidity.
And then migration becomes permanently impossible
migrateLegacyWithdrawQueue requires both withdrawsQueuedShares == 0 && withdrawsClaimedShares == 0. The instant any LP calls requestRedeem post-upgrade, withdrawsQueuedShares becomes non-zero forever. Even after that new request is claimed,
withdrawsClaimedShares is also non-zero forever. There is no path to call migrateLegacyWithdrawQueue once any new LP activity has happened.
So the operational sequence required to land this upgrade safely is:
1. Drain LP queue (all pending claimed).
2. Drain protocol queue (Lido NFT etc).
3. Freeze new LP activity.
4. Upgrade.
5. Owner calls migrateLegacyWithdrawQueue immediately, before unfreezing.
6. Resume.
Steps 1, 3, and 5 are not enforced by code, not bundled into upgradeToAndCall, and not asserted by any test. Step 3 isn't even possible — there's no pause mechanism on AbstractARM (no Pausable inheritance, no whenNotPaused modifiers). The
owner can't freeze deposits/redeems atomically with the upgrade.
Verdict
You're right that this is a footgun. The right fix is one of:
- Bundle the migration into upgradeToAndCall with a multicall (upgradeToAndCallBatch or have the new impl expose a single initializeUpgrade() that both checks the protocol queue and clears the legacy slot in one call). Then step 3 is just
"land the upgrade tx".
- Loosen the migration's pre-condition to "any time the legacy queued == claimed in the slot, even if new activity exists". This lets ops run migration whenever the legacy queue is balanced — though they still need to drain pending pre-upgrade
requests first.
- Add a pause modifier to requestRedeem so the owner can freeze new activity between upgrade and migration.
I'm adding this as a new Critical finding (C6) to the review document, with severity equal to C5 — it's effectively the operational version of the same root cause: the migration assumes a clean state that the deploy scripts don't enforce.
There was a problem hiding this comment.
I'm joining Shahul on this subject, and I think that someone can block us into the migration process.
- Check the governance proposal
- Deposit+request
- never claim it
- we are stuck.
Should we be able to claim on behalf of someone and remove this require on claimRedeem()? :
require(request.withdrawer == msg.sender, "Not requester");There was a problem hiding this comment.
You are correct. I missed making it so the Operator can claim a redeem on behalf of someone else. Fixed in commit 1988a98
There was a problem hiding this comment.
It looks good. But I also wonder if we should pause new requests until the migration happened. Otherwise, someone could theoretically create requests constantly to DoS from us upgrading it. Also, what about the storage slot overriding behaviour? If we delay the migration after upgrade, it'll mess up the storage slots
There was a problem hiding this comment.
I'm trying to squeeze in pause in PR #226
There was a problem hiding this comment.
Perhaps I'm missing something but I still think this is gonna mess up that storage slot: https://github.com/OriginProtocol/arm-oeth/blob/nicka/withdraw-on-swap/src/contracts/AbstractARM.sol#L756-L757
claimRedeem modifies that variable as well and it's not excluded during pause. This is fine for newly deployed ARM contracts. But for the upgrades, would be better to test this on fork first
There was a problem hiding this comment.
Any claimRedeem will also make the migration to not run since it messes up that storage slot
There was a problem hiding this comment.
It is annoying to address this issue while also battling with the contract size limits. There is another way this could be done which is the cleanest, but operationally very annoying:
- upgrade the existing ARMs by adding the ability to pause (we would need to be mindful of deprecating this storage slot in this PR / future contract upgrade) and claim withdrawals on other user's behalf
- once upgrade lands, and withdrawals are claimable claim all of them
- upgrade the arms to this implementation where the withdrawal queues are drained. The migration call happens atomically in the deploy script's "upgradeToAndCall"
| uint256 convertedAmountOut = _convertToAssets(config, amountOut); | ||
| // amountOut is converted to liquidity terms first, then multiplied by sellPrice | ||
| // to solve for the required liquidity input. | ||
| amountIn = convertedAmountOut * config.sellPrice / PRICE_SCALE + 3; |
There was a problem hiding this comment.
Why add 3 wei? To accounting for rounding issues? Wouldn't 1 wei work in that case?
There was a problem hiding this comment.
because stETH can round more with larger transfers. I've seen 2 wei. 3 wei was to be safe
There was a problem hiding this comment.
A comment about this would be good
| } | ||
|
|
||
| function setUnstakers(address[MAX_UNSTAKERS] calldata _unstakers) external onlyOwner { | ||
| unstakers = _unstakers; |
There was a problem hiding this comment.
Claude flagged this:
EthenaAssetAdapter.setUnstakers blindly overwrites — can orphan pending requests
function setUnstakers(address[MAX_UNSTAKERS] calldata _unstakers) external onlyOwner {
unstakers = _unstakers;
}
If any slot currently holds an unstaker with a non-zero requestShares[unstaker] (still cooling down), replacing it makes the funds locked in the old unstaker's cooldown unrecoverable through this adapter — redeem loops over
pendingUnstakerIndexes and reads unstakers[idx], which now points to a new (empty) address.
Recommendation: require requestShares[oldUnstaker] == 0 for every slot being overwritten, OR add a replaceUnstaker(uint8 index, address newUnstaker) that does the check per-slot.
There was a problem hiding this comment.
Looks good but I think we probably need to be cautious of the length of the unstaker array. If it's too long to fit in a block tx, we will be blocked from using this function without an upgrade
There was a problem hiding this comment.
MAX_UNSTAKERS is currently set to 42. I think that's small enough
| * legacy single-base storage so Lido, EtherFi, Ethena, and Origin ARMs can share this implementation. | ||
| * @author Origin Protocol Inc | ||
| */ | ||
| abstract contract AbstractARM is OwnableOperable, ERC20Upgradeable { |
There was a problem hiding this comment.
We usually have a method to recover tokens (transferToken), should we have something like that here for non-base assets?
There was a problem hiding this comment.
We are super tight on the contract size. That would definitely push the ARM contracts to be oversized
| lastAvailableAssets -= SafeCast.toInt128(SafeCast.toInt256(assets)); | ||
|
|
||
| // Escrow the redeemer's shares so they stay in totalSupply() and share losses/gains pro-rata. | ||
| _transfer(msg.sender, address(this), shares); |
There was a problem hiding this comment.
There seems to be no shares > 0 check here. This _transfer could fail for zero value transfers but having an explicit error would be nice
There was a problem hiding this comment.
_transfer itself allows zero-value transfers in OZ v5, but we should probably reject zero-share redeems explicitly so users cannot create empty withdrawal requests / advance the queue. The problem is we don't have much room to add this check.
The same goes for zero deposits in PR #229
I'll see if I can get the contract size down
clement-ux
left a comment
clement-ux
left a comment
There was a problem hiding this comment.
First review session focused on high level code architecture and not on security.
| function migrateLegacyWithdrawQueue() external onlyOwner { | ||
| require(withdrawsQueuedShares == 0 && withdrawsClaimedShares == 0, "ARM: already migrated"); | ||
|
|
||
| uint256 packedLegacyQueue = reservedWithdrawLiquidity; | ||
| uint128 legacyQueued = uint128(packedLegacyQueue); | ||
| uint128 legacyClaimed = uint128(packedLegacyQueue >> 128); | ||
| require(legacyQueued == legacyClaimed, "ARM: legacy withdrawals pending"); | ||
|
|
||
| reservedWithdrawLiquidity = 0; | ||
| } |
There was a problem hiding this comment.
I'm joining Shahul on this subject, and I think that someone can block us into the migration process.
- Check the governance proposal
- Deposit+request
- never claim it
- we are stuck.
Should we be able to claim on behalf of someone and remove this require on claimRedeem()? :
require(request.withdrawer == msg.sender, "Not requester");
|
Another idea to decrease the size of the contract could be to move the lending part into an external contract, like you did for the
It shouldn't increase gas cost for swap, even for large swap that need to withdraw from lending market. |
…the check in setPrice should also include base assets in the withdrawal queue
|
|
||
| if (!isBuySide) { | ||
| // Trader sells liquidity asset and buys the base asset. | ||
| // The ARM buys the liquidity asset and sells the base asset. | ||
| // ARM prices the base sale at sellPrice. | ||
| uint256 convertedAmountIn = _convertToShares(config, amountIn); | ||
| // sellPrice is liquidity assets per base asset, so divide liquidity input by sellPrice | ||
| // to get the base output owed to the trader. | ||
| amountOut = convertedAmountIn * PRICE_SCALE / config.sellPrice; | ||
| } else { | ||
| revert("ARM: Invalid in token"); | ||
| // Trader sells base asset and buys the liquidity asset. | ||
| // The ARM buys the base asset and sells the liquidity asset. | ||
| // ARM prices the base purchase at buyPrice. | ||
| uint256 convertedAmountIn = _convertToAssets(config, amountIn); | ||
| // buyPrice is liquidity assets per base asset. Since convertedAmountIn is the | ||
| // base input expressed in liquidity terms, multiply by buyPrice to get liquidity output. | ||
| amountOut = convertedAmountIn * config.buyPrice / PRICE_SCALE; | ||
|
|
||
| _accrueSwapFee(config.buyPrice, config.crossPrice, amountOut); | ||
| _ensureLiquidityAvailableForSwap(amountOut); | ||
| } |
There was a problem hiding this comment.
nit: this can be simplified into:
uint256 convertedAmountIn = _convertToShares(config, amountIn);
if (!isBuySide) {
// Trader sells liquidity asset and buys the base asset.
// The ARM buys the liquidity asset and sells the base asset.
// ARM prices the base sale at sellPrice.
// sellPrice is liquidity assets per base asset, so divide liquidity input by sellPrice
// to get the base output owed to the trader.
amountOut = convertedAmountIn * PRICE_SCALE / config.sellPrice;
} else {
// Trader sells base asset and buys the liquidity asset.
// The ARM buys the base asset and sells the liquidity asset.
// ARM prices the base purchase at buyPrice.
// buyPrice is liquidity assets per base asset. Since convertedAmountIn is the
// base input expressed in liquidity terms, multiply by buyPrice to get liquidity output.
amountOut = convertedAmountIn * config.buyPrice / PRICE_SCALE;
_accrueSwapFee(config.buyPrice, config.crossPrice, amountOut);
_ensureLiquidityAvailableForSwap(amountOut);
}| uint256 feeMultiplier = | ||
| buyPrice == 0 ? 0 : (crossPrice - buyPrice) * uint256(fee) * PRICE_SCALE / (buyPrice * FEE_SCALE); |
There was a problem hiding this comment.
I think we already tested it before, should we store this value and update it at every call of setPrices() and setFee(). Like this we avoid this calculation at every swap and thus reduce gas cost for a swap.
| /// @param isBuySide True when the ARM buys base asset and pays out liquidity asset. | ||
| /// @param amountOut Amount of output token sent by the ARM. | ||
| function _consumeSwapLiquidityLimit(BaseAssetConfig storage config, bool isBuySide, uint256 amountOut) internal { | ||
| uint256 remaining = isBuySide ? config.buyLiquidityRemaining : config.sellLiquidityRemaining; |
There was a problem hiding this comment.
Do we plan having config.buyLiquidityRemaining and config.sellLiquidityRemaining set to type(uint256).max? For example in a situation where we want to disable swap liquidity limits?
If so what do you think about returning earlier, to avoid writing new value?
| /// @notice Clear the legacy packed asset queue counter slot during the Model A upgrade. | ||
| /// @dev The reused slot previously packed `withdrawsQueued` in the low 128 bits and | ||
| /// `withdrawsClaimed` in the high 128 bits. It may be nonzero even when the old queue | ||
| /// is fully drained, so upgrade scripts should call this with `upgradeToAndCall`. |
There was a problem hiding this comment.
nit: comment upgrade scripts don't call this in "upgradeToAndCall"
| function migrateLegacyWithdrawQueue() external onlyOwner { | ||
| require(withdrawsQueuedShares == 0 && withdrawsClaimedShares == 0, "ARM: already migrated"); | ||
|
|
||
| uint256 packedLegacyQueue = reservedWithdrawLiquidity; | ||
| uint128 legacyQueued = uint128(packedLegacyQueue); | ||
| uint128 legacyClaimed = uint128(packedLegacyQueue >> 128); | ||
| require(legacyQueued == legacyClaimed, "ARM: legacy withdrawals pending"); | ||
|
|
||
| reservedWithdrawLiquidity = 0; | ||
| } |
There was a problem hiding this comment.
It is annoying to address this issue while also battling with the contract size limits. There is another way this could be done which is the cleanest, but operationally very annoying:
- upgrade the existing ARMs by adding the ability to pause (we would need to be mindful of deprecating this storage slot in this PR / future contract upgrade) and claim withdrawals on other user's behalf
- once upgrade lands, and withdrawals are claimable claim all of them
- upgrade the arms to this implementation where the withdrawal queues are drained. The migration call happens atomically in the deploy script's "upgradeToAndCall"
| address[] calldata path, | ||
| address to, | ||
| uint256 deadline | ||
| ) external virtual returns (uint256[] memory amounts) { |
There was a problem hiding this comment.
this one should be non-reentrant
| address[] calldata path, | ||
| address to, | ||
| uint256 deadline | ||
| ) external virtual returns (uint256[] memory amounts) { |
There was a problem hiding this comment.
this one should be non-reentrant
| amountOut = convertedAmountIn * config.buyPrice / PRICE_SCALE; | ||
|
|
||
| _accrueSwapFee(config.buyPrice, config.crossPrice, amountOut); | ||
| _ensureLiquidityAvailableForSwap(amountOut); |
There was a problem hiding this comment.
🟠 _ensureLiquidityAvailableForSwap can do an external 4626 call and _consumeSwapLiquidityLimit only later on changes the storage state. A maicious 4626 market could re-enter this function and cause damage.
Maybe move _consumeSwapLiquidityLimit(config, isBuySide, amountOut); into the if statement:
if (!isBuySide) {
// Trader sells liquidity asset and buys the base asset.
// The ARM buys the liquidity asset and sells the base asset.
// ARM prices the base sale at sellPrice.
uint256 convertedAmountIn = _convertToShares(config, amountIn);
// sellPrice is liquidity assets per base asset, so divide liquidity input by sellPrice
// to get the base output owed to the trader.
amountOut = convertedAmountIn * PRICE_SCALE / config.sellPrice;
_consumeSwapLiquidityLimit(config, isBuySide, amountOut);
} else {
revert("ARM: Invalid in token");
// Trader sells base asset and buys the liquidity asset.
// The ARM buys the base asset and sells the liquidity asset.
// ARM prices the base purchase at buyPrice.
uint256 convertedAmountIn = _convertToAssets(config, amountIn);
// buyPrice is liquidity assets per base asset. Since convertedAmountIn is the
// base input expressed in liquidity terms, multiply by buyPrice to get liquidity output.
amountOut = convertedAmountIn * config.buyPrice / PRICE_SCALE;
_accrueSwapFee(config.buyPrice, config.crossPrice, amountOut);
_consumeSwapLiquidityLimit(config, isBuySide, amountOut);
_ensureLiquidityAvailableForSwap(amountOut);
}
4 participants
Summary
Adds the next set of ARM upgrades ahead of the Ethena audit. This PR now includes active-market liquidity sourcing during swaps, discounted base-asset swap fees, per-price liquidity limits, multi-base-asset support, adapter-based redemption flows, and updated loss/fee accounting.
Merged changes
Key Changes
Withdraw From Active Market During Swaps
Swaps that need more liquidity than is currently held in the ARM can now pull the shortfall from the active ERC-4626 lending market.
This lets the ARM keep more liquidity deployed while still supporting swaps, provided the active market has enough withdrawable liquidity.
Discount-Based Swap Fees
Replaces the old performance fee on asset growth with fees accrued when the ARM buys base assets at a discount.
Fees are now tied to the executed buy price and accrued in liquidity-asset terms.
Per-Price Liquidity Limits
Buy and sell liquidity limits are tracked per configured price.
This lets operators cap how much liquidity can be consumed at the current buy/sell prices before prices are refreshed.
Multi-Base-Asset ARM
Folds reusable multi-base logic into
AbstractARM, allowing a single ARM to support multiple base assets against one liquidity asset.Examples:
stETH,wstETHeETH,weETHsUSDeOETH,wOETHEach base asset has its own config:
Asset Adapter Redemption Flow
Protocol-specific redemption logic has moved out of the ARM and into per-asset adapters.
Adapters now own details such as:
The ARM only tracks generic pending redeem accounting in liquidity-asset terms.
LP Loss Accounting
Updates LP redeem accounting so losses are shared pro-rata between redeemers and remaining LPs instead of allowing one side to absorb the full impact.
If assets per share falls after a redeem request is created, the claim uses the lower claim-time value.
Swap Fee Uses Cross Price
Swap fee accounting now calculates discount against the configured cross price rather than par value.
This aligns fee accrual with the ARM’s valuation price for the base asset.
Deployment, Task, and Test Updates
Includes updated deploy/upgrade scripts, ABIs, Hardhat tasks, diagrams, fork tests, smoke tests, invariant tests, and upgrade guard tests for the new architecture.
Gas Comparisons
Lido ARM
110,041-145,926+35,885(+32.61%)573,729+463,688(+421.38%)Ethena ARM
92,525-137,573+45,048(+48.69%)262,190+169,665(+183.37%)Testing