Missing access control, the bug that reads like working code

Access control failures are consistently among the most costly bug classes in deployed Solidity, and almost none of them are subtle once you see them. The difficulty is that the vulnerable version looks like the safe version: the contract has an owner, it stores it, it may even expose it publicly. It simply never checks it on the one function that matters.

The vulnerable contract

A marketplace forwards trading fees to this treasury as plain ETH. The intent, stated in its own documentation, is that only the deployer can withdraw what accumulates.

contract FeeTreasury {
    address public owner;

    constructor() {
        owner = msg.sender;
    }

    receive() external payable {}

    function collectFee() external payable {
        // marketplace forwards fees here
    }

    function withdrawFees(address payable to, uint256 amount) external {
        (bool success, ) = to.call{value: amount}("");
        require(success, "withdraw failed");
    }
}

Why it fails

withdrawFees is external with no modifier and no require on msg.sender. Anyone can call it, with any to address and any amount. There is no exploit to write and no setup to arrange — the attack is one transaction calling a public function exactly as it was written to be called.

The owner variable makes this worse rather than better. It is assigned in the constructor and never read anywhere in the contract. A reviewer skimming the file sees an owner, sees a withdrawal function, and their eye completes the pattern. Storing an owner is not access control; checking it is.

Nothing in the toolchain objects. The contract compiles cleanly, the types are correct, and a unit test that deploys as the owner and withdraws as the owner passes — because the owner is also allowed to do this. The test never tries it as anyone else, so the bug is invisible to a suite that only exercises the happy path.

The fix

Add the check. A modifier is worth it even for a single call site, because the next privileged function added to this contract will reach for the modifier that already exists rather than re-deriving the check.

error NotOwner();

modifier onlyOwner() {
    if (msg.sender != owner) revert NotOwner();
    _;
}

function withdrawFees(address payable to, uint256 amount) external onlyOwner {
    (bool success, ) = to.call{value: amount}("");
    require(success, "withdraw failed");
}

In production, prefer OpenZeppelin's Ownable or AccessControl over a hand-rolled owner. Not because the check above is wrong, but because they bring the parts people forget: a two-step ownership transfer (Ownable2Step) so a typo in a new owner address cannot permanently orphan the contract, events on transfer so the change is observable off-chain, and role separation for when "the owner" eventually needs to be three different privileges.

How to find these before deployment

• —Enumerate every state-changing external function and write down who is supposed to be able to call it. Any row where that answer is not "anyone" needs a visible check in the code.
• —Grep for privileged variables that are never read. An owner or admin that appears only in the constructor is a strong signal.
• —Write the negative test. For each privileged function, one test that calls it from an unauthorised address and expects a revert. This is the test that would have caught the contract above, and it is the one most often missing.