✅Notes

Security Pitfalls & Best Practices 101

Security Pitfalls & Best Practices 101

• Divide before multiply: Performing multiplication before division is generally better to avoid loss of precision because Solidity integer division might truncate. (see here)

• Transaction order dependence: Race conditions can be forced on specific Ethereum transactions by monitoring the mempool. For example, the classic ERC20 approve() change can be front-run using this method. Do not make assumptions about transaction order dependence. (see here)

• ERC20 approve() race condition: Use safeIncreaseAllowance() and safeDecreaseAllowance() from OpenZeppelin’s SafeERC20 implementation to prevent race conditions from manipulating the allowance amounts. (see here)

• Signature malleability: The ecrecover function is susceptible to signature malleability which could lead to replay attacks. Consider using OpenZeppelin’s ECDSA library. (see here, here and here)

• Dangerous strict equalities: Use of strict equalities with tokens/Ether can accidentally/maliciously cause unexpected behavior. Consider using >= or <= instead of == for such variables depending on the contract logic. (see here) -> Damn Vulnerable DeFi Unstoppable

• Locked Ether: Contracts that accept Ether via payable functions but without withdrawal mechanisms will lock up that Ether. Remove payable attribute or add withdraw function. (see here)

• Deleting a mapping within a struct: Deleting a struct that contains a mapping will not delete the mapping contents which may lead to unintended consequences. (see here)

• Account existence check for low-level calls: Low-level calls call/delegatecall/staticcall return true even if the account called is non-existent (per EVM design). Account existence must be checked prior to calling if needed. (see here)

• Calls inside a loop: Calls to external contracts inside a loop are dangerous (especially if the loop index can be user-controlled) because it could lead to DoS if one of the calls reverts or execution runs out of gas. Avoid calls within loops, check that loop index cannot be user-controlled or is bounded. (see here)

• DoS with block gas limit: Programming patterns such as looping over arrays of unknown size may lead to DoS when the gas cost of execution exceeds the block gas limit. (see here)

• Function default visibility: Functions without a visibility type specifier are public by default in solc < 0.5.0. This can lead to a vulnerability where a malicious user may make unauthorized state changes. solc >= 0.5.0 requires explicit function visibility specifiers. (see here)

• Hash collisions with multiple variable length arguments: Using abi.encodePacked() with multiple variable length arguments can, in certain situations, lead to a hash collision. Do not allow users access to parameters used in abi.encodePacked(), use fixed length arrays or use abi.encode(). (see here and here)

• Malleability risk from dirty high order bits: Types that do not occupy the full 32 bytes might contain “dirty higher order bits” which does not affect operation on types but gives different results with msg.data. (see here)

• Unprotected initializers in proxy-based upgradeable contracts: Proxy-based upgradeable contracts need to use public initializer functions instead of constructors that need to be explicitly called only once. Preventing multiple invocations of such initializer functions (e.g. via initializer modifier from OpenZeppelin’s Initializable library) is a must. (see here and here)

• Initializing state-variables in proxy-based upgradeable contracts: This should be done in initializer functions and not as part of the state variable declarations in which case they won’t be set. (see here)

• State variables in proxy-based upgradeable contracts: The declaration order/layout and type/mutability of state variables in such contracts should be preserved exactly while upgrading to prevent critical storage layout mismatch errors. (see here)

• Function ID collision between proxy/implementation in proxy-based upgradeable contracts: Malicious proxy contracts may exploit function ID collision to invoke unintended proxy functions instead of delegating to implementation functions. Check for function ID collisions. (see here and here)