Access Control Pitfalls

Access-control bugs remain the single largest source of catastrophic losses in production smart contracts. Most of them are not subtle — they are missing modifiers, footgun primitives, or single-key authorities masquerading as decentralized governance. This page enumerates the recurring patterns.

Authorization Through tx.origin

tx.origin returns the original externally-owned account that initiated the transaction chain, regardless of how many contract hops have intervened. Using it for authorization is broken by design: any contract the victim is induced to call can re-enter the protected function as msg.sender == ContractA, while tx.origin == victim.

// VULNERABLE
function withdraw(address payable to) external {
    require(tx.origin == owner);          // phishable
    to.transfer(address(this).balance);
}

The attacker deploys a malicious contract, persuades the owner to call any function on it (e.g. a fake airdrop claim), and inside that call invokes withdraw. The check passes because tx.origin is still the owner.

Remediation. Always use msg.sender for caller authorization. tx.origin has legitimate uses only in extremely narrow cases — most commonly disallowing contract callers (require(tx.origin == msg.sender)) — and even that pattern is discouraged because it conflicts with account abstraction (EIP-4337, EIP-7702).

Default Visibility

Pre-0.5.0 Solidity defaulted public functions to public visibility and state variables to internal. The compiler now requires explicit visibility, but two patterns still produce bugs:

  • Inherited functions that were intended to be internal but were declared without modifier in a library or base contract written for an older compiler.
  • State variables marked public by reflex, exposing automatic getters for fields the protocol assumed were private (admin addresses, fee recipients, internal accounting that callers were not supposed to learn until a state transition completed).

Remediation. Make visibility explicit on every function and state variable. Mark anything not meant for external callers as internal or private. Remember that private only blocks Solidity-level access — see §4.18.3 on unencrypted on-chain data.

Unprotected Ether Withdrawal

Any function that moves the contract's ether balance must be gated. The pattern is shockingly common in early-stage code:

function withdraw(uint256 amount) external {
    payable(msg.sender).transfer(amount);   // no access control
}

The Parity multisig wallet kill in 2017 (see §4.18.7 and §4.16.2) was effectively this pattern at the library level.

Remediation. Every state-changing function should answer the question "who is allowed to call this?" before any other concern. Use onlyOwner, role-based modifiers, or explicit require checks. Static analyzers (Slither's arbitrary-send-eth and suicidal detectors) catch most cases.

Unprotected SELFDESTRUCT

A reachable selfdestruct(addr) lets the caller delete the contract's code and forward its entire balance to addr. Combined with a missing access-control check, the contract becomes a one-call wipeout.

// VULNERABLE
function kill() external {
    selfdestruct(payable(msg.sender));
}

Note: the Cancun upgrade (EIP-6780) restricted SELFDESTRUCT to the same transaction as contract creation — outside that window it now only transfers the balance, not the code. Pre-Cancun code paths and historical exploits remain relevant for any contract deployed before then or running on chains that have not adopted the change.

Remediation. Avoid selfdestruct entirely in new code. If it must exist (typically for proxy or migration patterns), gate it behind the strongest available authority and verify the deployment-chain semantics.

Missed Modifier

The classic "one function in the family without the modifier" bug. A contract defines onlyOwner and applies it to nineteen functions; the twentieth — added during a feature merge — silently lacks it.

modifier onlyOwner() { require(msg.sender == owner); _; }

function setFee(uint256 f) external onlyOwner { fee = f; }
function setTreasury(address t) external onlyOwner { treasury = t; }
function setOracle(address o) external { oracle = o; }   // missed modifier

Detection. Grep for external|public functions that change state and cross-reference against the modifier list. Slither's access-control detector and Aderyn's modifier checks help. The reviewer's habit worth building is to scan the consistency of modifier application across every setter and admin function in a contract before reading any logic.

Incorrect Modifier Names

Closely related: a modifier named onlyOwer (typo) compiles, evaluates to nothing if it accepts no body and is unused, or — worse — silently masks an intended check.

modifier onyOwner() {           // typo
    if (msg.sender == owner) _; // missing else → silent no-op
}

function rugpull() external onyOwner {
    payable(msg.sender).transfer(address(this).balance);
}

The contract compiles. The function runs for any caller because the modifier returns without reverting when the condition fails.

Remediation. Every modifier must require (or revert) on the failure path, never relying on a missing _; branch. Lint for modifiers whose body is only an if (...) _; pattern. Naming-convention enforcement (lowercase-camel, consistent prefix only) makes typo modifiers visually obvious in review.

Overpowered Roles

A single externally-owned account controlling all admin functions is not access control — it is a centralization risk dressed in role decoration. Patterns that recur:

  • Single owner with the ability to pause, upgrade, sweep funds, change oracles, and modify fees.
  • Multisigs of size 2-of-3 where two signers belong to the same person or company.
  • Timelocks with delays shorter than any plausible exit window for users.
  • Role admin equal to the role itself, so any role-holder can grant or revoke.
  • Hidden god functions — sweepers, emergency rescue, "migrate" — added late in development and not documented.

Audit treatment. These are typically reported as Centralization or Owner-privilege findings rather than vulnerabilities. The audit should enumerate every privileged action, identify who holds the keys, and confirm that protections (timelock, multisig threshold, user exit window) match the protocol's stated decentralization claims. See also §4.12.4 Malicious Upgrades.

Unsafe Ownership Transfer

Single-step ownership transfer is unsafe because a typo in the new owner address is unrecoverable.

// UNSAFE
function transferOwnership(address newOwner) external onlyOwner {
    owner = newOwner;
}

If newOwner is a wrong-address typo, an EOA the team does not control, or a contract that cannot interact with the role, the protocol is effectively bricked at the admin layer.

Remediation. Use a two-step pattern such as OpenZeppelin's Ownable2Step: the outgoing owner nominates a successor, and the successor must acceptOwnership() from the new address before the role transfers. Apply the same pattern to every privileged role transfer, not just owner.

function transferOwnership(address newOwner) external onlyOwner {
    pendingOwner = newOwner;
}

function acceptOwnership() external {
    require(msg.sender == pendingOwner);
    owner = pendingOwner;
    pendingOwner = address(0);
}

Auditor Checklist

  • No tx.origin used for authorization.
  • Every function and state variable has explicit visibility.
  • Every state-changing external/public function is gated by a modifier or explicit caller check.
  • No reachable selfdestruct without strong authority; deployment-chain semantics confirmed.
  • Modifier set is consistent across all admin/setter functions; no typo-modifiers.
  • Every modifier reverts on the failure path; no silent if (...) _; patterns.
  • Privileged actions enumerated; key holders documented; protections match stated claims.
  • Ownership and role transfers use a two-step accept pattern.
  • Static analyzers (Slither, Aderyn) report no access-control findings.