Execution Guard

_isGuardUpdateOperation uses a strict equality check (data.length == 36) to identify setGuard calls:

    function _isGuardUpdateOperation(address safe, address to, bytes memory data, Enum.Operation operation)
        internal
        pure
        returns (bool)
    {
        return to == safe && operation == Enum.Operation.Call && data.length == 36
            && _selector(data) == IGuardManager.setGuard.selector;
    }

After identifying the setGuard call, it is checked against a timelock that must have been set prior to execution. However, by appending a trailing byte to the calldata (i.e., 37 bytes instead of 36), the _isGuardUpdateOperation check will return false. The transaction then falls through to the executor whitelist path in checkTransaction and executes without timelock validation. The EVM's ABI decoder ignores the extra byte, so setGuard executes normally on the Safe.

This allows a guard rotation (including replacement with a malicious guard) in a single transaction with no on-chain observation window, breaking the system's core timelocked config update invariant. Anyone relying on the timelock delay for monitoring are left with no time to respond.

Replace the strict length check with a minimum bound in _isGuardUpdateOperation:

return to == safe && operation == Enum.Operation.Call && data.length >= 36
    && _selector(data) == IGuardManager.setGuard.selector;

The guard only reasons about the outer Safe transaction

In Safe v1.5.0, execTransaction verifies signatures, calls guard.checkTransaction() once for the outer tx, then executes the requested CALL or DELEGATECALL,

It does not re-run the guard on any inner calls emitted by code running under that outer delegatecall

Separately, Safe authorized modifier only checks msg.sender = address(this), and Safe own setGuard / enableModule are authorized selfcall functions.

That becomes fatal when a generic delegatecall target is on delegateCallAllowlist

most notably Safe standard batcher MultiSendCallOnly, which Safe SDK uses for batched onlyCalls transactions.

when MultiSendCallOnly is delegatecalled, it can emit arbitrary CALL, and it maps to = address(0) to address(this), the Safe itself.

So an outer tx that looks harmless to the guard (DelegateCall to an allowlisted target) can hide inner Safe self calls that the guard never classifies

The cleanest exploit is post onboarding module enablement, the guard blocks direct enableModule/disableModule, but it only checks the outer calldata shape

A whitelisted executor with valid Safe signatures can submit one outer tx

1- DelegateCall into allowlisted MultiSendCallOnly 2- inner subcall A: self-call enableModule(attackerModule) 3- inner subcall B: call attackerModule.attack() Once enabled, a Safe module has effectively unlimited execution power,

and execTransactionFromModule only checks that the caller is an enabled module plus any separate module guard

Consider making delegatecall unsupported for guarded Safes

if (operation == Enum.Operation.DelegateCall) {
    revert DelegateCallNotAllowed(to);
}

The guard enforces that a whitelisted executor must approve all operations through checkTransaction. The only exceptions are setGuard(address(0)) execution and scheduling the emergency disable. Everything else requires an explicitly whitelisted msgSender.

However, setFallbackHandler is not blocked by the guard. Once a fallback handler is set, the Safe's fallback() forwards unrecognized calls to the handler via call with msg.sender as the Safe. This creates a path for operations to be executed as the Safe without flowing through execTransaction and therefore without any executor whitelist validation.

For example, if the handler is set to the guard's address, anyone can call guard functions like scheduleTimelockedOperation or cancelTimelockedOperation as the Safe, bypassing the requirement that a whitelisted executor must approve the operation. Setting the handler requires a guarded transaction through a whitelisted executor, but once configured, the bypass persists until the handler is reset.

Block setFallbackHandler in checkTransaction alongside module management (also ensure it is not pre-set):

bytes4 selector = _selector(data);
return selector == IModuleManager.enableModule.selector
    || selector == IModuleManager.disableModule.selector
    || selector == IFallbackManager.setFallbackHandler.selector;

setSafeAllowed does not verify that a Safe has zero modules enabled before adding it to the allowlist. The execution guard explicitly blocks enableModule and disableModule operations during checkTransaction to prevent modules from being added. However, a Safe with a pre-existing module can be onboarded without restriction.

A module on a Safe executes through execTransactionFromModule, which does not invoke the transaction guard's checkTransaction hook, allowing the safe to bypass every protection the guard provides: executor whitelist, timelock enforcement, delegatecall restrictions, and module management blocks. Additionally, if a module does exist, the guard's block on disableModule prevents remediation through normal guarded execution, forcing the Safe to go through the emergency guard removal path first.

The project documentation acknowledges that pre-enabled modules remain an independent execution path outside the guard's scope. However, enforcing this on-chain via a module check in setSafeAllowed would be consistent with the existing _isSafeUsingThisGuard pattern, rather than relying on off-chain diligence.

When adding a Safe to the allowlist, verify it has no modules enabled:

if (flags[i]) {
    (address[] memory modules,) = ISafe(safes[i]).getModulesPaginated(address(0x1), 1);
    if (modules.length > 0) revert SafeHasModulesEnabled(safes[i]);
}

Ownership transfer via Ownable2Step has no timelock, despite being the most privileged operation in the system. A new owner can immediately call setTimelockDelay, setSafeListManager, and schedule timelocked operations for setWhitelist and setDelegateCallAllowlist. The two-step transfer (transferOwnership + acceptOwnership) requires explicit acceptance but provides no enforced delay, both calls can land in consecutive blocks.

This is inconsistent with the system's design, since executor whitelist and delegatecall allowlist updates are timelocked despite already requiring onlyOwner, specifically to give external observers a reaction window. Ownership transfer is more powerful than any of those operations yet has no such window.

Apply the existing timelock mechanism to ownership transfers. transferOwnership could be routed through scheduleTimelockedOperation so that external monitors have the same visibility window they get for whitelist and delegatecall updates.

EthenaSafeGuard inherits Ownable2Step for safe two-step ownership transfers, but does not override renounceOwnership() from the base Ownable contract. If invoked, ownership is set to address(0), permanently freezing all onlyOwner functions, including setting the executor whitelist, delegatecall allowlist, timelock delay, and safe list manager roles.

Override renounceOwnership to always revert.

checkTransaction names its unused parameters with a trailing underscore (i.e., value_, signatures_) and then references each as a no-op statement (value_;, signatures_;) to suppress unused variable compiler warnings. Both patterns become unnecessary when parameter names are simply omitted from the function signature, so using both is not needed and causes code clutter.

function checkTransaction(
    address to,
    uint256 value_,    // named with underscore
    bytes memory data,
    ...
) external override onlyAllowedSafe {
    value_;            // and also referenced as no-op
    safeTxGas_;
    ...
}

Consider omitting parameter names entirely for unused arguments and remove the no-op references:

function checkTransaction(
    address to,
    uint256,           // value
    bytes memory data,
    Enum.Operation operation,
    uint256,           // safeTxGas
    uint256,           // baseGas
    uint256,           // gasPrice
    address,           // gasToken
    address payable,   // refundReceiver
    bytes memory,      // signatures
    address msgSender
) external override onlyAllowedSafe {

scheduleTimelockedOperation unconditionally overwrites timelockUnlockAt if called again with the same operation parameters. If an operation is already scheduled, even within its valid execution window, a new call resets the unlockTimestamp to block.timestamp + timelockDelay. This could cause confusion for off-chain observers if, for example, the owner reduces timelockDelay via setTimelockDelay and then reschedules a pending operation with a shorter window than what was originally set.

Within scheduleTimelockedOperation, revert if a timelock entry already exists for the given operation.

When a Safe is removed from allowedSafes via setSafeAllowed, its pending timelock entries in timelockUnlockAt are not cleared. If the Safe is re-onboarded within the TIMELOCK_EXPIRY window (7 days), previously scheduled operations can be executed immediately without waiting through a new delay period.

Document that re-onboarding a recently delisted Safe may carry stale timelock entries, or consider cancelling any pending timelocked operations as part of the offboarding process.

The constructor sets timelockDelay = DEFAULT_TIMELOCK_DELAY but does not emit a TimelockDelayUpdated event. Every other state change in the constructor emits its corresponding event (SafeListManagerUpdated, ExecutorWhitelistUpdated, SafeAllowedUpdated, DelegateCallTargetUpdated). Anyone relying on events to track the timelock delay would not capture the initial value.

Emit TimelockDelayUpdated in the constructor:

timelockDelay = DEFAULT_TIMELOCK_DELAY;
emit TimelockDelayUpdated(0, DEFAULT_TIMELOCK_DELAY);

_isSafeUsingThisGuard reverts unconditionally if the getStorageAt call fails or returns unexpected data. If a Safe's guard storage becomes unreadable for any reason, the manager cannot remove it from the allowlist, and the Safe becomes permanently stuck in allowedSafes.

function _isSafeUsingThisGuard(address safe) internal view returns (bool) {
    try IStorageAccessible(safe).getStorageAt(SAFE_GUARD_STORAGE_SLOT, 1) returns (bytes memory rawGuardWord) {
        if (rawGuardWord.length != 32) revert SafeGuardStatusUnavailable(safe);
        address configuredGuard = abi.decode(rawGuardWord, (address));
        return configuredGuard == address(this);
    } catch {
        revert SafeGuardStatusUnavailable(safe);  // blocks delist
    }
}

The project documentation acknowledges the fail-closed behavior as a safety tradeoff, preferring temporary delist unavailability over unsafe delisting. However, if a Safe's guard storage becomes permanently unreadable, there is no override path. The Safe remains in allowedSafes indefinitely with no mechanism to remove it.

If guard status cannot be determined, allow removal rather than blocking it. Alternatively, consider providing an owner-only emergency delist that bypasses the guard status check as a last resort.

EthenaSafeGuard does not implement a fallback() function. If the Safe is upgraded to a version that changes the guard interface (e.g., different checkTransaction signature), calls to the guard would revert. Since the emergency disable path (setGuard(address(0))) also flows through checkTransaction, the Safe could become permanently locked. Other guard implementations (Gnosis ScopeGuard, Yearn StealthSafeGuard) include a non-reverting fallback specifically to prevent this scenario:

fallback() external {
    // We don't revert on fallback to avoid issues in case of a Safe upgrade
    // E.g. The expected check method might change and then the Safe would be locked.
}

Consider adding a non-reverting fallback() to avoid permanently locking guarded Safes in the event of a Safe interface upgrade.