Solidity security: the patterns worth internalising

I had a smart contract audited for the first time in mid-2023. The audit report came back with three medium-severity findings and one high. None of them were novel attack vectors. All of them were well-known patterns that I knew about in theory but had not applied consistently.

The experience convinced me that security in Solidity isn't about knowing every possible attack. It's about internalising a small set of patterns so thoroughly that you apply them reflexively, the way a surgeon sterilises instruments without thinking about why.

Reentrancy and checks-effects-interactions

Reentrancy is the most discussed vulnerability in Solidity. An external call gives control to another contract, which can call back into your contract before the first call finishes.

The checks-effects-interactions pattern prevents it:

function withdraw(uint256 amount) external {
    // Checks
    require(balances[msg.sender] >= amount, "Insufficient balance");

    // Effects (state changes before external interaction)
    balances[msg.sender] -= amount;

    // Interactions (external call last)
    (bool success, ) = msg.sender.call{value: amount}("");
    require(success, "Transfer failed");
}

The order is non-negotiable. State changes must happen before external calls. If the external call re-enters your function, the state has already been updated and the check will fail.

For additional safety, OpenZeppelin's ReentrancyGuard provides a nonReentrant modifier that blocks reentrant calls entirely:

import "@openzeppelin/contracts/security/ReentrancyGuard.sol";

contract Vault is ReentrancyGuard {
    function withdraw(uint256 amount) external nonReentrant {
        // Even if the pattern is applied incorrectly,
        // the modifier prevents re-entry
    }
}

Use both. The pattern prevents reentrancy by design. The modifier catches it if you make a mistake in the pattern.

Access control

OpenZeppelin provides two access control contracts: Ownable and AccessControl.

Ownable is simple. One address is the owner, and certain functions are restricted to the owner. Use it when you have a single admin.

import "@openzeppelin/contracts/access/Ownable.sol";

contract Treasury is Ownable {
    function emergencyWithdraw() external onlyOwner {
        payable(owner()).transfer(address(this).balance);
    }
}

AccessControl supports multiple roles with granular permissions. Use it when different addresses need different levels of access.

import "@openzeppelin/contracts/access/AccessControl.sol";

contract ManagedVault is AccessControl {
    bytes32 public constant MANAGER_ROLE = keccak256("MANAGER_ROLE");
    bytes32 public constant WITHDRAWER_ROLE = keccak256("WITHDRAWER_ROLE");

    constructor() {
        _grantRole(DEFAULT_ADMIN_ROLE, msg.sender);
    }

    function setLimit(uint256 limit) external onlyRole(MANAGER_ROLE) {
        // Only managers
    }

    function withdraw(uint256 amount) external onlyRole(WITHDRAWER_ROLE) {
        // Only withdrawers
    }
}

The decision between them is about the access model, not the contract complexity. A complex contract with one admin uses Ownable. A simple contract with multiple roles uses AccessControl.

tx.origin vs msg.sender

tx.origin is the externally-owned account (EOA) that initiated the transaction. msg.sender is the address that directly called the current contract. When a user calls your contract directly, they're the same. When a contract calls your contract, they differ.

Using tx.origin for authorization creates a phishing attack vector:

1. Attacker deploys a malicious contract
2. Attacker tricks the contract owner into calling the malicious contract (via a phishing link, a misleading dApp)
3. The malicious contract calls the vulnerable contract
4. The vulnerable contract checks tx.origin, which is the real owner
5. Access is granted

// VULNERABLE
function transferOwnership(address newOwner) external {
    require(tx.origin == owner, "Not owner");  // DO NOT DO THIS
    owner = newOwner;
}

// SECURE
function transferOwnership(address newOwner) external {
    require(msg.sender == owner, "Not owner");
    owner = newOwner;
}

There's no legitimate use case for tx.origin in authorization. If you see it in a contract, it's a bug.

Integer arithmetic after 0.8

Solidity 0.8 added built-in overflow and underflow checking. Arithmetic operations revert on overflow instead of wrapping around. This eliminated an entire class of vulnerabilities.

But edge cases remain:

// Division truncates toward zero
uint256 result = 5 / 3;  // result = 1, not 1.666...

// Casting between different sizes can truncate
uint256 big = 300;
uint8 small = uint8(big);  // small = 44 (300 % 256)

// Multiplication before division for precision
uint256 price = (amount * rate) / PRECISION;  // Better
uint256 price = (amount / PRECISION) * rate;  // Loses precision

The multiplication-before-division pattern matters for financial calculations. Dividing first loses the fractional part, then multiplying amplifies the error.

Pull vs push payments

Push payments: your contract sends ETH to an address. Pull payments: your contract records a balance, and the recipient withdraws it.

Push payments are dangerous because the recipient is an unknown address that could be a contract. That contract's receive() function could revert, blocking your entire transaction. Or it could consume excessive gas. Or it could re-enter your contract.

// DANGEROUS: push payment in a loop
function distributeRewards(address[] calldata recipients, uint256[] calldata amounts) external {
    for (uint256 i = 0; i < recipients.length; i++) {
        // If any recipient reverts, the entire distribution fails
        payable(recipients[i]).transfer(amounts[i]);
    }
}

// SAFE: pull payment pattern
mapping(address => uint256) public pendingWithdrawals;

function distributeRewards(address[] calldata recipients, uint256[] calldata amounts) external {
    for (uint256 i = 0; i < recipients.length; i++) {
        pendingWithdrawals[recipients[i]] += amounts[i];
    }
}

function withdraw() external nonReentrant {
    uint256 amount = pendingWithdrawals[msg.sender];
    require(amount > 0, "Nothing to withdraw");
    pendingWithdrawals[msg.sender] = 0;
    (bool success, ) = msg.sender.call{value: amount}("");
    require(success, "Transfer failed");
}

With pull payments, each recipient is responsible for their own withdrawal. A reverting recipient doesn't affect anyone else.

The audit findings

The three medium-severity findings in my audit were all variations of these patterns:

1. A function that could be called by any address, which should have been restricted to a specific role
2. A state update that happened after an external call rather than before
3. A push payment in a distribution function

The high-severity finding was a function that used tx.origin for a secondary authorization check that I had added as a "convenience" without thinking about the implications.

All four were fixed in under an hour. The audit cost significantly more than the fixes. But the audit found them, and shipping those vulnerabilities to mainnet would have been irrecoverable.

The patterns aren't complex. They're simple rules applied consistently. The hard part is applying them every time, not just when you remember.