Delegatecall Remediation

Overview

Related Detector: Delegatecall

delegatecall executes code from another contract in the caller’s storage context. The primary risks are: (1) user-controlled call targets enabling arbitrary code execution, and (2) storage slot collisions between proxy administrative variables and implementation state variables. Both must be addressed.

Recommended Fix

Before (Vulnerable)

contract VulnerableProxy {
    address public implementation; // slot 0

    // VULNERABLE: target comes from user input — attacker passes malicious contract
    function execute(address target, bytes calldata data) external {
        (bool success, ) = target.delegatecall(data);
        require(success);
    }
}

After (Fixed)

import "@audited/proxy/ERC1967/ERC1967Proxy.sol";
import "@audited/proxy/utils/UUPSUpgradeable.sol";
import "@audited/access/OwnableUpgradeable.sol";

// FIXED: use battle-tested proxy pattern with EIP-1967 storage slots
// and governance-controlled upgrade authorization
contract SafeImplementation is UUPSUpgradeable, OwnableUpgradeable {
    // State variables start at slot 0 — no collision because proxy uses
    // EIP-1967 pseudo-random high slots for implementation and admin
    uint256 public totalSupply;
    mapping(address => uint256) public balances;

    function initialize(address initialOwner) public initializer {
        __Ownable_init(initialOwner);
        __UUPSUpgradeable_init();
    }

    // Only owner can upgrade — all other callers are rejected
    function _authorizeUpgrade(address newImplementation) internal override onlyOwner {}
}

// Deploy with:
// ERC1967Proxy proxy = new ERC1967Proxy(
//     address(implementation),
//     abi.encodeCall(SafeImplementation.initialize, (owner))
// );

Alternative Mitigations

1. Allowlist Valid Implementation Addresses

contract AllowlistedProxy {
    // Implementation slot: keccak256("eip1967.proxy.implementation") - 1
    bytes32 private constant _IMPL_SLOT =
        0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc;

    mapping(address => bool) public allowedImplementations;
    address public proxyAdmin;

    modifier onlyAdmin() {
        require(msg.sender == proxyAdmin, "Not admin");
        _;
    }

    function allowImplementation(address impl) external onlyAdmin {
        allowedImplementations[impl] = true;
    }

    function upgradeTo(address newImpl) external onlyAdmin {
        require(allowedImplementations[newImpl], "Implementation not allowlisted");
        assembly {
            sstore(_IMPL_SLOT, newImpl)
        }
    }

    fallback() external payable {
        address impl;
        assembly { impl := sload(_IMPL_SLOT) }
        assembly {
            calldatacopy(0, 0, calldatasize())
            let result := delegatecall(gas(), impl, 0, calldatasize(), 0, 0)
            returndatacopy(0, 0, returndatasize())
            switch result
            case 0 { revert(0, returndatasize()) }
            default { return(0, returndatasize()) }
        }
    }
}

2. Transparent Proxy Pattern

import "@audited/proxy/transparent/TransparentUpgradeableProxy.sol";
import "@audited/proxy/transparent/ProxyAdmin.sol";

// ProxyAdmin handles upgrade authorization — admin calls go to proxy,
// user calls go to implementation (no function selector clash)
ProxyAdmin admin = new ProxyAdmin(owner);
TransparentUpgradeableProxy proxy = new TransparentUpgradeableProxy(
    address(implementation),
    address(admin),
    initData
);

3. Storage Gap Pattern for Future-Safe Upgrades

// Reserve storage gap to prevent collision when adding new variables in upgrades
abstract contract StorageV1 {
    uint256 public value;
    address public owner;

    // Reserve 48 slots for future V1 variables — total contract uses ≤50 slots
    uint256[48] private __gap;
}

// V2 adds new variable AFTER the gap — no collision with V1 layout
abstract contract StorageV2 is StorageV1 {
    uint256 public newVariable; // Goes into the next available slot (50)
}

Common Mistakes

Mistake 1: Uninitialized Implementation Contract

// WRONG: leaving the implementation contract uninitialized allows anyone
// to call initialize() and take ownership
contract MyImplementation is UUPSUpgradeable {
    address public owner;

    function initialize(address _owner) public initializer {
        owner = _owner;
    }
}

// WRONG: deploying implementation without calling initialize
// Anyone can call initialize() on the bare implementation contract
MyImplementation implementation = new MyImplementation();
// Should immediately call: implementation.initialize(owner) or disable initializers

Fix: Call _disableInitializers() in the implementation constructor:

constructor() {
    _disableInitializers(); // Prevents anyone from calling initialize() directly
}

Mistake 2: Selfdestruct in Implementation

// WRONG: if the implementation selfdestructs, the proxy is permanently bricked
function destroy() external onlyOwner {
    selfdestruct(payable(owner)); // Destroys implementation — proxy now points to dead code
}

Never put selfdestruct in an upgradeable implementation contract.

Mistake 3: Storage Collision from Inheritance Ordering

// WRONG: multiple inheritance where both parents use slot 0
contract A { uint256 public value; } // slot 0
contract B { uint256 public counter; } // slot 0

contract C is A, B {
    // A.value and B.counter both map to slot 0 — collision!
}