Ethena Pay

When direct mode is active, directEthWithdrawal() and directTokenWithdrawal() require a single signer via LibAuth.verifyOwnerSignature(). If any one signer is compromised, the attacker can drain funds without executor co‑signing. The 1‑hour activation delay only helps if direct mode is not activated.

All actions that help the user - removing owner, exiting direct mode - can be cancelled by the compromised signer. This leaves the real owners of the wallet with only one possible mitigation - race with the malicious actor to withdraw all funds, which in most of the time results in the malicious actor taking the assets.

This issue is also present when direct mode is not turned on because the executor will most probably still sign a withdrawal message and let the compromised key withdraw funds.

Consider requiring min(2, totalSignerCount()) of the owners to have signed when funds are withdrawn in direct mode.

One downside of this solution is that wallets that want to enter direct mode will be unable to be used for the next 24 hours if there is more than one signer, but the key for only one of them is usable.

Also rethink if the flow when direct mode is not turned on needs to be reworked, or additional authentication means need to be added to the executor sign path.

The wallet stores replay-protection nonces per signer, but the EIP-712 payloads do not include the signer identity whose nonce is being used. Instead, the contract hashes only the action parameters together with a bare nonce and deadline, then recovers the signer from the submitted signature and consumes signerNonces[signerKey]. This affects all owner signature types supported by the wallet, including ECDSA, EIP-1271, and WebAuthn, because the signer is selected only after verifying the same digest. This creates a mismatch between what is actually being authorized and which replay-protection state gets consumed. The same payload can be authorized by any registered signer, and the contract will accept it as long as that signer’s current nonce matches the provided numeric nonce. A malicious signer can therefore advance their own nonce until it matches another signer’s nonce, then submit the same action payload using their own valid signature. This is especially dangerous for withdrawal flows. In executor withdrawal paths, the same executor signature can also be reused, since the executor signs the same digest and that digest is not tied to a specific owner signer. As a result, a single withdrawal authorization payload can be reused across multiple signer nonce lanes instead of being single-use for the wallet.

Bind the signer’s identity to every owner-signed EIP-712 message that uses per-signer nonces, and check that the recovered signer matches the one included in the signed data. For actions that should only be usable once for the whole wallet, such as withdrawals, consider using a single global wallet nonce.

The executor has a daily limit of tokens it can transfer on behalf of the wallet by calling spendDailyAllowance() and spendDailyAllowanceEth(). It's expected for most wallets to have given the executor full allowance because the default value 0 is interpreted as unlimited spending.

The case where the executor is compromised can be countered by allowing the user to opt out of the executor, which can be done by entering direct mode. However, there is a 1 hour window that must pass before the direct mode takes effect. The allowance for the executor becomes irrevocable because instantSetAllowance() requires a signature both from a valid owner and the executor itself.

When the protocol team notices the executor is compromised, they can call UserProxyFactoryBeacon.globalPause() and block the spending, but this may not be sufficient, because the compromised executor can atomically drain all wallets that have given permission to it.

Consider implementing a two step spending mechanism, where the executor makes a request and the actual funds transfer happens after a small delay, for example 5 minutes.

In this period between request and execution, the protocol team has enough time to call globalPause() and schedule executor change. The payment processing should revert if the factory is paused or the current executor is not the one that initiated the request. The downside of this approach is that any unprocessed legitimate payments that were initiated in the waiting period before the protocol was paused, will probably be lost. It's actually the same right now, but with a smaller time period.

Notice that in case the factory owner goes rogue as well, there is nothing that the user can do to prevent their funds from being stolen.

LibAuth.initiatePendingPasskey() and LibAuth.initiateAddOwner() revert when the current signer count hits MAX_PASSKEYS / MAX_OWNERS. This check blocks signer replacement for additional 24 hours when the length of the current array is at the max.

For example:

• There are 10 passkeys
• Passkey A should be replaced with Passkey B
• The admins of the wallet cannot initiate both the addition and the removal at the same time. They have to wait 24 hours for Passkey A to be removed and then wait another 24 hours to add Passkey B.

Because the same cap check is already present in executePendingPasskey()/executeAddOwner(), you can either remove it from initiatePendingPasskey()/initiateAddOwner() or relax it to being applied only if the pending removals are empty.

initiateRemovePasskey authorizes passkey removal by signing only the passkey array index, instead of signing a stable identifier of the target passkey. This is unsafe because the passkey array is mutable, so the passkey stored at a given index can change between the moment the signature is created and the moment the removal is initiated. For example, assume the wallet passkeys are [A, B, C], and a signer creates a signature to remove the passkey at index 1, intending to remove B. Before that request is submitted, another signer removes B or otherwise changes the array ordering. As a result, index 1 may now point to a different passkey. The old signature will still be valid because it only commits to the numeric index, but it may now remove a different passkey than the signer originally intended.

Bind passkey removal signatures to an immutable identifier of the target passkey rather than its current position in the array.

getPendingDirectMode and getPendingExitDirectMode both document that activatesAt should return 0 when nothing is pending. However, after direct mode activates, getPendingDirectMode still returns the historical directModeAt timestamp instead of 0. The same applies to getPendingExitDirectMode, after an exit completes, it returns the stale exitDirectModeAt value. In both cases isPending correctly returns false, but activatesAt is non-zero despite nothing being pending, contradicting the documented behavior.

In addition, after a complete enter→exit cycle, directModeAt is still non-zero because initiateExitDirectMode never clears it. This is intentional to allow the cooldown process to take place. However, this causes getPendingDirectMode to return a stale activatesAt value even though direct mode is no longer active, which could mislead users/integrators into misinterpreting the wallet's direct mode state.

Return 0 for activatesAt when nothing is pending for both getPendingDirectMode and getPendingExitDirectMode:

function getPendingDirectMode() internal view returns (uint64 activatesAt, bool isPending) {
    uint64 timestamp = directModeStorage().directModeAt;
    isPending = timestamp > 0 && block.timestamp < timestamp;
    return (isPending ? timestamp : 0, isPending);
}

The direct mode cancel messages are not tied to a specific pending directModeAt or exitDirectModeAt value. Both cancel payloads sign only a bare nonce and deadline. When a cancel signature is later submitted, the contract only checks that some pending action currently exists. It does not verify that the signature was created for the current pending timestamp: Because of this, an old unused cancel signature can be replayed against a different future direct-mode action, as long as the signer’s nonce is still unchanged and the deadline has not expired. For example, a signer may sign a cancellation message for one pending direct-mode entry, but if it is not submitted at that time, it can remain valid and later be used against a different pending direct-mode action. If a new entry or exit is initiated before that signer uses the same nonce for another action, the stale signature can still be submitted and will cancel the new pending state even though it was not created for that specific case.

Consider including the current directModeAt and exitDirectModeAt value in the signed cancel payload so each cancel signature is tied to a specific pending action.

RecoveryFacet grants the ability to add a new signer to the recovery owners. A recovery is initiated and once the 10-days waiting period passes, it can be completed by calling executeRecovery(). If the signer to be added is EOA/smart contract, the code proceeds executing addOwnerDirect().

There exists the check that reverts the transaction if the owner is the same as the executor in the factory.

        // Re-check executor at execution time (executor may have changed since recovery initiation)
        address currentExecutor = IUserProxyFactory(LibFactory.getFactory()).executor();
        if (_owner == currentExecutor) revert ExecutorCannotBeOwner();

This makes the wallet dependent on the factory, even in direct mode. A compromised factory owner can intercept every recovery request and change the executor to the owner that's being added, rendering the recovery mechanism useless for that wallet, unless a passkey is being added.

Either remove the validation check and allow even the executor to be added or clearly document the risk and inform users that they must use passkeys in this case.

Once a wallet sets a custom recovery admin list via setRecoveryConfig(), rs.hasCustomConfig is set to true and there is no supported way to revert to factory default recovery admins while staying opted‑in. The only available option is optOutRecovery(), which disables recovery entirely. This prevents wallets from adopting future factory default changes.

Allow reverting to defaults by accepting admins.length == 0 in setRecoveryConfig() as a “use factory defaults” signal, clearing customRecoveryAdmins and setting hasCustomConfig = false.

There is inconsistency between the naming of the functions that perform actions related to owner addition vs the functions performing actions related to passkey addition in LibAuth:

• initiateAddOwner() vs initiatePendingPasskey()
• executeAddOwner() vs executePendingPasskey()
• cancelAddOwner() vs cancelPendingPasskey()

Compared to the functions for removal, where the naming is consistent:

• initiateRemoveOwner()/initiateRemovePasskey()
• executeRemoveOwner()/executeRemovePasskey()
• cancelRemoveOwner()/cancelRemovePasskey()

Consider making the naming consistent for add/remove.

The comment in LibReentrancyGuard says:

 *      The slot is derived using the ERC-7201 namespaced pattern:
 *      keccak256("ethenapay.storage.ReentrancyGuard") - 1, masked to align.

However, the ERC-7201 formula (https://eips.ethereum.org/EIPS/eip-7201#formula) hashes the id of the namespace twice before applying & to it.

keccak256(abi.encode(uint256(keccak256(bytes(id))) - 1)) & ~bytes32(uint256(0xff))

The REENTRANCY_SLOT is hashed only once and therefore it deviates from the actual ERC-7201 standard.

Consider hashing the namespace id twice.

In approveAddPasskey(), approveAddOwner(), and approveRemoveOwner(), the exists flag read from LibAuth.getPendingPasskey(), LibAuth.getPendingOwner(), and LibAuth.getPendingOwnerRemoval() is used to gate PasskeyAdditionExecuted, OwnerAdditionExecuted, and OwnerRemovalExecuted. However, LibAuth.executePendingPasskey(), LibAuth.executeAddOwner(), and LibAuth.executeRemoveOwner() all revert if the pending entry does not exist or is not ready, so reaching the emit implies exists was true. The conditional check adds minor gas and noise without changing behavior.

Remove the if (exists) guards and always emit PasskeyAdditionExecuted, OwnerAdditionExecuted, and OwnerRemovalExecuted after the respective LibAuth.execute*() call succeeds.

deployWalletWithBoth() accepts passkey coordinates (qx, qy) but the corresponding WalletDeployedWithBoth event does not emit them. This makes off-chain indexing and monitoring less convenient compared to WalletDeployedWithPasskey, which includes qx and qy. Consumers must parse calldata or query the deployed wallet state to recover the passkey data.

Include qx and qy in the WalletDeployedWithBoth event and emit them from deployWalletWithBoth() so the event is self-contained for indexers.

LibAllowance uses 0 (default value) to allow unlimited spending. Since there is no value that disallows the executor from spending a specific token, users who want to block executor access to a specific token must enter direct mode, which disables executor operations across all tokens. This means a user cannot block executor spending for one token while keeping daily limits active for others. Setting the token limit to 1 wei still technically allows spending, since it is not truly zero.

Reserve type(uint256).max for unlimited and treat 0 as "no spending permitted". Alternatively, consider adding a per-token bool disabled flag to the AllowanceStorage struct.

PasskeyRemovalInitiated emits only the original passkey index, while the pending removal state stores both the index and the passkey coordinates. This makes the event inconsistent with PasskeyAdditionInitiated, which emits qx and qy directly. Passkey removal is based on qx and qy, not the original index. Consequently, the event alone does not clearly identify which passkey is being removed, and off-chain consumers cannot determine it without querying contract state.

Emit qx and qy in PasskeyRemovalInitiated so the event fully identifies the passkey being removed.

UserProxyFactoryBeacon.scheduleDefaultRecoveryAdmins() reverts when admins.length == 0 and LibRecovery.validateAdmins() rejects address(0), so the factory default recovery admin list cannot be cleared to an empty set. This means removing defaults is only possible per wallet via optOutRecovery() rather than at the factory level.

If clearing factory defaults is intended, allow admins.length == 0 in scheduleDefaultRecoveryAdmins() to explicitly clear _defaultRecoveryAdmins, or add a dedicated clearDefaultRecoveryAdmins() flow. Otherwise, document that defaults are always non‑empty and that wallets must use optOutRecovery() to disable recovery.

RecoveryConfigChanged emits only adminCount, even though setRecoveryConfig applies a specific recovery admin set. A similar issue exists in the factory, where DefaultRecoveryAdminsScheduled and DefaultRecoveryAdminsExecuted emit only the number of admins rather than the actual admin array or its hash. As a result, an off-chain consumers cannot determine the exact recovery admin configuration from logs alone.

Consider emitting the full admin array in the event, or at least emit a hash of it, so off-chain consumers can easily identify which recovery admin set was configured from the logs.

When the configured dailyLimit() is 0 (unlimited), _checkAndUpdateDailyLimit() returns early and does not update dailySpending. Spends via spendDailyAllowance() and spendDailyAllowanceEth() still emit AllowanceSpent(), so offchain consumers cannot distinguish whether a spend was accounted toward a daily cap, while dailySpent()/dailyRemaining() stay unchanged. If a user later calls instantSetAllowance() to set a finite dailyLimit() the same day, the executor can spend the full new limit on top of earlier unlimited spends.

If the intended semantics are “same-day caps apply to all spending,” update _checkAndUpdateDailyLimit() to update dailySpending even when dailyLimit() is 0 (skip only the revert). If unlimited spending should remain unaccounted, emit a distinct event or extend AllowanceSpent() with an accounted flag or limit field so offchain systems can disambiguate.

The NatSpec for hashInstantSetAllowance() says nonce is the “current wallet nonce,” but the code consumes a per‑signer nonce via LibAuth._useCheckedNonce() in instantSetAllowance(). There is no global wallet nonce; nonces are tracked per signer across the system. This mismatch can mislead integrators.

Update the NatSpec to say the nonce is the current per‑signer nonce for replay protection, consistent with the rest of the codebase/docs.

AllowanceFacet.spendDailyAllowance() rejects amount == type(uint256).max because it compares amount to balanceOf() before transfer. There are some ERC20 tokens that accept uint256.max as "transfer the available balance".

There is no balance check in WithdrawalFacet.executorTokenWithdrawal() and WithdrawalFacet.directTokenWithdrawal() - they pass amount directly to safeTransfer() and emit ExecutorWithdrawal/DirectWithdrawal with the same amount. If this feature is used, the event will emit uint256.max, while the transfer value will be different.

Consider adding the balance check to WithdrawalFacet as well.

When adding a recovery admin via setRecoveryConfigor the factory's scheduleDefaultRecoveryAdmins, LibRecovery.validateAdmins checks for zero addresses and duplicates but does not check whether any admin is the current executor. The protocol prevents the executor from being an owner, but does not apply the same restriction to recovery admin configuration, which would give the executor more privilege than what is intended for that role.

Check against factory.executor() in validateAdmins.

instantSetAllowance uses enforceUserNotBlocked, which permits execution when the user is in direct mode even during a global pause. However, spendDailyAllowance and spendDailyAllowanceEth use enforceExecutorNotBlocked, which blocks spending during both global pause and direct mode. This means a user can change daily limits during a global pause via direct mode, but the executor cannot spend against those limits until both the pause lifts and direct mode is exited. This could mislead integrators into thinking the allowance is immediately in effect.

Document that allowances set during a global pause via direct mode only take effect for executor spending once the pause is lifted and direct mode is exited.

The protocol intends to support delayed settlement of user obligations, for example when the executor later deducts configured allowance for activity. However, the current settlement and allowance-spend flows are exposed only as single operations. If multiple obligations, or multiple user actions and their related obligations, must be settled on-chain, the executor has to submit them one by one. This increases gas overhead and operational friction compared to a batch-based design. At scale, the executor may be forced to submit a large number of separate transactions for actions that could otherwise be grouped together.

Consider adding support for batching transfer and allowance-spend operations, so multiple off-chain computed obligations can be settled in a single transaction when appropriate.

Executor settlement relies on spendDailyAllowance() / spendDailyAllowanceEth() executing after an offchain service is delivered. Even with a “smart executor” that tracks balances and state, several on‑chain conditions can still change between service delivery, transaction submission, and inclusion, causing executor pulls to revert. Examples include: direct mode activation (initiateDirectMode() → isDirectMode() becomes true) which makes LibDirectMode.enforceExecutorNotBlocked() revert; global pause (globalPaused()) which also blocks executor pulls; token behaviors like rebasing, fee‑on‑transfer, blacklists, or pausable hooks that can change IERC20.balanceOf() or make safeTransfer() revert after a pre‑check; and reorgs/ordering delays that invalidate recent state observations. These are inherent settlement/credit risks rather than protocol bugs, but they can cause valid executor pulls to fail even when allowances are unlimited.

Document this as an explicit trust/operational assumption and implement mitigations on the executor side: minimize time between service and spendDailyAllowance() / spendDailyAllowanceEth() submission, monitor pendingDirectMode() / isDirectMode() and globalPaused(), use token allowlists or token‑specific handling for rebasing/fee‑on‑transfer tokens, and consider prepayment/escrow or atomic “set + spend” flows for high‑value services.

The codebase uses ETH‑specific naming (for example ETH_TOKEN, UseSpendAllowanceEthForETH(), and spendDailyAllowanceEth()), and comments like dailyLimit() mention “ETH.” On Avalanche, this can confuse integrators and users, since the native token is not ETH. The functionality uses address(0) as the native token, but the naming implies Ethereum‑specific semantics.

Update comments, errors, and public‑facing terminology to say “native token” (or chain‑specific token like AVAX) instead of ETH, or document clearly that “ETH” is used as a generic alias for the native token across deployments.

executeAddOwner and addOwnerDirect check pending.owner == factory.executor() to enforce the invariant that the executor cannot also be an owner. However, neither checks against factory.pendingExecutor(). A wallet owner can call initiateAddOwner with the pending executor address during the 48h executor change delay. Since the address is not yet the active executor, it passes all checks. After the 24h signer cooldown, executeAddOwner re-checks against the current executor, which is still the old one. The owner addition succeeds. When the executor change finalizes at 48h, the address holds both the owner and executor roles, breaking the invariant.

Note that adding a pendingExecutor check on the wallet side would introduce a DoS vector: a compromised factory owner could block any wallet's owner additions by scheduling a pending executor change to match that wallet's pending owner address. Since there is no delay, it can immediately DoS the pending owner addition execution.

Document the ability for users to add a pending executor as an owner as a known constraint.

Recovery has a fixed 10-day timelock while facet updates have a 48h timelock. If a facet update that modifies or removes recovery-related selectors is executed during an active recovery period, executeRecovery will revert because the selector no longer resolves to a valid facet. The recovery must then be re-initiated after the facet is restored, adding at least another 10 days. Both scheduleFacetUpdate and executeFacetUpdate have onlyOwner modifier, so this is not an external attack vector, although it can pose as an operational risk during upgrades.

Document that pending recoveries should be checked before executing facet updates that affect recovery selectors. Alternatively, gate facet updates behind a check for active recoveries across affected wallets.

setRecoveryConfig and scheduleDefaultRecoveryAdmins function accept the recovery admin array as calldata, but both pass it to LibRecovery.validateAdmins, which takes address[] memory. As a result, Solidity copies the full array from calldata into memory before validation, even though the helper only reads the array and does not modify it. This introduces unnecessary gas overhead on every recovery config update and default admin scheduling call.

Change LibRecovery.validateAdmins to accept address[] calldata.

verifyExecutorSignature receives executorSig as bytes calldata but passes it to SignatureChecker.isValidSignatureNow which takes bytes memory, forcing an unnecessary calldata→memory copy for the signature on every call if the executor is an EOA. OpenZeppelin SignatureChecker.sol provides isValidSignatureNowCalldata which uses tryRecoverCalldata on the EOA path, avoiding the copy and saving gas. There is no difference if the executor is not an EOA, since the ERC-1271 path will always require calldata→memory conversion due to the external call, however in the case the executor is an EOA, the conversion is unnecessary and wastes gas.

Replace isValidSignatureNow with isValidSignatureNowCalldata in verifyExecutorSignature.

All privileged factory operations use a 48h timelock: beacon upgrades, facet updates, executor changes, and recovery admin changes. However, ownership transfer via Ownable2Step has no delay, the pending owner can accept the ownership transfer immediately. An ownership transfer to a compromised or incorrect address takes effect instantly with no monitoring window for users to react.

Consider adding a timelock to ownership transfer consistent with other admin operations, or document that ownership transfer is immediate (no delay between nomination and acceptance) and that users should monitor OwnershipTransferStarted events.

The NatSpec for addOwnerDirect() says it reverts with InvalidOwnerAddress() if the address is 0, but the implementation also reverts with the same error when _owner == address(this). This mismatch can confuse integrators about the actual validation rules.

Update the NatSpec to mention both conditions (address(0) and address(this))

As described on the OZ website (https://docs.openzeppelin.com/community-contracts/api/utils/cryptography?#WebAuthn), origin and rpid checks, as well as signature counter, extension outputs and attestation checks, are not performed.

It's assumed the authenticator will take care of the origin check. This increases the risk of phishing attacks if the authenticator fails to validate the origin.

Recheck if skipping each of the validations is fine.

In RecoveryFacet.setRecoveryConfig(), when called with an empty admins array (admins.length == 0), the function resets hasCustomConfig to false and deletes customRecoveryAdmins, but fails to set rs.optedOut = false.

A wallet owner who has previously opted out of recovery and later calls setRecoveryConfig() with an empty admins array — intending to revert to factory default recovery — will believe recovery is re-enabled. However, optedOut remains true, causing initiateRecovery() to revert via enforceNotOptedOut().

If the owner subsequently loses all signing keys, the wallet becomes permanently unrecoverable. No recovery admin can initiate recovery, and the owner has no keys to correct the misconfigured state. All funds in the wallet are locked forever.

Set optedOut = false when reverting back to default recovery admins.

The new signer parameter added to the functions in AuthFacet is not documented in the NatSpecs.

Add @param signer documentation to the NatSpec of each function.