Security Pitfalls

// VULNERABLE
function withdraw(uint256 amount) external {
    require(balances[msg.sender] >= amount);
    (bool ok, ) = msg.sender.call{value: amount}(""); // callee re-enters here
    require(ok);
    balances[msg.sender] -= amount; // too late — already drained
}

// FIXED: Checks-Effects-Interactions
function withdraw(uint256 amount) external nonReentrant {
    require(balances[msg.sender] >= amount, "Insufficient");
    balances[msg.sender] -= amount; // EFFECT before external call
    (bool ok, ) = payable(msg.sender).call{value: amount}("");
    require(ok, "Transfer failed");
}

uint256 private _status = 1;

modifier nonReentrant() {
    require(_status != 2, "Reentrant");
    _status = 2;
    _;
    _status = 1;
}

// Transient storage version (0.8.24, EIP-1153 — cheaper)
uint256 transient private _entered;
modifier nonReentrant() {
    require(_entered == 0);
    _entered = 1;
    _;
}

// View functions can also be re-entered. If another protocol reads your
// "view" state mid-transaction, it can see inconsistent values.
// Solution: use nonReentrant on state-changing functions,
// or price oracles that are reentrancy-safe.

// Safe since 0.8.0
uint256 x = type(uint256).max;
x += 1; // reverts — Panic(0x11)

// If you use unchecked, overflow silently wraps:
unchecked { x += 1; } // 0 — be certain this is intentional

// VULNERABLE — anyone can call
function setOwner(address newOwner) external {
    owner = newOwner;
}

// FIXED
modifier onlyOwner() {
    require(msg.sender == owner, "Not owner");
    _;
}
function setOwner(address newOwner) external onlyOwner {
    require(newOwner != address(0));
    owner = newOwner;
}

// OpenZeppelin AccessControl or custom:
mapping(bytes32 => mapping(address => bool)) private _roles;

bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");

modifier onlyRole(bytes32 role) {
    require(_roles[role][msg.sender], "Missing role");
    _;
}

function grantRole(bytes32 role, address account) external onlyRole(DEFAULT_ADMIN_ROLE) {
    _roles[role][account] = true;
}

// VULNERABLE — tx.origin is the original EOA, not msg.sender
function restricted() external {
    require(tx.origin == owner, "Not owner");
    // A malicious contract can trick owner into calling it,
    // which then calls this function — tx.origin is still owner
}

// FIXED — use msg.sender
function restricted() external {
    require(msg.sender == owner, "Not owner");
}

// Pattern: commit-reveal scheme
mapping(address => bytes32) public commitments;

function commit(bytes32 hash) external {
    commitments[msg.sender] = hash;
}

function reveal(uint256 value, bytes32 salt) external {
    require(commitments[msg.sender] == keccak256(abi.encode(value, salt)));
    // process value — it was hidden until now
    delete commitments[msg.sender];
}

// Pattern: slippage protection
function swap(uint256 amountIn, uint256 minAmountOut) external {
    uint256 out = _swap(amountIn);
    require(out >= minAmountOut, "Slippage too high");
}

// VULNERABLE — same signature reusable across chains/contracts
function execute(bytes32 hash, uint8 v, bytes32 r, bytes32 s) external {
    address signer = ecrecover(hash, v, r, s);
    require(signer == owner);
    _doAction();
}

// FIXED — include chain ID, contract address, nonce (EIP-712)
mapping(address => uint256) public nonces;

bytes32 constant DOMAIN_SEPARATOR = keccak256(abi.encode(
    keccak256("EIP712Domain(string name,uint256 chainId,address verifyingContract)"),
    keccak256("MyContract"),
    block.chainid,
    address(this)
));

function executeWithSig(
    address user,
    uint256 nonce,
    bytes32 action,
    uint8 v, bytes32 r, bytes32 s
) external {
    require(nonce == nonces[user]++, "Invalid nonce");
    bytes32 digest = keccak256(abi.encodePacked(
        "\x19\x01",
        DOMAIN_SEPARATOR,
        keccak256(abi.encode(user, nonce, action))
    ));
    require(ecrecover(digest, v, r, s) == user, "Bad signature");
    _execute(action);
}

// VULNERABLE — ignores failure
addr.call{value: 1 ether}("");

// FIXED
(bool ok, ) = addr.call{value: 1 ether}("");
require(ok, "Transfer failed");

// VULNERABLE — unbounded loop can hit block gas limit
function payAll() external {
    for (uint i; i < recipients.length; ++i) {
        payable(recipients[i]).transfer(shares[i]); // can fail + reverts all
    }
}

// FIXED — pull payment pattern
mapping(address => uint256) public owed;

function claim() external {
    uint256 amount = owed[msg.sender];
    owed[msg.sender] = 0;
    payable(msg.sender).transfer(amount);
}

// VULNERABLE — external call in loop, one failure blocks all
// FIXED — use try/catch or pull pattern per recipient

// VULNERABLE — spot price from AMM (manipulable in same tx via flash loans)
function getPrice() internal view returns (uint256) {
    (uint112 r0, uint112 r1,) = IUniswapV2Pair(pool).getReserves();
    return uint256(r1) * 1e18 / uint256(r0);
}

// FIXED — use TWAP, Chainlink, or multi-block average
function getTWAP() internal view returns (uint256) {
    // Use Uniswap V3 OracleLibrary.consult() or Chainlink AggregatorV3Interface
}

// delegatecall runs implementation code in THIS contract's storage
// Storage layout MUST match between proxy and implementation

// VULNERABLE — uninitialized proxy allows takeover
contract Proxy {
    address public implementation; // slot 0
    // if implementation has owner at slot 0 too: delegatecall can overwrite!
}

// FIXED — use EIP-1967 unstructured storage slots
bytes32 constant IMPL_SLOT = bytes32(uint256(keccak256("eip1967.proxy.implementation")) - 1);

// VULNERABLE assumption
require(address(this).balance == totalDeposited);

// FIXED — track deposits separately
uint256 public totalDeposited;

function deposit() external payable {
    totalDeposited += msg.value;
}

// Division truncates in Solidity — rounding errors accumulate
uint256 share = totalRewards * userBalance / totalSupply; // can round down

// Round in favor of the protocol (never user)
// Use WAD math (1e18 scaling) for percentages
uint256 WAD = 1e18;
uint256 fee = amount * feeBps / 10_000; // 30 BPS = 0.3%

// Avoid division before multiplication
// BAD:  (a / b) * c   — loses precision
// GOOD: a * c / b     — multiply first