Week 04 — Security Foundations: CEI & Access Control

“A contract that ships without explicit CEI ordering and a documented privilege map isn’t a contract — it’s a request to be exploited. The bug classes in this week’s lesson are responsible for more lost USD than any other category, not because they are difficult, but because developers keep building on the assumption that ‘the owner won’t be malicious’. The auditor’s job is to write that sentence down, attach a dollar number to it, and put it on the front page of the report.”

Tags: web3-security security-foundations cei access-control ownership pausable pull-payment storage-layout centralization-risk Learner: Past Tuan-03-Solidity-Foundry-Workflow → ready to begin systematic vulnerability analysis Time: 7 days (4–6h/day) Related: Tuan-05-Vulnerability-Classes-Part-1 · Tuan-14-Governance-DAO-Security · Case-Parity-Multisig-2017 · Case-Radiant-Capital-2024

1. Context & Why

1.1 The week’s thesis

Every line of every smart contract sits inside two implicit contracts the developer signed without reading:

The execution-order contract: the EVM lets external code run in the middle of your function. If you state-update after you call out, you are giving the world a free re-entry.
The privilege contract: every onlyOwner, every onlyRole, every if (msg.sender == admin) is a trust dependency. If the admin is compromised, the contract is compromised — regardless of how well-tested the rest of the logic is.

This week we attack both. Checks-Effects-Interactions (CEI) is the structural defense against the first. A disciplined access-control architecture — Ownable2Step, AccessControl, multisig, timelock, pause — is the defense against the second.

By Friday you will be unable to read a privileged function without instinctively asking: who can call this, under what delay, with what recovery, and what’s the worst they can do? That instinct is the difference between “I found a reentrancy in a function nobody calls” and “I caught a $100M centralization risk three weeks before launch”.

1.2 The auditor’s mandate (this week’s deliverable mindset)

Every audit report you will ever write contains a section called “Centralization Risks” (sometimes “Trust Assumptions” or “Privileged Roles”). Top firms (Trail of Bits, OpenZeppelin, Spearbit, ChainSecurity) include it as a standard section, not an “if applicable”. The L2Beat Stages framework codifies it for L2s: a Stage-0 rollup has full training wheels, Stage-1 has limited admin powers behind a Security Council and exit windows, Stage-2 is no training wheels. The work you do this week is the work that produces that section.

The output you should aim for, by end of week, on any contract you audit:

Function	Role required	Delay (timelock)	Worst-case impact	Severity if abused
`setOracle(address)`	`ADMIN_ROLE`	none	swap to manipulated oracle → drain	Critical
`pause()`	`PAUSER_ROLE`	none	DoS all flows	High (operational)
`upgradeTo(address)`	`proxyAdmin`	48h timelock	re-implement contract → drain	Critical, mitigated by delay
`setFeeRecipient(address)`	`ADMIN_ROLE`	none	redirect fees	Medium

That table is the artifact. Everything in §1–§10 is how to produce it accurately.

1.3 Learning goals

By Friday, you can:

Apply CEI ordering to any function with external calls and explain why it works at the EVM level.
Recognize and articulate where CEI is necessary but not sufficient (preview of cross-function and read-only reentrancy that Week 05 covers in depth).
Compare Ownable, Ownable2Step, AccessControl, and AccessManager and pick the right one for a given protocol.
Explain the “forwarder anti-pattern” and find one in code in <60 seconds.
Convert a push payment flow to pull and explain the gas/DoS/reentrancy trade-offs you fixed.
Add a storage gap (or, in OZ v5+, an ERC-7201 namespaced layout) to a base contract and demonstrate a V1→V2 layout collision the gap prevents.
Catalog every privileged function in a real contract and write the “Centralization Risks” appendix.

1.4 Primary references

Source	URL	Status
OpenZeppelin Contracts — Access Control	https://docs.openzeppelin.com/contracts/5.x/access-control	Current (OZ 5.x targets Solidity ^0.8.20)
OpenZeppelin API — `access` module	https://docs.openzeppelin.com/contracts/5.x/api/access	Current
OZ blog — AccessManager (announcement)	https://www.openzeppelin.com/news/access-management	Current (introduced in OZ 5.0) [verify against OZ 5.x release notes for any post-5.0 changes]
OZ — Upgradeable Contracts (storage)	https://docs.openzeppelin.com/contracts/5.x/upgradeable · https://docs.openzeppelin.com/upgrades-plugins/writing-upgradeable	Current; OZ 5.x uses ERC-7201 namespaced storage instead of `__gap` for its own contracts
Solidity Security Considerations	https://docs.soliditylang.org/en/latest/security-considerations.html	Current
Solidity custom errors	https://docs.soliditylang.org/en/latest/contracts.html#errors-and-the-revert-statement	Custom errors available since 0.8.4; `require(cond, CustomError())` syntax since 0.8.26 (via-IR) / 0.8.27 (legacy) [verify]
Trail of Bits — Building Secure Contracts	https://github.com/crytic/building-secure-contracts	Current
ConsenSys SCBP	https://consensys.github.io/smart-contract-best-practices/	Partial — still accurate on CEI/AC; randomness and a few other sections dated
Solodit checklist (Cyfrin)	https://github.com/Cyfrin/audit-checklist · https://solodit.xyz/checklist	Current; central reference for “centralization risk” findings
L2Beat Stages framework	https://l2beat.com/stages	Current; useful template for how to think about privilege disclosure
EIP-7201 (Namespaced Storage Layout)	https://eips.ethereum.org/EIPS/eip-7201	Final
EIP-7702 (set-code for EOAs)	https://eips.ethereum.org/EIPS/eip-7702	Live as of Pectra (May 2025) [verify exact mainnet activation date and any 2026 EIP follow-ups]

2. The CEI Pattern — Checks, Effects, Interactions

2.1 What CEI is (and why it’s not optional)

CEI is the rule:

For any function: do all checks first, then make all effects (state changes), then perform any interactions (external calls).

function withdraw(uint256 amount) external {
    // 1) CHECKS
    require(amount > 0, "zero amount");
    require(balances[msg.sender] >= amount, "insufficient");
 
    // 2) EFFECTS
    balances[msg.sender] -= amount;
 
    // 3) INTERACTIONS
    (bool ok,) = msg.sender.call{value: amount}("");
    require(ok, "send failed");
}

The reason this is the rule and not a “best practice” is rooted in EVM semantics. Every external CALL opcode transfers control to another contract that can do anything within its gas budget — including calling back into your contract. If your state hasn’t already reflected the user’s withdrawal, the re-entered call sees stale balances and the withdrawal can be repeated. This is the DAO bug (2016), and it is still being re-discovered in production code in 2024 (Penpie/Pendle reentrancy on reward accounting).

A clean way to internalize the rule: the external call is a portal to an adversarial universe; close every door (commit every state change) before walking through it.

2.2 The full picture: where the external call lives

“External call” is broader than .call{value:}. Any of these can hand control to attacker code:

Pattern	Where it appears
`address.call{value: x}(data)`	low-level value+data call
`IERC20(token).transfer(to, amt)` / `transferFrom(...)`	including ERC-777, fee-on-transfer, and any token with hooks
`IERC721(nft).safeTransferFrom(...)`	calls `onERC721Received` on the recipient
`IERC1155(nft).safeTransferFrom(...)`	calls `onERC1155Received`
`payable(addr).transfer(amt)` / `.send(amt)`	2300-gas hop, usually not re-entrant — but don’t rely on this (see §2.4)
`address.delegatecall(data)`	full execution in your storage (covered Week 05 §4)
Solidity `try/catch` on an external contract call	still an external call; same rule
`extcodesize` check + `staticcall`	`staticcall` cannot mutate, so callers can’t be re-entered to write — but they can return adversarial data

The auditor’s reflex: every time control crosses your contract boundary, mentally insert “adversary runs here” and verify that none of your state-update needs are still pending.

2.3 CEI necessary but not sufficient — preview

CEI is structural protection for single-function reentrancy within one contract. It does not prevent:

Cross-function reentrancy: function A calls externally; the attacker re-enters into function B on the same contract that reads from the same state. If A is CEI but B is unprotected, B may see partially-updated state.
Cross-contract reentrancy: two of your contracts share state via storage in a third party (e.g., a token balance). A callback into the second contract sees stale view of the first.
Read-only reentrancy: a view function you publish returns interim state during your own state transition. Another protocol reads that view as an oracle and consumes wrong data.

Week 05 covers each in depth with PoCs. For Week 04 the rule is: CEI is the floor. Pair it with ReentrancyGuard (mutex), pair it with pull-over-push (§5), pair it with a contract-wide mental model of which functions share which state.

2.4 Anti-pattern catalog

Each of these should fire instantly when you read the code:

// ANTI-PATTERN 1: state update after external call
function badWithdraw() external {
    uint256 amt = balances[msg.sender];
    (bool ok,) = msg.sender.call{value: amt}("");
    require(ok);
    balances[msg.sender] = 0;        // <-- too late
}

// ANTI-PATTERN 2: relying on 2300-gas transfer for safety
function badWithdraw2() external {
    uint256 amt = balances[msg.sender];
    payable(msg.sender).transfer(amt); // 2300 gas, "safe"
    balances[msg.sender] = 0;          // <-- still after the call, fragile
}
// Why fragile: EIP-1884 (Istanbul) increased opcode prices; 2300 may be insufficient
// for legitimate receivers (especially smart accounts under ERC-4337). And `transfer`
// re-throws on failure, turning every receiver-side revert into a DoS for the sender.

// ANTI-PATTERN 3: external call inside the checks block
function badRedeem(IERC20 token, uint256 amount) external {
    require(token.balanceOf(address(this)) >= amount, "no liquidity"); // call!
    // ... attacker controls `token` and uses balanceOf as a re-entry point
    balances[msg.sender] -= amount;
    token.transfer(msg.sender, amount);
}
// "Checks" must check local state. Reading from an attacker-controllable contract is
// an interaction in disguise.

// ANTI-PATTERN 4: looping external calls with state updates inside
function badBatchPayout(address[] calldata recipients) external {
    for (uint256 i = 0; i < recipients.length; i++) {
        uint256 amt = pending[recipients[i]];
        (bool ok,) = recipients[i].call{value: amt}(""); // re-entry, gas grief, DoS
        require(ok);
        pending[recipients[i]] = 0;
    }
}
// Convert to a pull pattern (§5) or, at minimum, snapshot+zero state before the loop.

// ANTI-PATTERN 5: token transferFrom before recording credit
function badDeposit(IERC20 token, uint256 amount) external {
    token.transferFrom(msg.sender, address(this), amount);  // call!
    balances[msg.sender] += amount;                         // effect after
}
// With ERC-777, fee-on-transfer, or hook-equipped tokens, this is exploitable.
// Cream/Iron Bank 2021 had a variant of this shape (Week 05).

// ANTI-PATTERN 6: ignoring the return value
function badPay(address to, uint256 amt) external {
    to.call{value: amt}("");  // <-- no `(bool ok,)` destructure
    paid[to] = true;          // marked as paid even when the call failed silently
}

2.5 Defense in depth: CEI + ReentrancyGuard + Pull

For any function that performs an external call, the strong-by-default pattern is all three:

contract Vault is ReentrancyGuard {
    mapping(address => uint256) public balances;
    mapping(address => uint256) public withdrawable; // for pull
 
    // PUSH style, defense in depth
    function withdrawNow(uint256 amount) external nonReentrant {
        // CHECKS
        require(amount > 0, "zero");
        uint256 bal = balances[msg.sender];
        require(bal >= amount, "no funds");
        // EFFECTS
        balances[msg.sender] = bal - amount;
        // INTERACTIONS
        (bool ok,) = msg.sender.call{value: amount}("");
        require(ok, "send failed");
    }
}

nonReentrant is cheap (~2,300 gas after the first SSTORE in a tx on warm-slot rules, slightly more on the first call due to non-zero-to-non-zero pricing; transient storage via EIP-1153 is cheaper but check Solidity version support). CEI is free at runtime. The combination is structurally sound against single-function reentrancy. Cross-function and read-only require additional care (Week 05).

3. Access Control Patterns

3.1 The single design question

For every privileged function in a protocol, the auditor must answer the same five sub-questions:

Who can call this? (specific address, role, or open?)
How many independent parties does it take? (1-of-1, m-of-n, threshold signature?)
How fast can it execute? (instant, after a timelock, after a grant delay?)
Who can stop it before execution? (guardian, governance veto, pause?)
What does it do if abused? (worst-case dollar impact, reversibility?)

The access-control library you choose is just an implementation of that answer. The library hides nothing; it shifts the question to which addresses hold which roles. That second question is where centralization findings live.

3.2 `Ownable` — single owner, legacy default

import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol";
 
contract Vault is Ownable {
    constructor(address initialOwner) Ownable(initialOwner) {}
 
    function withdraw(uint256 amt) external onlyOwner { /* ... */ }
}

OZ 5.x changed the constructor to take initialOwner explicitly (previously it defaulted to msg.sender). This is itself an audit-relevant change: factories deploying Ownable contracts pre-5.x silently became owners; 5.x forces the deployer to think about who owns what.

Properties:

Single address controls all onlyOwner functions.
transferOwnership(newOwner) is immediate — no two-step.
renounceOwnership() sets owner to address(0), permanently disabling all onlyOwner functions. Used to “decentralize” a contract by removing admin control entirely.

Audit findings you should always raise on bare Ownable:

Finding	Why
Single point of failure	One key compromise = full control. Increasingly unacceptable for protocols holding user funds.
Typo risk on transfer	`transferOwnership(0x...wrongAddress)` is instantaneous and irreversible. The owner can transfer to a contract that can’t accept (lost) or to a wrong EOA (potentially lost or phished).
No role separation	Pauser, fee setter, upgrader, oracle setter — all collapsed into one address. Principle of Least Privilege violated by design.
`renounceOwnership` is a footgun	A live function callable by the owner that permanently disables admin. A compromised key can use this for griefing (brick all admin functions before draining is even possible).

For a contract holding any meaningful TVL, Ownable should rarely appear in 2026 except as a base class wrapped by a multisig + timelock. Bare Ownable with an EOA owner is a Medium severity centralization finding by default.

3.3 `Ownable2Step` — two-step transfer, mandatory floor

import {Ownable2Step} from "@openzeppelin/contracts/access/Ownable2Step.sol";
 
contract Vault is Ownable2Step {
    constructor(address initialOwner) Ownable(initialOwner) {}
}

The transfer flow:

Current owner calls transferOwnership(newOwner) → sets _pendingOwner = newOwner. Emits OwnershipTransferStarted.
New owner calls acceptOwnership() → verifies msg.sender == _pendingOwner, then performs the transfer. Emits OwnershipTransferred.

Why this matters as a security property and not just UX:

Typo resistance: if step 1 targets a wrong address, the wrong address must explicitly accept. A random typo’d address won’t.
Governance handoff verification: when handing the keys to a multisig or Timelock, the new owner must demonstrably control the recipient address by calling acceptOwnership from it. This proves the recipient can sign at all, before you’ve actually transferred power. Catches “we set the timelock contract as owner but never tested that the timelock can call acceptOwnership” — a real class of post-deployment incident.
Compromised-owner mitigation: if the owner key is leaking but not yet drained, you can call transferOwnership(safeWallet) from the leaked key. The attacker, who also has the leaked key, cannot complete the transfer without controlling the safe wallet. The two-step buys recovery time. (Limited mitigation — attacker can also call transferOwnership(attackerAddr). But combined with monitoring + a faster mover, it can save funds.)

Released in OpenZeppelin v4.8 (January 2023). There is no defensible reason to ship bare Ownable over Ownable2Step in 2026. Solodit and most major audit checklists treat “uses Ownable instead of Ownable2Step” as at least Low severity, sometimes Medium when context warrants.

Caveat to flag: renounceOwnership() in Ownable2Step is still single-step and immediate (it’s inherited from Ownable and the two-step extension doesn’t override it). A compromised owner can still renounce and brick the contract. If your protocol’s threat model cares about this, override renounceOwnership() to revert. The OpenZeppelin Cairo contracts repo has had advisories around Ownable2Step + renounceOwnership interactions — read it as a signal that this corner is non-obvious.

3.4 `AccessControl` — role-based access control (RBAC)

For any contract with more than one distinct admin action, AccessControl is the canonical pattern.

import {AccessControl} from "@openzeppelin/contracts/access/AccessControl.sol";
 
contract Vault is AccessControl {
    bytes32 public constant PAUSER_ROLE   = keccak256("PAUSER_ROLE");
    bytes32 public constant ORACLE_ADMIN  = keccak256("ORACLE_ADMIN");
    bytes32 public constant FEE_MANAGER   = keccak256("FEE_MANAGER");
 
    constructor(address admin) {
        _grantRole(DEFAULT_ADMIN_ROLE, admin);
        // Optionally: _grantRole(PAUSER_ROLE, admin);
    }
 
    function setOracle(address o) external onlyRole(ORACLE_ADMIN) { /* ... */ }
    function pause()              external onlyRole(PAUSER_ROLE)  { /* ... */ }
    function setFee(uint256 bp)   external onlyRole(FEE_MANAGER)  { /* ... */ }
}

Key mechanics:

Element	Behaviour
`bytes32` role IDs	Conventionally `keccak256("ROLE_NAME")`. `DEFAULT_ADMIN_ROLE` is `0x00`.
`hasRole(role, addr)`	View; cheap; the modifier `onlyRole(role)` calls `_checkRole(role)`.
`grantRole(role, addr)`	Caller must hold the admin role of `role`. By default that’s `DEFAULT_ADMIN_ROLE`.
`revokeRole(role, addr)`	Same admin requirement as `grantRole`.
`renounceRole(role, confirmation)`	A holder removes their own role. OZ 5.x requires the second arg to equal `msg.sender` as anti-typo defense.
`getRoleAdmin(role)`	Returns the role that administers `role` (default: `DEFAULT_ADMIN_ROLE` for all roles).
`_setRoleAdmin(role, adminRole)`	Internal-only. Lets you build role hierarchies (e.g., `ORACLE_ADMIN`’s admin is `GOVERNANCE_ROLE` instead of the default).

Role hierarchies — the under-used power feature:

// At deployment:
_setRoleAdmin(PAUSER_ROLE,  GOVERNANCE_ROLE);
_setRoleAdmin(ORACLE_ADMIN, GOVERNANCE_ROLE);
_setRoleAdmin(FEE_MANAGER,  TREASURY_ROLE);
_setRoleAdmin(GOVERNANCE_ROLE, TIMELOCK_ROLE);
_setRoleAdmin(TIMELOCK_ROLE, TIMELOCK_ROLE);  // self-administered

Now changing the pauser requires governance, changing the fee setter requires the treasury, and rotating governance itself requires the timelock. This is the right structure for any non-trivial protocol — it lets different stakeholders manage different surfaces without crossing wires.

The DEFAULT_ADMIN_ROLE problem:

By default, DEFAULT_ADMIN_ROLE administers every role. Whoever holds it can grant themselves any other role. So a centralization audit must answer: who holds DEFAULT_ADMIN_ROLE, and how is that address controlled?

In practice, the right answer is one of:

A multisig (Safe), with an explicit signer list and threshold.
A TimelockController, with a published delay.
An AccessManager (see §3.6) for protocols that have outgrown AccessControl’s implicit hierarchy.

If DEFAULT_ADMIN_ROLE is held by a single EOA, the protocol is one phishing email away from total compromise. That’s a High-or-Critical finding depending on the impact of the most powerful function gated by any role (because the holder can grant themselves all roles).

AccessControlDefaultAdminRules is OZ’s hardened variant: enforces single-account DEFAULT_ADMIN_ROLE, mandates a two-step transfer for it (à la Ownable2Step), and supports a delay before the new admin gains powers. The current OZ recommendation is to use it whenever you use AccessControl. Treat its absence as a finding.

Other audit signals on AccessControl:

Signal	Why it’s a finding
`DEFAULT_ADMIN_ROLE` granted to >1 account	Mutual revocation, key-control unclear. Treat as Medium unless intentional.
Roles granted in constructor and never revoked from deployer	Deployer EOA retains super-user; should be transferred to multisig and revoked.
`_grantRole` called from a public function	Self-granting backdoor. Critical.
Same address holds multiple sensitive roles	PoLP violation; even if intended, document explicitly.
Role used as the only gate on an `upgradeTo` function	Combine with timelock — instant-upgrade auth is unacceptable for funded protocols.
`AccessControlEnumerable` not used when role membership matters off-chain	Indexers can’t enumerate role holders without an event scan; not a bug, but a UX/governance smell.

3.5 `AccessControlEnumerable` — when off-chain visibility matters

Adds two view functions: getRoleMember(role, index) and getRoleMemberCount(role). Gas cost is slightly higher (membership stored in an EnumerableSet), but it’s the right default for protocols where role holders need to be enumerated by governance UIs, dashboards, or monitoring. If you find a protocol that promises “all roles publicly visible” but uses plain AccessControl, that’s a documentation-vs-code gap.

3.6 `AccessManager` — centralized authority for multi-contract protocols (OZ 5.0+)

AccessControl solves access for one contract. When you have ten contracts that share an admin model, you end up duplicating role assignments and worse — synchronization bugs where a role granted on contract A was forgotten on contract B.

AccessManager (introduced in OZ 5.0) is a single contract that holds all role assignments for a fleet of AccessManaged contracts. Each managed contract carries the restricted modifier on its privileged functions; the modifier calls back into the AccessManager to check canCall(caller, target, selector).

import {AccessManaged} from "@openzeppelin/contracts/access/manager/AccessManaged.sol";
 
contract Vault is AccessManaged {
    constructor(address initialAuthority) AccessManaged(initialAuthority) {}
 
    function withdraw(uint256 amt) external restricted { /* ... */ }
    function pause()               external restricted { /* ... */ }
}

Roles in AccessManager are uint64 numeric IDs (not bytes32), and the model adds two powerful primitives:

Grant delay: when granting a role, the holder doesn’t immediately gain power. A configurable delay must elapse first. Defends against a compromised admin who grants the attacker a role — gives guardians time to intervene.
Execution delay: each role member can have a per-call delay. They must schedule() a call, wait the delay, then execute(). Effectively a per-action timelock.
Guardians: a separate role per role that can cancel() scheduled operations. Incident response without full revocation.

Audit considerations for AccessManager [verify against current OZ 5.x release notes]:

It’s newer than AccessControl, so the auditor pool is smaller and fewer findings have accumulated. Default to extra scrutiny.
msg.sender in restricted functions becomes the AccessManager, not the user — any code that reads msg.sender for non-auth reasons (e.g., balanceOf(msg.sender)) breaks. This is the most common migration bug.
The ADMIN_ROLE (role 0) administers everything. Same concentration concern as DEFAULT_ADMIN_ROLE; same mitigation (multisig/timelock).
setTargetClosed(target, true) lets the admin freeze an entire contract’s restricted functions — an incident-response tool. Verify governance for it; it’s a centralization risk if instant.

When to recommend AccessManager: protocols with ≥3 contracts sharing roles, especially when the same admin should pause anything, the same fee manager should adjust any fee, etc. For single-contract protocols, AccessControl (or AccessControlDefaultAdminRules) remains the right call.

3.7 Multisig owner (Safe / Gnosis Safe)

By far the most common “owner” in DeFi mainnet 2026 is a Safe multisig — not an EOA. From the contract’s perspective, the Safe is just an address. The audit angle moves off-chain:

Audit question for a Safe owner	Why
What’s the threshold (m/n)?	3-of-5 has different failure modes than 5-of-9. The Radiant Capital October 2024 incident drained $50M with 3 of 11 signers compromised — threshold too low for the multisig size.
Who are the signers?	Are they identifiable individuals? Are they on different hardware? Different jurisdictions? Are any of them anon?
Are signers active?	A 5-of-7 where 4 signers haven’t responded in months is effectively 1-of-3 of the responsive set.
What modules/guards are installed?	Safe modules can bypass the threshold (e.g., a module that allows a single delegate to execute under certain conditions).
Is the Safe owner of itself a Safe?	”Multisig of multisigs” — increased blast radius, complex recovery paths. Document the tree.
Is the Safe behind a Timelock?	If yes, the timelock is what reads as owner on-chain; the Safe is the proposer/canceler of timelocked ops.

Radiant Capital (Oct 2024): three devs with hardware wallets were compromised by malware that swapped transactions during signing. The compromise was on the signing path, not the multisig design. Two lessons: (1) the multisig design assumes signers verify exactly what they sign — a UX assumption violated by blind-signing hardware-wallet flows; (2) “more signers” only helps if each signer’s signing path is independent. A coordinated supply-chain attack on the same wallet client compromises N-of-N signers simultaneously.

This is squarely in the Week 12 lesson territory (wallet/AA/key management) — but it must appear in the centralization risks section of any audit involving a multisig.

3.8 Timelock + governance — delay as a defense

TimelockController (OZ) is the canonical pattern. It splits privileged actions into three steps:

Schedule (PROPOSER_ROLE): post the intended call with a timestamp ≥ block.timestamp + minDelay.
Wait: the delay elapses publicly.
Execute (EXECUTOR_ROLE): anyone with the executor role (often address(0) = anyone, to maximize liveness) calls execute(...).
Cancel (CANCELLER_ROLE / PROPOSER_ROLE): an authorized party can void a scheduled op before execution.

The audit value of a timelock isn’t “the admin can’t act” — it’s “the users can see the action coming and exit if they don’t like it”. Standard delays:

Action class	Typical delay	Reason
Parameter tweak (small fee)	1–2 days	UX friction tolerable; exit time sufficient
Oracle or major param change	7 days	Real users need exit time
Contract upgrade	7–14 days	Worst-case action; long exit window
Emergency pause	none (uncomfortable but pragmatic)	A pause needs to be instant to be useful; mitigate by limiting what pause can do

Common timelock mis-configurations:

Finding	Severity
`minDelay = 0`	Critical — defeats the entire purpose
Same address holds `PROPOSER` and `EXECUTOR` with delay 0	Same as no timelock
Timelock admin has open `updateDelay` with no delay on that call	High — admin can race a malicious schedule
No `CANCELLER` configured	High — accidentally-scheduled actions can’t be canceled
Timelock-controlled contract has functions that bypass the timelock (e.g., `emergencyWithdraw`)	Document and weigh: legitimate emergency vs. bypass that defeats the design

3.9 Pause / Unpause patterns

OpenZeppelin Pausable:

import {Pausable} from "@openzeppelin/contracts/utils/Pausable.sol";
 
contract Vault is Pausable, AccessControl {
    bytes32 public constant PAUSER_ROLE = keccak256("PAUSER_ROLE");
 
    function pause()   external onlyRole(PAUSER_ROLE) { _pause(); }
    function unpause() external onlyRole(PAUSER_ROLE) { _unpause(); }
 
    function withdraw(uint256 amt) external whenNotPaused { /* ... */ }
}

whenNotPaused and whenPaused are the two modifiers. _pause() and _unpause() are internal.

Audit considerations:

Pause scope: which functions are whenNotPaused? If deposit is paused but withdraw isn’t, the protocol can pause inbound funds but users can still exit (correct for most cases). The opposite — withdraw paused, deposit open — is almost always wrong, and is a real finding pattern when developers mark “all state-changing functions paused” reflexively.
Pauser concentration: a single-key pauser is a DoS vector. A protocol can be bricked by a compromised pauser even without funds-loss authority. Pair with a separate UNPAUSER_ROLE held by a slower (more decentralized) party, so a malicious pause can be reversed even if the pauser key is compromised.
Unpause-only-by-governance: extreme variant. Pauser is fast, unpauser is timelocked + governance. Common in mature protocols.
Upgradeable variant: PausableUpgradeable exists; same logic but for upgradeable contracts. The state variable is on the proxy’s storage (correctly initialized via __Pausable_init()).
Read functions during pause: ensure view functions return sensible values during pause. Some protocols accidentally make balanceOf revert during pause because it reads from a paused subsystem — breaks indexers and integrations.

3.10 Principle of Least Privilege & Separation of Duties

Two rules that should structure every access-control design:

PoLP (Principle of Least Privilege): each role gets exactly the powers it needs, no more. A pauser should only pause. A fee manager should only adjust fees. A vault administrator should not be able to change the oracle.

Separation of Duties: high-impact actions require multiple roles or multiple steps. Upgrade is proposer → wait → executor. Treasury withdrawal is committee approval → executor execution. Don’t collapse into one role.

The reflex when reading a contract: for every onlyRole(X) function, ask “if role X is compromised, what’s the worst-case dollar impact?” If the answer is “drain the contract”, the role is too broad. Split.

3.11 The forwarder anti-pattern

Possibly the single most dangerous access-control bug pattern, and one that periodically appears in modern, well-funded protocols:

// CRITICAL ANTI-PATTERN
contract Forwarder {
    address public target;
 
    function setTarget(address t) external onlyOwner { target = t; }
 
    function execute(bytes calldata data) external onlyOwner {
        (bool ok,) = target.call(data);  // <-- arbitrary calldata to anywhere
        require(ok);
    }
}

This pattern — “an onlyOwner function that proxies arbitrary calldata to an arbitrary (or even fixed) target” — collapses the entire access-control surface of the target down to a single owner key. Even if the target contract has 12 separate roles, the forwarder’s owner can execute(abi.encodeWithSelector(target.privilegedFunction.selector, ...)) to invoke any of them.

The 2025 OpenZeppelin audit of Cubewire’s vault contracts identified exactly this: a trustedForwarder whose compromise allowed impersonation of any privileged role including DEFAULT_ADMIN_ROLE. The audit recommendation: treat the forwarder as a governance-critical dependency, restrict updates to the highest privilege, and ideally enforce a timelock before forwarder changes take effect.

Variants to recognize:

multicall(bytes[] calldata data) on an admin contract that delegates each call internally — same anti-pattern with one more step.
“Universal executor” / “operator” patterns where one role can execute on behalf of any user.
ERC-2771 meta-transaction forwarders that pass msg.sender = trustedForwarder and append the original sender from calldata — legitimate use case, but the forwarder becomes a privileged dependency.

The audit reflex: any function whose body contains target.call(data) or delegatecall(data) with attacker-influenceable data is Critical unless proven otherwise. Even onlyOwner-gated, because owner compromise is a real and frequent attack vector (Radiant 2024, the Bybit hack 2025, etc).

4. Input Validation

Input validation is “checks” in CEI, but it deserves its own treatment because the failure modes are different from reentrancy.

4.1 Zero-address checks

Every address parameter that, if set to address(0), would brick the protocol or cause funds-loss must be validated.

function setOracle(address newOracle) external onlyRole(ADMIN) {
    require(newOracle != address(0), "zero oracle");
    oracle = newOracle;
}

When to apply:

Constructor parameters for any address that will be stored.
Setter functions for stored addresses.
Transfer destinations for value or tokens (some protocols accept address(0) as “burn”; document explicitly).
Beneficiary / recipient parameters in claim flows.

Counter-example: Ownable2Step.transferOwnership(address(0)) is intentionally allowed — it cancels a pending transfer. Don’t blindly reject; read the design intent.

4.2 Parameter range validation

Numeric parameters that have a sensible range need explicit bounds:

function setFee(uint256 feeBps) external onlyRole(FEE_MANAGER) {
    require(feeBps <= MAX_FEE_BPS, "fee > max");  // e.g., 1000 = 10%
    fee = feeBps;
}
 
function setCollateralFactor(uint256 cfBps) external onlyRole(RISK_ADMIN) {
    require(cfBps <= 9500, "cf > 95%");  // never 100%
    collateralFactor = cfBps;
}

The fee bug class: a contract whose admin can set fee to 100% effectively has a hidden “drain all incoming funds” function. Even with onlyOwner, this is a centralization finding. Cap the maximum in code, not by social agreement.

4.3 Array length matching for batch ops

function batchTransfer(address[] calldata to, uint256[] calldata amount)
    external
{
    require(to.length == amount.length, "length mismatch");
    require(to.length > 0, "empty");
    require(to.length <= MAX_BATCH, "batch too large"); // DoS guard
    for (uint256 i = 0; i < to.length; ++i) {
        require(to[i] != address(0), "zero address");
        // ...
    }
}

Length mismatch by itself doesn’t usually cause exploit (Solidity reverts on OOB), but unchecked batches that iterate externally are gas-grief and DoS targets. Cap batch size.

4.4 Enum bounds

Solidity reverts on enum-out-of-range as of 0.8.x for uint8 → enum conversions. But raw uint8 parameters used as semantic categories should still be range-checked:

function setState(uint8 newState) external onlyRole(ADMIN) {
    require(newState <= uint8(State.Closed), "invalid state");
    state = State(newState);
}

4.5 Math sanity

function deposit(uint256 amount) external {
    require(amount > 0, "zero deposit");           // not 'amount >= 0' (always true!)
    require(amount <= type(uint128).max, "overflow risk if downcast later");
    // ...
}

The > 0 (not != 0) check protects against a specific bug class: a deposit of 0 that nevertheless mints shares or records balance. ERC-4626 vault inflation attacks exploit zero-share edge cases — covered Week 07.

4.6 Validation as documentation

A virtue of validation that’s often overlooked: every require is documentation. When an auditor reads require(amount <= MAX_DEPOSIT, "max"), they know without asking that there’s a maximum. Functions without input validation force the auditor to ask the team — wastes audit hours and signals immaturity.

5. Custom Errors vs Require Strings

5.1 The basics

Solidity 0.8.4 introduced custom errors as a gas-cheaper alternative to require(cond, "string"):

// Old style
require(amount > 0, "Vault: zero amount");
 
// Custom error
error ZeroAmount();
if (amount == 0) revert ZeroAmount();
 
// Solidity 0.8.26+ (via-IR) / 0.8.27+ (legacy): require with custom error
require(amount > 0, ZeroAmount());  // [verify exact version requirements]

5.2 Why the gas saves

Revert strings are stored in the contract’s deployment bytecode (paying calldata-deploy gas and runtime-load gas every revert). Custom errors compile to a 4-byte selector + ABI-encoded arguments — small constant overhead. Typical savings: ~50–150 gas per revert, plus ~50–300 bytes per error message removed from bytecode.

For contracts deployed many times (factories, account abstraction wallets), bytecode-size savings compound.

5.3 Audit implications beyond gas

Property	Require string	Custom error
Caller-readable	Yes (in revert data)	Yes (selector + encoded args)
Tooling support (4byte)	n/a	Yes (selectors are like function selectors)
Parameters	No	Yes — `error InsufficientBalance(uint256 has, uint256 needs)`
Exposure of internal info	A long string can leak business logic	Same risk — error names matter
Frontend UX	Manual string parsing	ABI-decoded; first-class wallet displays

Modern best practice (OZ 5.x adopts this comprehensively):

Use custom errors with informative names: Vault__InsufficientBalance(...) not Err1().
Include parameters when they help debugging (InsufficientBalance(has, needs)).
Group errors at the top of the contract or in a dedicated *.errors.sol for discoverability.

Auditor’s flag: a codebase that mixes require-strings and custom errors haphazardly is a smell. Pick one. Auditors will notice and reviewers will comment.

6. Pull over Push Payments

6.1 Why push is dangerous

“Push” payment = the contract sends funds to the recipient as part of the same transaction.

// PUSH (dangerous in some shapes)
function refund(address user) external {
    uint256 amt = pending[user];
    pending[user] = 0;
    (bool ok,) = user.call{value: amt}("");
    require(ok, "refund failed");
}

Three failure modes:

Reentrancy (Week 05): if state ordering is wrong, the recipient re-enters.
DoS via revert: if the recipient is a contract that reverts on receive (out of gas, deliberate revert, no receive/fallback), the entire transaction reverts. If the push is in a loop, one hostile recipient blocks payouts for everyone.
Gas griefing: a hostile recipient burns all forwarded gas, OOM’ing subsequent state changes in the caller.

The cleanest, simplest case where push goes wrong is a batched payout:

// VULNERABLE: one bad recipient bricks the whole batch
function distributePrizes(address[] calldata winners) external {
    uint256 share = address(this).balance / winners.length;
    for (uint256 i; i < winners.length; ++i) {
        (bool ok,) = winners[i].call{value: share}("");
        require(ok, "send failed");  // one revert = full revert
    }
}

A single attacker with a contract that reverts on receive can DoS the entire payout. This is the canonical push-DoS pattern.

6.2 The pull pattern

Recipients withdraw their funds in a separate transaction:

mapping(address => uint256) public credited;
 
function creditPrize(address winner, uint256 amt) internal {
    credited[winner] += amt;
}
 
function withdrawCredit() external {
    uint256 amt = credited[msg.sender];
    require(amt > 0, "nothing to withdraw");
    credited[msg.sender] = 0;
    (bool ok,) = msg.sender.call{value: amt}("");
    require(ok, "withdraw failed");
}

Properties:

One recipient’s failure to withdraw doesn’t block others.
Reentrancy surface is isolated: only the withdrawer’s own call can re-enter, and the state is already zeroed (CEI).
Gas costs shift from the protocol to the recipient — usually fine, since the recipient is the one with funds at stake.
Slight UX friction: an extra transaction. Acceptable for most flows.

6.3 OZ `PullPayment` and `Escrow`

OpenZeppelin provides:

PullPayment (utility base): credit recipients via _asyncTransfer(addr, amt); recipient calls withdrawPayments(payable account). (Note: in OZ 5.x the PullPayment utility was deprecated in favor of explicit pull patterns and Escrow/ConditionalEscrow for finer control — [verify exact deprecation in the OZ 5.x release notes].)
Escrow: a standalone escrow contract that holds deposits per beneficiary and lets the owner trigger withdrawals.
ConditionalEscrow: subclass with a withdrawalAllowed() hook the implementer overrides — e.g., “withdrawals open after a deadline”.

Reference these in your audit when reviewing payment flows; use them in the lab to convert push → pull.

6.4 When push is acceptable

Push isn’t always wrong. It’s acceptable when:

The recipient is fixed and trusted (your protocol’s own fee receiver multisig).
The amount is small and the call is gas-bounded ({gas: 50_000} with explicit revert tolerance).
DoS impact is negligible (a single off-chain refund that the team can retry manually).

The general rule: if recipients are untrusted or many, prefer pull. If there’s any loop with external calls, prefer pull. The cognitive cost of “should this be pull?” should fire on every payment-related function.

7. Error Handling

7.1 `try/catch` for external calls

Solidity’s try/catch lets a caller continue execution when an external call reverts:

function safeRefund(address user, uint256 amt) external {
    try IRefundReceiver(user).receiveRefund{value: amt}() {
        // success
    } catch (bytes memory reason) {
        // fallback: credit pull-payment escrow
        credited[user] += amt;
    }
}

Constraints:

Only works on external function calls and contract creations, not on low-level .call.
Catches reverts, but not out-of-gas in some cases — a callee that consumes all forwarded gas and then reverts will still return control, but if the callee’s gas is the caller’s gas (no explicit {gas:} cap), the caller may be left with insufficient gas to continue.
Catches reverts with reason, panic codes, and bare reverts via the catch Error(string), catch Panic(uint), and catch (bytes memory) clauses.

Audit angle: a try that silently swallows failures hides bugs. Always log or otherwise observe the failure path. The audit-bad pattern:

try someProtocol.action() {} catch {} // <-- silent

The audit-fine pattern:

try someProtocol.action() returns (uint256 result) {
    _onSuccess(result);
} catch Error(string memory reason) {
    emit ExternalCallFailed(reason);
    _onFailure();
}

7.2 Revert reason propagation

When using low-level .call, the return data contains the revert reason as ABI-encoded bytes. To re-throw cleanly:

(bool ok, bytes memory ret) = target.call(data);
if (!ok) {
    assembly {
        revert(add(ret, 0x20), mload(ret))
    }
}

OpenZeppelin’s Address.functionCall does this. Use the library.

7.3 `assert` vs `require` vs `revert`

Form	Semantics	Use case
`assert(cond)`	Panic on false; consumes all remaining gas (pre-0.8) / leaves `Panic(uint)` selector (0.8+). Use only for invariant violations that should never happen if the code is correct.	Internal-state invariants. Almost never user-facing.
`require(cond, "msg")` / `require(cond, CustomError())`	Revert with reason on false; refunds remaining gas.	User-facing preconditions.
`revert("msg")` / `revert CustomError()`	Direct revert, refunds gas.	Inside branches, more flexibility than `require`.

The cognitive rule for an auditor:

assert reachable from user input = bug. The contract is asserting something the user can falsify.
require for invariants = smell. Should be assert if truly an invariant; otherwise the check belongs lower (input validation).

7.4 Handling custom-error reverts via low-level call

(bool ok, bytes memory ret) = target.call(data);
if (!ok) {
    // ret = abi.encodeWithSelector(CustomError.selector, args...)
    if (ret.length >= 4) {
        bytes4 sel;
        assembly { sel := mload(add(ret, 0x20)) }
        if (sel == IMyTarget.InsufficientBalance.selector) {
            // handle specific error
        }
    }
    // Otherwise re-throw
    assembly { revert(add(ret, 0x20), mload(ret)) }
}

Custom-error matching from low-level calls is a 2026-current pattern. OpenZeppelin’s Errors library and the try/catch syntax over high-level interface calls is usually cleaner; reach for low-level dispatch only when you must.

8. Storage Layout Hazards in Inheritance

This section is preview-only; full storage-collision-and-proxy-upgrade material is Week 05. Here we focus on the non-proxy hazards that already exist in plain inheritance.

8.1 The slot rule

Storage slots are assigned in C3-linearization order of the inheritance tree, top-down. Each contract’s state variables get sequential slots starting where the parent’s left off.

contract A {
    uint256 a1;  // slot 0
    uint256 a2;  // slot 1
}
 
contract B is A {
    uint256 b1;  // slot 2
    uint256 b2;  // slot 3
}
 
contract C is B {
    uint256 c1;  // slot 4
}

If you add a state variable to A in V2 of the codebase, every derived contract shifts:

contract A {
    uint256 a1;   // slot 0
    uint256 a3;   // slot 1   <-- inserted
    uint256 a2;   // slot 2   <-- shifted!
}
// In B: b1 is now at slot 3, b2 at slot 4. C.c1 at slot 5.

For a non-upgradeable contract, this only matters for new deployments and existing storage in old deployments is fine (they don’t share storage). The bug class is more subtle: if you have a library that reads from slot offsets (e.g., Slot library, EIP-1967-style derived slots) and the layout shifts, the library reads from the wrong place.

For upgradeable contracts (proxy holds state, implementation gets swapped), this is catastrophic — the old proxy’s storage is interpreted with the new layout. Hence the storage gap (§8.2) and the namespaced-storage (§8.4) patterns.

8.2 Storage gaps (`__gap`)

The defensive pattern: every upgradeable base contract reserves a chunk of trailing slots so derived contracts have a buffer.

contract BaseUpgradeable {
    uint256 a1;
    uint256 a2;
    uint256[50] private __gap;  // 50 slots reserved
}
 
contract VaultV1 is BaseUpgradeable {
    uint256 v1;   // slot 52 (0,1 used by Base; 2..51 are __gap)
    address v2;   // slot 53
}

When you upgrade BaseUpgradeable to add a variable, you also reduce the gap:

contract BaseUpgradeable {
    uint256 a1;
    uint256 a2;
    uint256 newA3;       // slot 2 (was __gap[0])
    uint256[49] private __gap;  // 49 slots reserved (was 50)
}

The total Base storage size stays 52 slots. VaultV1’s storage doesn’t shift.

Audit signals:

Upgradeable base contract with no __gap = finding (Medium-High depending on use).
__gap size changed without comment = check that the difference matches the size of any added variable.
New variables added but __gap not reduced = total layout grew = derived contracts’ storage shifted = Critical if deployed.

8.3 Why diamond inheritance is dangerous

Multiple inheritance with shared ancestors (diamond inheritance) causes the C3-linearization order to determine slot layout in subtle ways. Two cases:

contract A { uint256 a; }
contract B is A { uint256 b; }
contract C is A { uint256 c; }
contract D is B, C { uint256 d; }
// Linearization: A, B, C, D
// Slots: A.a=0, B.b=1, C.c=2, D.d=3

Now consider swapping the order:

contract D is C, B { uint256 d; }
// Linearization: A, C, B, D
// Slots: A.a=0, C.c=1, B.b=2, D.d=3

The same logical state variables end up in different slots depending on inheritance order. This is fine for fresh deployments but lethal across upgrades: if V1 was D is B, C and V2 became D is C, B, every variable in B and C is relocated.

The OpenZeppelin Upgrades Plugin (@openzeppelin/upgrades-core) checks for this. Use it. Audit signal: any upgradeable codebase without this plugin in CI is flagged.

8.4 ERC-7201 namespaced storage (the modern alternative)

OZ 5.x abandoned __gap for its own upgradeable contracts in favor of ERC-7201 namespaced storage. The idea: instead of placing state variables at sequential low slots, place each contract’s state in a struct whose location is keccak256(namespace) - 1 (a high-entropy slot impossible to collide with sequential layouts).

contract VaultUpgradeable {
    /// @custom:storage-location erc7201:my.protocol.vault
    struct VaultStorage {
        uint256 totalAssets;
        mapping(address => uint256) balances;
    }
 
    // bytes32(uint256(keccak256("my.protocol.vault")) - 1) & ~bytes32(uint256(0xff))
    bytes32 private constant _VAULT_STORAGE_LOCATION =
        0x{precomputed};
 
    function _getVaultStorage() private pure returns (VaultStorage storage $) {
        assembly { $.slot := _VAULT_STORAGE_LOCATION }
    }
}

Properties:

New variables can be added to VaultStorage freely — they’re at offsets within the namespaced location.
No __gap needed.
Inheritance order doesn’t affect this contract’s storage layout.
Collision with another namespaced contract requires a keccak256 collision — practically impossible.

Audit signals for ERC-7201:

Comment /// @custom:storage-location erc7201:... should match the namespace used in the _LOCATION constant. Mismatch = bug.
Functions accessing storage should consistently go through the _getStorage() accessor, not direct field references.
Mixing namespaced and sequential layouts in one contract is asking for confusion — pick one.

This is current best practice as of OZ 5.x. For older codebases (OZ 4.x), __gap is still the right pattern.

9. The “Rug Surface” — Cataloging Centralization Risks

9.1 The mandate

Every audit report has a “Centralization Risks” or “Privileged Roles” section. The content of that section is a table:

Function	Privileged role	Worst-case impact	Reversible?	Time to detect	Severity
`upgradeTo(address)`	`proxyAdmin`	New implementation can rewrite all logic, drain all funds	No	Instant (event), 48h timelock window	Critical (mitigated)
`setOracle(address)`	`ORACLE_ADMIN`	Swap to manipulated oracle, drain via liquidation	Yes (revert), but funds gone	Instant (event), no delay	Critical
`pause()`	`PAUSER`	DoS deposits/withdrawals	Yes	Instant (event), no delay	High (operational)
`setFeeRecipient(address)`	`FEE_MANAGER`	Redirect future fees	Yes	Instant (event), no delay	Medium
`setFee(uint256 bps)`	`FEE_MANAGER`	Capped at `MAX_FEE_BPS = 1000` (10%)	Yes	Instant (event), no delay	Low
`mintReward(address, uint256)`	`REWARDS_DISTRIBUTOR`	Mint reward token, inflation	Yes (slash future), but tokens distributed	Instant (event), no delay	High (if uncapped)

Each row is a finding the team must explicitly accept or mitigate.

9.2 How to build it

The mechanical process:

Grep every privileged modifier: onlyOwner, onlyRole, restricted, whenNotPaused, custom modifiers. Result: a list of every privileged external function.
For each function, identify the role and its admin: trace getRoleAdmin chains.
For each function, write the worst-case impact sentence: “If X is compromised, Y happens”.
Identify mitigations: timelock delays, parameter caps, separation of duties.
Identify the role-holder: who is X on mainnet? An EOA? A 3-of-5 Safe? A timelock-fronted multisig?
Score severity:
- Drain funds without recovery → Critical.
- Drain with delay (timelock) → High mitigated.
- DoS the contract → High operational.
- Redirect future revenue → Medium.
- Tweak within capped bounds → Low.
Cross-check: does this match the team’s documentation? Does the marketing claim “decentralized” while the table shows critical EOA control? That gap is itself a finding.

9.3 Severity heuristics

A starter rubric (Immunefi and Sherlock vary in details, but the structure is similar):

Trust assumption fails →	Severity
Direct drain of user funds	Critical
Drain of protocol funds (treasury)	High
Permanent loss of upgradability/control	High
Temporary DoS (recoverable)	Medium
Loss of future revenue	Medium
Configuration changes within sensible caps	Low
No funds at stake, only governance flow	Informational

Combine with who can trigger:

Single EOA → multiply severity (add one level)
Small multisig (≤3 signers / low threshold) → keep as-is
Large multisig + timelock + active community oversight → reduce by one level

9.4 Real-world templates

L2Beat Stages (https://l2beat.com/stages): the canonical template for “centralization disclosure” applied to rollups. Stage 0 / 1 / 2 maps to “training wheels on / limited / off”. Use this structure as a template for any protocol with admin powers, not just L2s.
DeFiScan (https://deficollective.org/blog/introducing-defiscan/): similar verifiable-insight framework for DeFi infrastructure.
Solodit checklist (https://solodit.xyz/checklist): includes centralization-risk patterns aggregated from competitive audit findings.

9.5 Examples from real audit reports

The patterns you’ll see in published Trail of Bits / OpenZeppelin / Spearbit reports under “Centralization Risks” (paraphrased archetypes; specific protocol names omitted to keep this evergreen):

“The setFeeRecipient function can change the destination of all protocol fees to any address with no delay, no cap, and only an onlyOwner check. If the owner key is compromised, all future fee revenue is redirected to the attacker.” Severity: Medium.
“The upgradeTo function on the proxy contract has no timelock, allowing the owner to immediately replace the implementation. A compromised owner can deploy malicious code and drain all user funds in a single transaction.” Severity: Critical (recommendation: add TimelockController with ≥ 7 day delay).
“The pause function can be called by a single EOA. While this design is intentional for incident response, the unpause is governed by the same single key, meaning a compromised key can permanently DoS the protocol. We recommend a separation: pause by EOA, unpause by multisig with delay.” Severity: High (operational).
“The setOracle function is onlyOwner with no validation that the new oracle address is a contract. A typo or malicious change can swap to address(0) — all calls to oracle.latestAnswer() will revert and the protocol will be bricked.” Severity: High.
“The mintReward function allows the rewards distributor to mint an unbounded amount of reward tokens. The implementer’s stated intent is ‘only for verified rewards from the off-chain calculator’, but the on-chain function has no cap, no proof requirement, and no per-period limit.” Severity: High.

When you write your audit report, model your findings on these archetypes.

10. Lab — Harden a Weak Contract

10.1 Lab structure

~/web3-sec-lab/wk04/
├── src/
│   ├── 01-WeakVault.sol           (starting point, multiple bugs)
│   ├── 02-VaultV2.sol             (Ownable2Step + AccessControl + Pausable)
│   ├── 03-BaseV1.sol / BaseV2.sol (storage layout demo)
│   └── 04-PullEscrow.sol          (pull-payment refactor)
├── test/
│   ├── CEI.t.sol
│   ├── Ownership.t.sol
│   ├── Access.t.sol
│   ├── PullPush.t.sol
│   └── StorageGap.t.sol
└── audit/
    └── centralization-risks.md    (your written deliverable)

10.2 Starting contract — `WeakVault.sol`

This is deliberately weak. Save as src/01-WeakVault.sol.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.27;
 
import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol";
 
/// @notice Deliberately weak. DO NOT DEPLOY.
contract WeakVault is Ownable {
    address public oracle;
    address public feeRecipient;
    uint256 public feeBps;
    bool    public paused;
 
    mapping(address => uint256) public balances;
    address[] public depositors;  // for the buggy batch refund
 
    constructor(address _oracle) Ownable(msg.sender) {
        oracle = _oracle;
        feeRecipient = msg.sender;
        feeBps = 100; // 1%
    }
 
    function deposit() external payable {
        require(!paused, "paused");
        if (balances[msg.sender] == 0) depositors.push(msg.sender);
        balances[msg.sender] += msg.value;
    }
 
    // BUG: state changes AFTER external call — CEI violation
    function withdraw() external {
        uint256 amt = balances[msg.sender];
        require(amt > 0, "no balance");
        (bool ok,) = msg.sender.call{value: amt}("");
        require(ok);
        balances[msg.sender] = 0;
    }
 
    // BUG: push payment in a loop, no DoS protection
    function refundAll() external onlyOwner {
        for (uint256 i; i < depositors.length; ++i) {
            address u = depositors[i];
            uint256 amt = balances[u];
            balances[u] = 0;
            (bool ok,) = u.call{value: amt}("");
            require(ok, "send failed"); // one bad recipient halts everything
        }
    }
 
    // BUG: no input validation, no cap, no delay
    function setFee(uint256 newFee) external onlyOwner {
        feeBps = newFee; // can be 100% = drain
    }
 
    // BUG: no zero-address check
    function setOracle(address newOracle) external onlyOwner {
        oracle = newOracle;
    }
 
    // BUG: no role separation, single owner can pause and unpause
    function setPaused(bool p) external onlyOwner {
        paused = p;
    }
 
    // BUG: forwarder anti-pattern
    function execute(address target, bytes calldata data) external onlyOwner {
        (bool ok,) = target.call(data);
        require(ok);
    }
 
    receive() external payable {}
}

10.3 Exercise 1 — Demonstrate CEI violation

Write a Foundry test that exploits the missing CEI. (You did the full classic reentrancy in Week 5 prep — this is just to confirm the bug is real and to set up Exercise 2’s fix.)

// test/CEI.t.sol
pragma solidity ^0.8.27;
import "forge-std/Test.sol";
import "../src/01-WeakVault.sol";
 
contract ReenterAttacker {
    WeakVault public v;
    constructor(WeakVault _v) payable { v = _v; }
    function attack() external payable {
        v.deposit{value: msg.value}();
        v.withdraw();
    }
    receive() external payable {
        if (address(v).balance >= 1 ether) v.withdraw();
    }
}
 
contract CEITest is Test {
    WeakVault v;
    function setUp() public {
        v = new WeakVault(address(0xDEADBEEF));
        // Innocent users
        for (uint256 i = 1; i <= 9; i++) {
            address u = address(uint160(0x1000 + i));
            vm.deal(u, 1 ether);
            vm.prank(u);
            v.deposit{value: 1 ether}();
        }
    }
    function test_reentrancyDrain() public {
        ReenterAttacker a = new ReenterAttacker{value: 1 ether}(v);
        a.attack{value: 1 ether}();
        assertLt(address(v).balance, 1 ether, "vault should be (nearly) drained");
    }
}

Task: Fix withdraw() by applying CEI (state change before external call) and adding ReentrancyGuard. Re-run; test should fail (drain prevented).

10.4 Exercise 2 — Convert `Ownable` to `Ownable2Step`, demonstrate why

Build src/02-VaultV2.sol that inherits Ownable2Step instead of Ownable.

Then write a test that:

On the old WeakVault: calls transferOwnership(address(0xBAD)) — confirm owner() == 0xBAD immediately, irreversibly.
On the new VaultV2: calls transferOwnership(address(0xBAD)) — confirm owner() is still the original, pendingOwner() is 0xBAD, and a later acceptOwnership from 0xBAD (which can’t happen if it’s a typo’d EOA with no key) is required to complete.
Demonstrate that calling transferOwnership(safeWallet) followed by acceptOwnership() from the safeWallet confirms the safe wallet can sign.

10.5 Exercise 3 — Add `AccessControl` with custom role admin

Refactor VaultV2 to use AccessControl with separate roles:

bytes32 public constant PAUSER_ROLE   = keccak256("PAUSER_ROLE");
bytes32 public constant UNPAUSER_ROLE = keccak256("UNPAUSER_ROLE");
bytes32 public constant ORACLE_ADMIN  = keccak256("ORACLE_ADMIN");
bytes32 public constant FEE_MANAGER   = keccak256("FEE_MANAGER");
bytes32 public constant GOVERNANCE    = keccak256("GOVERNANCE");

In the constructor, set _setRoleAdmin(PAUSER_ROLE, GOVERNANCE), _setRoleAdmin(UNPAUSER_ROLE, GOVERNANCE), etc.

Verify in tests:

PAUSER_ROLE holder can pause() but not unpause().
UNPAUSER_ROLE holder can unpause() but not pause().
Only GOVERNANCE can grant/revoke either.
DEFAULT_ADMIN_ROLE does not have governance unless explicitly granted.

10.6 Exercise 4 — Push → Pull conversion

Convert refundAll() from push (loop with external calls) to pull:

mapping(address => uint256) public refundCredit;
 
function scheduleRefundAll() external onlyRole(GOVERNANCE) {
    for (uint256 i; i < depositors.length; ++i) {
        address u = depositors[i];
        uint256 amt = balances[u];
        balances[u] = 0;
        refundCredit[u] += amt;
    }
    // No external calls in the loop. No DoS surface.
}
 
function withdrawRefund() external nonReentrant {
    uint256 amt = refundCredit[msg.sender];
    require(amt > 0, "nothing to withdraw");
    refundCredit[msg.sender] = 0;
    (bool ok,) = msg.sender.call{value: amt}("");
    require(ok, "withdraw failed");
}

Test: deploy a “hostile recipient” that reverts on receive. Confirm:

The old WeakVault.refundAll() reverts entirely when the hostile recipient is in the list.
The new pull version completes the schedule successfully; only the hostile recipient’s withdrawRefund() would fail (which is fine — they’re harming only themselves).

10.7 Exercise 5 — Storage gap demonstration

Create two versions of a base contract:

// src/03-BaseV1.sol
contract BaseV1 {
    address public admin;
    uint256 public version;
    // No __gap
}
 
contract DerivedV1 is BaseV1 {
    mapping(address => uint256) public balances; // slot 2
}

Now imagine “V2” adds a variable to the base:

// src/03-BaseV2.sol
contract BaseV2 {
    address public admin;
    uint256 public version;
    uint256 public feeBps;   // <-- new
    // balances mapping would now collide
}

Demonstration (without actually upgrading — just compute slot layouts):

Compute cast index uint256(address(0xAlice)) 2 (the slot for balances[Alice] in V1) → call it S.
Show that in V2, slot 2 is feeBps, not the mapping root.

Then add uint256[50] private __gap to BaseV1 and re-do the calculation: derived balances is now at slot 52, and adding feeBps to base (reducing __gap to 49) leaves derived balances still at slot 52. Layout preserved.

(For full effect, run this with the OZ upgrades plugin: npx hardhat run scripts/validate-upgrade.js — the plugin will explicitly report the layout incompatibility for the no-gap case.)

10.8 Exercise 6 — Write the “Centralization Risks” appendix

Audit your patched VaultV2 and produce the table. For each privileged function:

| Function | Role | Holder (mainnet plan) | Worst case | Mitigation | Severity |
|----------|------|----------------------|------------|------------|----------|
| `pause()` | `PAUSER_ROLE` | DAO multisig (3-of-5) | DoS deposits+withdrawals | Separate UNPAUSER on GOVERNANCE delay; monitoring | Medium |
| `unpause()` | `UNPAUSER_ROLE` | TimelockController (24h) | Delayed unpause | Same; pair w/ pause SLA | Low |
| `setOracle(addr)` | `ORACLE_ADMIN` | DAO multisig (3-of-5) behind 7d Timelock | Manipulated price → liquidation drain | 7d delay; zero-addr check; oracle interface check | High (mitigated) |
| `setFee(bps)` | `FEE_MANAGER` | Treasury multisig (4-of-7) | Capped at MAX_FEE_BPS=1000 (10%); redirect fees | Cap in code; event monitoring | Low |
| `grantRole(GOVERNANCE, x)` | `DEFAULT_ADMIN_ROLE` | Timelock (7d) | Onboard new governance member | 7d delay | Low |

The deliverable for the lab is this table, formatted as you would in an audit report, plus a one-paragraph “trust assumptions” prose summary of the overall surface.

10.9 Stretch — Migrate to `AccessManager`

Refactor VaultV2 to inherit AccessManaged and put all roles in an AccessManager. Demonstrate:

Grant delay: schedule role grant; observe it doesn’t take effect until the delay passes.
Execution delay: a FEE_MANAGER with a 1-day execution delay must schedule() the call, wait, then execute().
Guardian cancellation: a guardian cancels a scheduled call mid-delay.

This stretch is heavier — budget half a day — but it’s the cleanest hands-on with the OZ 5.x model.

10.10 Expected outcomes

After this lab, you should be able to:

Read a privileged function and immediately mentally fill in: role / admin / delay / impact / severity.
Apply CEI mechanically before considering other defenses.
Argue for Ownable2Step over Ownable in any code review and quote the typo-resistance and governance-handoff reasons.
Convert push payments to pull when DoS risk is non-trivial.
Reserve storage gaps (or use ERC-7201 namespaced storage) in any upgradeable base contract.
Produce a “Centralization Risks” table as a deliverable section in your audit report.

11. Anti-pattern Catalog (add to master checklist)

12. Trade-offs and Open Debates

Decision	Option A	Option B	Auditor view
Ownership pattern	`Ownable`	`Ownable2Step`	Always `Ownable2Step` for funded protocols. The single counter-argument is “we’ll never transfer” — but `renounceOwnership` then becomes the only ownership-mutating path, and even that should be considered carefully.
Access pattern	`Ownable2Step`	`AccessControl`	If you have one admin action: Ownable2Step. Two or more: AccessControl with role separation.
AccessControl variant	Plain `AccessControl`	`AccessControlDefaultAdminRules`	Plus rules variant always, in OZ 5.x. It’s strictly safer.
Access architecture for multi-contract protocols	Per-contract `AccessControl`	Centralized `AccessManager`	AccessManager for ≥3 managed contracts; otherwise per-contract is simpler. Audit pool is smaller for AccessManager — be aware of that.
Owner key	EOA	Multisig	Multisig always for >$1M TVL. EOA-owned funded protocols are de facto “this is going to happen” centralization risks.
Multisig threshold	Low (1-of-3, 2-of-5)	High (4-of-7, 5-of-9)	Higher threshold trades liveness for safety. Radiant 2024 was 3-of-11 — too low. 5-of-9 is a reasonable default for mainnet treasuries.
Timelock delay	Short (1d)	Long (7d+)	7d for upgrades and oracle changes. Shorter for low-impact parameters. Zero for emergency pause (with a separately-governed unpause).
Pause architecture	Single role pause+unpause	Split: fast pauser, slow unpauser	Split. Compromised pauser shouldn’t be able to permanently DoS.
`renounceOwnership`	Available	Overridden to revert	Override to revert in production. Renouncing is rarely what you actually want, and it’s irreversible.
Custom errors	Use everywhere	Mix with require strings	Custom errors everywhere. Mixed style is a smell.
Push vs pull payments	Push	Pull	Pull for untrusted/many recipients. Push only for fixed, trusted recipients.
Upgradeable storage	`__gap` reserves	ERC-7201 namespaced	ERC-7201 if starting fresh (OZ 5.x style); `__gap` if extending existing OZ 4.x codebases. Don’t mix in one contract.

13. Quiz (≥80% to advance)

Q: Why does CEI fail to prevent cross-function reentrancy? A: CEI orders state changes before external calls within a single function. If the attacker re-enters into a different function on the same contract that reads from the same state, that function may see partially-updated state from the in-progress original call. CEI doesn’t enforce contract-wide invariants. Mitigation: contract-wide ReentrancyGuard (or nonReentrant on every state-mutating function reading shared state), CEI in every function, ideally pull-payment patterns.
Q: A protocol uses transferOwnership(newOwner) from bare Ownable and accidentally passes a typo’d address. What happens, and how does Ownable2Step fix it? A: Bare Ownable.transferOwnership immediately sets the owner to the typo’d address — irreversible if the typo’d address has no private key, and a security disaster if the typo lands on an attacker-controlled address. Ownable2Step sets _pendingOwner instead; the new owner must explicitly call acceptOwnership() to complete the transfer. A typo’d address can’t accept (no key), so the transfer never finalizes; the original owner can recover by transferring to the correct address.
Q: In AccessControl, what role administers a custom role by default? How do you change it? A: DEFAULT_ADMIN_ROLE (the bytes32(0) constant) administers every role by default. To change it, call _setRoleAdmin(role, newAdminRole) from within the contract (it’s internal). This is how role hierarchies are built — e.g., make GOVERNANCE_ROLE the admin of PAUSER_ROLE so only governance can grant/revoke pause power.
Q: What is the “forwarder anti-pattern” and why is it Critical even when gated by onlyOwner? A: A privileged function that proxies arbitrary calldata to an arbitrary (or even fixed) target. Even with onlyOwner, owner compromise (a frequent attack vector — Radiant 2024 lost $50M to malware on three signers) means the attacker can invoke any function on the target contract, including ones gated by other roles. The forwarder collapses the entire access surface of the target down to one key. Recommendation: never proxy arbitrary calldata; always have specific, narrow setters with explicit input validation; or, if delegation is needed, restrict the selectors and add a timelock.
Q: A protocol holds $10M and is owned by a single EOA. What severity is this as an audit finding? Why? A: Critical (or High depending on the exact threat-model framing) as a centralization risk. The protocol is one phishing email, one supply-chain attack, one compromised laptop away from total loss. Standard recommendation: transfer ownership to a multisig (Safe) with ≥5 signers and ≥3 threshold, ideally behind a Timelock with 7-day delay for high-impact operations.
Q: Why is push-payment in a loop a DoS vector? Give the canonical exploit. A: If the loop reverts on the first failed external call (require(ok, "send failed")), a single hostile recipient — a contract that reverts on receive — blocks the entire batch for everyone else. The exploit is trivial: deploy a contract whose receive() calls revert(), get included as a recipient, and now nobody else can be paid until you’re removed (which may not be possible if the recipient list is fixed). Mitigation: convert to pull (each recipient withdraws their own credit), or try/catch each push and accept partial-success semantics.
Q: What is a storage gap, why is it needed, and what’s the modern alternative? A: A storage gap (uint256[50] private __gap;) is a trailing array of unused slots in an upgradeable base contract. It reserves room so future versions of the base can add new state variables without shifting the storage layout of derived contracts. When adding a new variable, you reduce the gap size by the corresponding number of slots so the total layout stays the same. Modern alternative: ERC-7201 namespaced storage, where each contract puts its state in a struct at a high-entropy keccak256-derived slot. OZ 5.x adopted this for its own upgradeable contracts, eliminating the need for __gap in OZ-derived contracts.
Q: Why is assert(cond) reachable from user input a bug? A: assert is semantically reserved for invariant violations — conditions that should never be false if the code is correct. A user-reachable assert says “the contract’s correctness depends on this user-controlled input never violating this condition”, which is a logical contradiction. The check belongs in require (an input precondition) or in earlier validation, not in assert. In Solidity 0.8+, failed assert triggers Panic(uint256), which is also useful diagnostic signal but doesn’t change the principle.
Q: A protocol uses AccessControl with DEFAULT_ADMIN_ROLE held by a 5-of-9 Safe multisig and no Timelock. The most powerful single function is upgradeTo(address). What’s the severity, and what’s the recommendation? A: High (potentially Critical depending on TVL). The multisig is robust but instant — a compromised majority of signers (or a single zero-day in the Safe interface) can deploy a malicious implementation in one transaction. Recommendation: place a TimelockController between the Safe and the proxy admin role, with a ≥7-day delay. The Safe becomes proposer + canceller; the timelock is the actual admin. Users get a 7-day window to exit if the proposed upgrade is malicious.
Q: You’re auditing a contract. The Centralization Risks section in the team’s documentation says “fully decentralized, no admin”. You grep the code and find onlyRole(GOVERNANCE) on six functions and onlyRole(EMERGENCY_PAUSER) on three. What do you do? A: This is itself a finding (Medium-to-High depending on impact of the most dangerous privileged function). Two parts: (a) the contract is not fully decentralized — privileged functions exist and someone holds those roles; (b) the documentation is misleading, which has its own user-protection implications because users base risk decisions on these claims. Recommendation: produce the actual Centralization Risks table for the team, require them to either decentralize the powers fully (renounce roles, transfer to timelock, etc.) or correct the documentation, and re-audit after their choice.

14. Week 04 Deliverables

All Lab exercises 1–6 complete; tests passing.
Stretch (Ex. 10.9 — AccessManager) attempted if time permits.
audit/centralization-risks.md written for your VaultV2: includes the role table, worst-case-impact sentences, and a one-paragraph trust-assumptions prose summary.
Master checklist (§11) added to your personal audit checklist file.
One-paragraph reflection: “If I were given an unknown contract tomorrow, how would I produce its Centralization Risks section in <2 hours?“

15. Where this leads

Next week: Tuan-05-Vulnerability-Classes-Part-1. You’ll take the CEI pattern from this week and confront its limits — cross-function reentrancy, cross-contract reentrancy, read-only reentrancy. You’ll add delegatecall and proxy-storage hazards, signature replay, and the unified meta-model that ties them all together: the EVM allows external code to execute in the middle of a state transition the developer assumed was atomic.

The shift in mindset is: this week, you treated access control as a static design problem — who can call what, with what delay. Next week, you treat the same surface as a dynamic problem — what happens when execution leaves your contract and an adversary chooses what comes back.

Pair Week 04’s privilege map with Week 05’s execution-order discipline and you have the foundation that the rest of the course — DeFi, oracles, bridges, governance, AA — all build on.

Last updated: 2026-05-16 See also: Roadmap · References · Tuan-03-Solidity-Foundry-Workflow · Tuan-05-Vulnerability-Classes-Part-1 · Tuan-14-Governance-DAO-Security · Case-Parity-Multisig-2017 · Case-Radiant-Capital-2024

lthieu's notes

Explorer

Tuan-04-Security-Foundations-CEI-AC

Week 04 — Security Foundations: CEI & Access Control

1. Context & Why

1.1 The week’s thesis

1.2 The auditor’s mandate (this week’s deliverable mindset)

1.3 Learning goals

1.4 Primary references

2. The CEI Pattern — Checks, Effects, Interactions

2.1 What CEI is (and why it’s not optional)

2.2 The full picture: where the external call lives

2.3 CEI necessary but not sufficient — preview

2.4 Anti-pattern catalog

2.5 Defense in depth: CEI + ReentrancyGuard + Pull

3. Access Control Patterns

3.1 The single design question

3.2 Ownable — single owner, legacy default

3.3 Ownable2Step — two-step transfer, mandatory floor

3.4 AccessControl — role-based access control (RBAC)

3.5 AccessControlEnumerable — when off-chain visibility matters

3.6 AccessManager — centralized authority for multi-contract protocols (OZ 5.0+)

3.7 Multisig owner (Safe / Gnosis Safe)

3.8 Timelock + governance — delay as a defense

3.9 Pause / Unpause patterns

3.10 Principle of Least Privilege & Separation of Duties

3.11 The forwarder anti-pattern

4. Input Validation

4.1 Zero-address checks

4.2 Parameter range validation

4.3 Array length matching for batch ops

4.4 Enum bounds

4.5 Math sanity

4.6 Validation as documentation

5. Custom Errors vs Require Strings

5.1 The basics

5.2 Why the gas saves

5.3 Audit implications beyond gas

6. Pull over Push Payments

6.1 Why push is dangerous

6.2 The pull pattern

6.3 OZ PullPayment and Escrow

6.4 When push is acceptable

7. Error Handling

7.1 try/catch for external calls

7.2 Revert reason propagation

7.3 assert vs require vs revert

7.4 Handling custom-error reverts via low-level call

8. Storage Layout Hazards in Inheritance

8.1 The slot rule

8.2 Storage gaps (__gap)

8.3 Why diamond inheritance is dangerous

8.4 ERC-7201 namespaced storage (the modern alternative)

9. The “Rug Surface” — Cataloging Centralization Risks

9.1 The mandate

9.2 How to build it

9.3 Severity heuristics

9.4 Real-world templates

9.5 Examples from real audit reports

10. Lab — Harden a Weak Contract

10.1 Lab structure

10.2 Starting contract — WeakVault.sol

10.3 Exercise 1 — Demonstrate CEI violation

10.4 Exercise 2 — Convert Ownable to Ownable2Step, demonstrate why

10.5 Exercise 3 — Add AccessControl with custom role admin

10.6 Exercise 4 — Push → Pull conversion

10.7 Exercise 5 — Storage gap demonstration

10.8 Exercise 6 — Write the “Centralization Risks” appendix

10.9 Stretch — Migrate to AccessManager

10.10 Expected outcomes

11. Anti-pattern Catalog (add to master checklist)

12. Trade-offs and Open Debates

13. Quiz (≥80% to advance)

14. Week 04 Deliverables

15. Where this leads

Graph View

Table of Contents

Backlinks

3.2 `Ownable` — single owner, legacy default

3.3 `Ownable2Step` — two-step transfer, mandatory floor

3.4 `AccessControl` — role-based access control (RBAC)

3.5 `AccessControlEnumerable` — when off-chain visibility matters

3.6 `AccessManager` — centralized authority for multi-contract protocols (OZ 5.0+)

6.3 OZ `PullPayment` and `Escrow`

7.1 `try/catch` for external calls

7.3 `assert` vs `require` vs `revert`

8.2 Storage gaps (`__gap`)

10.2 Starting contract — `WeakVault.sol`

10.4 Exercise 2 — Convert `Ownable` to `Ownable2Step`, demonstrate why

10.5 Exercise 3 — Add `AccessControl` with custom role admin

10.9 Stretch — Migrate to `AccessManager`