Access control failures: the simplest bug class, and the most expensive

The simplest class, and the most expensive

Access control failures are trivial to describe: a function that should be restricted is not. There is no clever state interleaving, no economic modelling, no cryptography. And yet this class accounts for an enormous share of total value lost in this industry.

The reason is that access control is the one thing a reviewer's eye skips. A modifier is a short token at the end of a function signature. When a codebase has two hundred functions and a hundred and ninety of them are correctly guarded, the ten that are not do not look wrong — they look like the others.

This is why we review access control as a matrix rather than as a code read: build the full table of roles against functions, fill it in from the code, and then check it against what the team believes the table should be. The disagreements are the findings.

Missing modifiers and the enumeration that finds them

The base case remains common in production:

// VULNERABLE — anyone can set the fee recipient to themselves
function setFeeRecipient(address recipient) external {
    feeRecipient = recipient;
}

// FIXED
function setFeeRecipient(address recipient) external onlyOwner {
    require(recipient != address(0), "zero address");
    emit FeeRecipientUpdated(feeRecipient, recipient);
    feeRecipient = recipient;
}

Grepping for external and public and checking each one has a guard is a five-minute exercise that teams routinely skip because it feels beneath them. Do it anyway. Then do the harder version: for each guarded function, verify the guard is the correct one. A function marked onlyKeeper that should be onlyOwner is a finding a grep will never surface.

Uninitialised proxies and re-runnable initializers

Upgradeable contracts cannot use constructors for the proxy's state, so initialisation becomes a regular function — and regular functions are callable.

// VULNERABLE — callable by anyone, once, and whoever gets there first wins
function initialize(address _owner) external {
    owner = _owner;
}

// FIXED
function initialize(address _owner) external initializer {
    require(_owner != address(0), "zero owner");
    __Ownable_init(_owner);
}

// AND, in the implementation contract:
constructor() {
    _disableInitializers();   // the implementation itself is never initialisable
}

Two separate bugs live here. The first is a proxy deployed but not initialised in the same transaction — a front-runner initialises it and owns your protocol. The second is the implementation contract left initialisable: an attacker initialises the implementation directly (not through the proxy), becomes its owner, and on a UUPS proxy can then call upgradeToAndCall with a selfdestruct payload, bricking every proxy pointing at it.

Check both. Check also that deployment scripts atomically deploy and initialise, because a correct contract deployed in two transactions has a window.

tx.origin, signature checks and confused deputies

tx.origin is the EOA that started the transaction. msg.sender is the immediate caller. Authorising on tx.origin means any contract the owner interacts with can act as the owner.

// VULNERABLE — a phishing contract calls this with the owner as tx.origin
require(tx.origin == owner, "not owner");

// FIXED
require(msg.sender == owner, "not owner");

The same confused-deputy shape appears in signature-based authorisation, and there it is subtler:

• No domain separator — a signature valid on one contract is valid on another with the same struct.
• No chain ID — a signature valid on a testnet or a fork is replayable on mainnet.
• No nonce, or a nonce that is set but never checked — the same signature is usable repeatedly.
• Malleability — accepting s values in the upper half of the curve order, so one valid signature has a second valid encoding.
• ecrecover returning address(0) on failure — if the expected signer can ever be zero, a garbage signature authenticates.

The question nobody asks: what can the admin actually do?

Most protocols document their admin as “can update parameters”. Then you trace the code and find the admin can set the oracle address, set the fee to 100%, upgrade the implementation, and pause withdrawals but not deposits.

That is not a bug in the sense of a broken require statement. It is a risk that users have not been told about, and in an audit it is a finding — because the admin key is now a single point of failure whose compromise is equivalent to a total drain.

We write out the admin capability list explicitly in every report:

• Which functions can move or redirect value, directly or indirectly?
• What is the worst outcome if the key is compromised right now, with no timelock?
• Are the parameters bounded? A fee setter with no maximum is a drain function with extra steps.
• Is there a timelock, and does it apply to every dangerous path, including upgrades?
• Can the admin pause deposits and withdrawals independently? Pausing only withdrawals is a hostage function.

// Bounded parameters turn an admin key from a drain into a nuisance
uint256 public constant MAX_FEE_BPS = 500;   // 5%, enforced in code

function setFee(uint256 newFeeBps) external onlyOwner {
    require(newFeeBps <= MAX_FEE_BPS, "fee too high");
    emit FeeUpdated(feeBps, newFeeBps);
    feeBps = newFeeBps;
}

Designing roles that survive an upgrade

• Two-step ownership. Ownable2Step everywhere. A typo in a single-step transfer is unrecoverable.
• Roles, not one owner. Separate keeper, pauser, upgrader and treasury roles so an operational key compromise is not a protocol compromise. The keeper key lives on a hot server; the upgrader key should not.
• Timelock the dangerous paths. Upgrades and parameter changes go through a delay long enough for users to exit. Emergency pause is the exception, and it should only ever be able to make the protocol safer.
• Events on every privileged change. Without them, monitoring cannot exist and your incident timeline will be reconstructed from raw traces.
• Bound every parameter in code. Governance discipline is not a security control.
• Re-check the matrix after every upgrade. New functions arrive without guards more often than existing guards are removed.

Access control is boring, and that is precisely why it keeps working as an attack. Build the matrix, check it against intent, bound the parameters, and split the keys. It is the cheapest security work available and it prevents the most expensive class of failure.

All code in this article is illustrative — written to demonstrate a pattern, not taken from any client engagement. If you want this class of issue hunted in your own codebase, that is what a Smart Contract Security Audit is for.