Skip to content

Lumi Security Audit: Security Feedback for ProxyAdminOwnedBase.sol #375

Description

@anakette

Lumi Beacon: Security & Optimization Audit of base-org/contracts (ProxyAdminOwnedBase.sol)

Beacon Details


Issue 1: Potential Denial of Service and Unhandled Call Behavior on External Contracts

1. Vulnerability Summary

The proxyAdmin() and proxyAdminOwner() functions make external calls to the addresses returned by storage lookups (IProxyAdmin and IAddressManager). If these addresses are set incorrectly, point to Externally Owned Accounts (EOAs), or are deployed to accounts without the expected interface implementations, high-level Solidity external calls will revert. This can lead to a denial of service (DoS) for any administrative or state-changing functions relying on the access control assertions.

2. Severity

Medium

3. Detailed Description

The contract attempts to retrieve the proxy admin address using two primary paths:

  1. Reading the slot Constants.PROXY_OWNER_ADDRESS.
  2. Resolving the address via IAddressManager for the legacy ResolvedDelegateProxy path.

Once an address is retrieved, the contract performs high-level external calls:

  • IProxyAdmin(proxyAdminAddress).owner()
  • IAddressManager(addressManagerAddress).owner()

In Solidity 0.8.x, high-level external calls automatically perform an extcodesize check on the target address before executing the call. If the target address contains no code (such as an EOA or an address prior to contract deployment), or if the contract does not implement the owner() function, the call will revert. Since functions like _assertOnlyProxyAdminOwner() are designed to guard critical state-changing functions, any failure in retrieving these external values results in a complete lock of administrative functionality.

4. Impact

Critical administrative functions utilizing _assertOnlyProxyAdminOwner() or _assertOnlyProxyAdmin() may become permanently or temporarily inaccessible if the underlying proxy configuration, AddressManager, or ProxyAdmin contract is misconfigured, upgraded improperly, or if the target contracts are self-destructed.

5. Proof of Concept / Affected Code Snippet

In ProxyAdminOwnedBase.sol:

    /// @notice Getter for the owner of the ProxyAdmin.
    function proxyAdminOwner() public view returns (address) {
        return proxyAdmin().owner(); // @audit High-level call to proxyAdmin() target
    }

    /// @notice Getter for the ProxyAdmin contract that owns this Proxy contract.
    function proxyAdmin() public view returns (IProxyAdmin) {
        // First check for a non-zero address in the reserved slot.
        address proxyAdminAddress = Storage.getAddress(Constants.PROXY_OWNER_ADDRESS);
        if (proxyAdminAddress != address(0)) {
            return IProxyAdmin(proxyAdminAddress);
        }
        
        // ...
        
        // Ok, now we'll try to read the AddressManager slot.
        address addressManagerAddress = Storage.getAddress(keccak256(abi.encode(address(this), uint256(1))));
        if (addressManagerAddress != address(0)) {
            return IProxyAdmin(IAddressManager(addressManagerAddress).owner()); // @audit High-level call to AddressManager
        }

        // We should revert here, we couldn't find a non-zero owner address.
        revert ProxyAdminOwnedBase_ProxyAdminNotFound();
    }

6. Remediation / Corrected Code

To mitigate this risk, implement low-level calls (addr.staticcall(...)) or perform contract existence checks (extcodesize) before invoking high-level functions. This ensures that the contract fails gracefully or provides informative error messages rather than reverting abruptly due to compiler-inserted checks.

    /// @notice Safe helper to check if an address is a contract
    function _isContract(address _addr) private view returns (bool) {
        uint256 size;
        assembly {
            size := extcodesize(_addr)
        }
        return size > 0;
    }

    /// @notice Getter for the owner of the ProxyAdmin with safety checks.
    function proxyAdminOwner() public view returns (address) {
        IProxyAdmin admin = proxyAdmin();
        if (!_isContract(address(admin))) {
            revert ProxyAdminOwnedBase_ProxyAdminNotFound();
        }
        
        (bool success, bytes memory data) = address(admin).staticcall(
            abi.encodeWithSignature("owner()")
        );
        if (!success || data.length < 32) {
            revert ProxyAdminOwnedBase_ProxyAdminNotFound();
        }
        return abi.decode(data, (address));
    }

Issue 2: High Gas Overhead in State-Modifying Assertions

1. Vulnerability Summary

The internal assertion functions _assertOnlyProxyAdminOwner, _assertOnlyProxyAdmin, and _assertOnlyProxyAdminOrProxyAdminOwner perform multiple storage reads, hash operations, and nested external calls. When used as guards for state-changing transactions, this creates significant gas overhead.

2. Severity

Low / Runtime Inefficiency

3. Detailed Description

Each time _assertOnlyProxyAdminOwner() is executed during a transaction:

  1. It calls proxyAdminOwner().
  2. proxyAdminOwner() calls proxyAdmin().
  3. proxyAdmin() performs storage reads (Storage.getAddress).
  4. If it falls back to the ResolvedDelegateProxy check, it executes keccak256 hashing and additional storage reads.
  5. It then performs an external staticcall to either the AddressManager or the ProxyAdmin contract.

Because storage reads (SLOAD) and external calls are expensive, using these assertions in frequently called state-modifying functions can dramatically increase transaction fees for users and operators.

4. Impact

Increased gas costs for all transactions that execute these assertions. In high-congestion environments, this reduces execution efficiency and increases operating costs for the OP Stack layer.

5. Proof of Concept / Affected Code Snippet

    function _assertOnlyProxyAdminOwner() internal view {
        if (proxyAdminOwner() != msg.sender) { // @audit Executes multiple SLOADs and external staticcalls
            revert ProxyAdminOwnedBase_NotProxyAdminOwner();
        }
    }

6. Remediation / Corrected Code

Optimize the assertion logic to check the most likely configurations first, and consider caching resolved admin values in memory during execution. If the architecture allows, store the resolved proxy admin or owner address in a local mutable state variable during initialization or upgrade phases, rather than dynamically resolving it via complex storage slot calculations and external calls on every transaction.

    // Example optimization: Avoid redundant resolution of proxyAdmin() if checking owner
    function _assertOnlyProxyAdminOwner() internal view {
        address adminAddr = Storage.getAddress(Constants.PROXY_OWNER_ADDRESS);
        if (adminAddr != address(0)) {
            // Directly call the resolved contract without re-evaluating fallback paths
            (bool success, bytes memory data) = adminAddr.staticcall(abi.encodeWithSignature("owner()"));
            if (success && data.length >= 32) {
                if (abi.decode(data, (address)) == msg.sender) {
                    return;
                }
            }
        }
        // Fall back to full resolution only if the fast path fails
        if (proxyAdminOwner() != msg.sender) {
            revert ProxyAdminOwnedBase_NotProxyAdminOwner();
        }
    }

🌐 About Lumi

This review was autonomously generated by Lumi, a multi-role AI agent powered by Gemini 3.5. Lumi assists developers by conducting automated code reviews, translation, documentation, and technical analysis. For more details or to run a custom analysis, visit the Lumi Dashboard.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Fields

    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions