Access Control Vulnerabilities: Field Notes from the Auditor's Desk

If I had to pick the single category of bug that I find most often in production Solidity, it would be broken access control. The mistakes are rarely glamorous — a missing require, a stray internal function exposed to the world, an onlyOwner modifier that's missing the literal word require. But the impact is almost always total: drained treasuries, infinite mints, stolen ownership. The protocol may have a sophisticated AMM, a clever staking curve, a beautifully written governance system — and lose every dollar of TVL because somebody forgot to gate a function.

In this write-up I want to walk through four engagements I conducted, each one demonstrating a different flavor of access control failure. I'm going in order of subtlety, not severity. They all end with the attacker walking away with everything.

Part 1 — Audit 1: "ProtocolVault" — The Exposed Internal Helper

The first contract was a tiny ETH vault written in old pragma ^0.4.24. The story is that users deposit ETH into it via the fallback function, and only the owner can withdraw the balance back out. Three users had deposited 10 ETH each, leaving 30 ETH in the contract. My job was to drain it from an attacker account that is not the owner.

pragma solidity ^0.4.24;

contract ProtocolVault {
    address public owner;

    function ProtocolVault() public {
        owner = msg.sender;
    }

    function withdrawETH() external {
        require(msg.sender == owner, "Not owner");
        this._sendETH(msg.sender);
    }

    function _sendETH(address to) {
      to.transfer(address(this).balance);
    }

    function() external payable {}
}

My First Read

Upon reviewing this code, my eye went straight to withdrawETH. It has a clean require(msg.sender == owner, "Not owner") guard. The function then calls this._sendETH(msg.sender) to actually move the ETH. From a first-glance read, this looks safe — the owner check is the first line of the only externally callable withdraw path.

But upon closer inspection, I realized something was very wrong with _sendETH. The function name begins with an underscore — by convention, that means "internal helper, don't call me from outside." But the function declaration is missing a visibility specifier:

function _sendETH(address to) {

In Solidity 0.4.x, when a function has no visibility specifier, it defaults to public. The leading underscore is just a naming convention — it has zero effect on the compiler. So _sendETH is, in fact, a public function that anyone in the world can call directly. It takes a destination address as a parameter, and unconditionally transfers the entire contract balance to that address. The withdrawETH function is window dressing — the real backdoor is the "internal" helper that isn't internal.

The Exploit

it('Exploit', async function () {
    await this.protocolVault.connect(attacker)._sendETH(attacker.address);
});

One line. The attacker calls the helper directly, names themselves as the recipient, and walks away with 30 ETH.

Root Cause and Fix

Two compounding bugs. First, the missing internal keyword. Second, the use of this._sendETH(...) from withdrawETH — that this. prefix turns an internal call into an external call, which is what forced the developer to make the function visible to the outside world in the first place. The fix is to make _sendETH internal and to call it as _sendETH(msg.sender) without the this.:

function _sendETH(address to) internal {
    to.transfer(address(this).balance);
}

function withdrawETH() external {
    require(msg.sender == owner, "Not owner");
    _sendETH(msg.sender);
}

Modern Solidity (>=0.5.0) refuses to compile a function with no visibility, which closes this hole at the language level. But plenty of legacy code is still on chain.

Part 2 — Audit 2: "To The Moon" — The Modifier That Modifies Nothing

The second engagement was an ERC20 token called To The Moon, deployed on BSC. It inherits OZ's ERC20Pausable, has an owner, and exposes mint and pause behind an onlyOwner modifier. My goal: own more than one million tokens from an attacker account that the deployer never minted to.

contract ToTheMoon is ERC20Pausable {

    address public owner;

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

    constructor(uint256 _initialSupply) ERC20("To The Moon", "TTM") {
        owner = msg.sender;
        _mint(owner, _initialSupply);
    }

    function mint(address _to, uint256 _amount) external onlyOwner {
        _mint(_to, _amount);
    }

    function pause(bool state) external onlyOwner {
        // ...
    }
}

My First Read

Upon reviewing this code, the structure looked completely standard. Owner is set in the constructor. mint is gated by onlyOwner. Initial supply is minted to the deployer. There's a pause for emergencies. Every line is what you'd expect from a textbook ERC20.

But upon closer inspection, I read the onlyOwner modifier very slowly, character by character. Here it is again:

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

That line msg.sender == owner; looks like a check. It is not a check. It is an expression statement — the Solidity compiler evaluates the boolean msg.sender == owner, discards the result, and moves on. There is no require. There is no if (...) revert. The modifier reduces to nothing more than _; — it does literally no access control work. Every function decorated with onlyOwner is effectively public to the world.

This is one of those bugs that auditors call a "compiler-doesn't-help" bug. The code compiles. It deploys. It runs. The protocol team probably tested the happy path — owner mints, supply increases — and saw it work. They never tested the unhappy path of a non-owner calling mint, so they never saw the modifier silently allow it.

The Exploit

it('Exploit', async function () {
    await this.toTheMoon.connect(attacker)
        .mint(attacker.address, ethers.utils.parseEther("2000000"));
});

Two million tokens, minted directly to the attacker.

The Fix

Add the require. That's the entire fix.

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

The lesson is procedural rather than technical: any access control modifier deserves a negative test — a test that explicitly calls the gated function from an unauthorized account and asserts a revert. If the negative test passes when it shouldn't, the modifier is broken.

Part 3 — Audit 3: "Kilian Exclusive" — When the Withdraw Function Forgets Who You Are

The third audit was a luxury-NFT contract for the fragrance brand Kilian. Each fragrance NFT costs 10 ETH; six are sold during the engagement, leaving 60 ETH in the contract. My target: steal those 60 ETH from an attacker account.

The contract is otherwise sensible — ERC721 with Ownable, an admin-gated addFragrance, a flipSaleState, a purchaseFragrance with proper price and id checks. Then this:

function withdraw(address _to) public {
    require(msg.sender == _to);
    payable(_to).transfer(address(this).balance);
}

My First Read

Upon reviewing this code, I'll be honest, the contract is mostly fine. The mint logic is well-fenced. The onlyOwner modifier inherited from OZ's Ownable actually does work (unlike the previous case). The refill timer uses block.timestamp correctly. The base URI is admin-only.

But then I got to withdraw. The function takes a destination _to parameter and requires that msg.sender == _to. That's it. There is no check that _to is the owner. There is no check that _to is on an allowlist. The function is public, the price-check is msg.sender == _to, and the entire treasury is sent to _to. The bar to clear is "name yourself as the recipient."

The intent of the code is clear: the developer wanted to prevent someone from withdrawing the funds to a different address. They forgot that the actual security property they needed was "only the owner can withdraw." msg.sender == _to is a self-tautology — anyone can satisfy it by passing their own address.

The Exploit

it('Exploit', async function () {
    await this.kilian.connect(attacker).withdraw(attacker.address);
});

60 ETH transferred to the attacker.

Root Cause and Fix

The fix is to gate the function on the owner, not on the caller-being-equal-to-themselves:

function withdraw(address _to) public onlyOwner {
    payable(_to).transfer(address(this).balance);
}

Or — even better — remove the _to parameter entirely and hardcode the destination to owner():

function withdraw() public onlyOwner {
    payable(owner()).transfer(address(this).balance);
}

The investigator's lesson: when a function moves money, the access check must validate who is allowed, not who claims to be the recipient. Those are different questions.

Part 4 — Audit 4: "Starlight" — The Ownership Land-Grab

The fourth engagement was the most insidious of the four, because the access control on the money-moving function actually worked. The bug was somewhere else entirely.

Starlight is an ICO contract. Users buy tokens at 0.01 ETH each; the contract accumulates ETH; the owner can withdraw. Three users deposit 95, 65, and 33 ETH, for a total of 193 ETH. My goal: drain it from an attacker account.

The relevant function on the money-moving side is fine:

function withdraw() external onlyOwner {
    payable(owner()).sendValue(address(this).balance);
}

Compare this to the previous Kilian case — onlyOwner is correctly applied, the destination is hardcoded to owner(). There's no obvious hole here.

My First Read

Upon reviewing this code, my first read of Starlight.sol itself looked clean. buyTokens checks for a non-zero recipient and validates the ETH-to-token ratio. withdraw is owner-gated, owner-destination. So if this were a one-file audit, I'd be looking for something exotic like a buyTokens accounting bug.

But this contract inherits from a custom Ownable defined in the same file, not OpenZeppelin's. That's the cardinal sign to slow down. Whenever I see a hand-rolled version of a contract that has a perfectly good standard library version, I assume there's a reason — and the reason is usually a bug.

So I read the custom Ownable line by line. The onlyOwner modifier looks correct (require(_owner == _msgSender(), "Ownable: caller is not the owner")). The renounceOwnership is gated correctly. The lock/unlock mechanism is bolted on but the gates are present. And then:

function transferOwnership(address newOwner) public virtual {
    require(newOwner != address(0), "Ownable: new owner is the zero address");
    emit OwnershipTransferred(_owner, newOwner);
    _owner = newOwner;
}

There is no onlyOwner modifier on transferOwnership.

Compare with the OpenZeppelin reference implementation, which is public virtual onlyOwner. The developer copy-pasted the function signature but dropped the modifier. Anyone in the world can call transferOwnership and become the owner. Once you are the owner, every other access-controlled function — including withdraw — is yours.

The Exploit

it('Exploit', async function () {
    // Step 1: become the owner.
    await this.starlight.connect(attacker).transferOwnership(attacker.address);
    // Step 2: walk through the front door.
    await this.starlight.connect(attacker).withdraw();
});

193 ETH transferred to the attacker.

Root Cause and Fix

The fix is one keyword:

function transferOwnership(address newOwner) public virtual onlyOwner {
    require(newOwner != address(0), "Ownable: new owner is the zero address");
    emit OwnershipTransferred(_owner, newOwner);
    _owner = newOwner;
}

Better still: don't hand-roll Ownable. Use OpenZeppelin's @openzeppelin/contracts/access/Ownable.sol. Hand-rolling primitives is how you generate exactly this class of bug.

Part 5 — Closing Notes from the Auditor's Desk

If I zoom out across these four engagements, the same lesson repeats in different costumes:

In ProtocolVault, the developer assumed a naming convention (_sendETH) was a security boundary. It wasn't.

In ToTheMoon, the developer wrote a modifier that looked like it did a check, but in fact discarded the boolean and let everyone through.

In KilianExclusive, the developer guarded the wrong predicate — caller-equals-recipient rather than caller-equals-owner.

In Starlight, the developer copied a well-known function signature but dropped the modifier that made it safe, handing ownership to whoever asked first.

When I audit a contract for access control, these are the questions I now ask reflexively, in this order:

I ask whether every state-changing function has an explicit visibility specifier. I ask whether every modifier actually contains a require (or equivalent) — not an expression statement that the compiler will silently discard. I ask whether the gates on each function answer the question who is authorized to perform this action, rather than some weaker tautological predicate. I ask whether ownership-transfer, role-grant, and admin-setter functions are themselves gated — because losing ownership is losing everything. And I ask whether any "internal helper" is reachable from outside the contract because of a missing visibility keyword, a this.-prefixed call, or a public default in a legacy Solidity version.

The smallest typos in access control modifiers have produced some of the largest losses in this industry. Audit accordingly.