Access Control Remediation

Overview

Related Detector: Access Control

Missing access controls allow unauthorized callers to execute privileged functions — draining funds, upgrading contracts, changing ownership, or modifying protocol parameters. The remedy is to always gate privileged operations with a verified caller check.

Recommended Fix

Before (Vulnerable)

contract VulnerableProtocol {
    address public treasury;
    mapping(address => bool) public whitelistedUsers;

    // No caller check — anyone can drain the treasury
    function withdrawAll() external {
        uint256 balance = address(this).balance;
        payable(msg.sender).transfer(balance);
    }

    // No caller check — anyone can whitelist themselves
    function addToWhitelist(address user) external {
        whitelistedUsers[user] = true;
    }
}

After (Fixed)

import "@audited/access/Ownable.sol";

contract SecureProtocol is Ownable {
    mapping(address => bool) public whitelistedUsers;

    constructor() Ownable(msg.sender) {}

    // Only owner can withdraw
    function withdrawAll() external onlyOwner {
        uint256 balance = address(this).balance;
        payable(owner()).transfer(balance);
    }

    // Only owner can whitelist — user requests go through governance
    function addToWhitelist(address user) external onlyOwner {
        whitelistedUsers[user] = true;
    }
}

The onlyOwner modifier performs the caller check: require(msg.sender == owner(), "Ownable: caller is not the owner"). This pattern is battle-tested and gas-efficient.

Alternative Mitigations

1. Role-Based Access Control (RBAC)

For protocols with multiple privilege levels, use OpenZeppelin’s AccessControl:

import "@audited/access/AccessControl.sol";

contract RBACProtocol is AccessControl {
    bytes32 public constant ADMIN_ROLE = keccak256("ADMIN_ROLE");
    bytes32 public constant OPERATOR_ROLE = keccak256("OPERATOR_ROLE");
    bytes32 public constant PAUSER_ROLE = keccak256("PAUSER_ROLE");

    constructor() {
        _grantRole(DEFAULT_ADMIN_ROLE, msg.sender);
        _grantRole(ADMIN_ROLE, msg.sender);
    }

    // Critical operations require ADMIN role
    function upgradeImplementation(address newImpl) external onlyRole(ADMIN_ROLE) {
        _upgrade(newImpl);
    }

    // Day-to-day operations can be delegated to OPERATOR role
    function processWithdrawal(address user, uint256 amount)
        external onlyRole(OPERATOR_ROLE)
    {
        _processWithdrawal(user, amount);
    }

    // Emergency pause can be called by PAUSER role
    function pause() external onlyRole(PAUSER_ROLE) {
        _pause();
    }
}

2. Time-locked Admin Actions

For irreversible operations, require a time delay to allow users to respond:

contract TimelockAdmin {
    address public pendingAdmin;
    uint256 public adminTransferTime;
    uint256 public constant TIMELOCK_DELAY = 2 days;

    address public admin;

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

    function proposeAdmin(address newAdmin) external onlyAdmin {
        pendingAdmin = newAdmin;
        adminTransferTime = block.timestamp + TIMELOCK_DELAY;
    }

    function acceptAdmin() external {
        require(msg.sender == pendingAdmin, "Not pending admin");
        require(block.timestamp >= adminTransferTime, "Timelock not expired");
        admin = pendingAdmin;
        pendingAdmin = address(0);
    }
}

3. Multi-Signature Authorization

For critical protocol operations, require multiple signers:

// Use Gnosis Safe or similar multi-sig as the owner
// All critical functions require N-of-M signatures from the Safe
contract SafeControlledProtocol {
    address public immutable multiSig; // Gnosis Safe address

    modifier onlyMultiSig() {
        require(msg.sender == multiSig, "Not multi-sig");
        _;
    }

    function upgradeImplementation(address newImpl) external onlyMultiSig {
        _upgrade(newImpl);
    }
}

Common Mistakes

Mistake 1: Using `tx.origin` instead of `msg.sender`

// WRONG: tx.origin can be spoofed via phishing attacks
function withdraw() external {
    require(tx.origin == owner, "Not owner"); // Use msg.sender instead!
    payable(msg.sender).transfer(address(this).balance);
}

tx.origin is the original transaction initiator, not the immediate caller. A phishing contract can trick the owner into calling it, which then calls this function — tx.origin is the owner but msg.sender is the attacker.

Mistake 2: Forgetting to protect initialization functions

// VULNERABLE: initialize() can be called by anyone, multiple times
contract UpgradeableProxy {
    address public implementation;
    address public admin;
    bool private initialized;

    function initialize(address impl, address _admin) external {
        // Missing: require(!initialized) and require(msg.sender == factory)
        implementation = impl;
        admin = _admin;
        initialized = true;
    }
}

Mistake 3: Incorrect modifier implementation

// WRONG: modifier does not revert — it just checks and continues
modifier onlyOwner() {
    if (msg.sender != owner) {
        // Missing: revert or require
    }
    _;
}

Always use require or revert in modifiers.