Opinionated security and code quality standard for Solidity smart contracts. Based off work by BoringCrypto.
- Read the project's docs, specs, and whitepaper to understand what the smart contracts are meant to do.
- Construct a mental model of what you expect the contracts to look like before checking out the code.
- Glance over the contracts to get a sense of the project's architecture. Tools like Surya can come in handy.
- Compare the architecture to your mental model. Look into areas that are suprising.
- Create a threat model and make a list of theoretical high level attack vectors.
- Look at areas that can do value exchange. Especially functions like transfer, transferFrom, send, call, delegatecall, and selfdestruct. Walk backward from them to ensure they are secured properly.
- Look at areas that interface with external contracts and ensure all assumptions about them are valid like share price only increases, etc.
- Do a generic line-by-line review of the contracts.
- Do another review from the perspective of every actor in the threat model.
- Glance over the project's tests + code coverage and look deeper at areas lacking coverage.
- Run tools like Slither/Solhint and review their output.
- Look at related projects and their audits to check for any similar issues or oversights.
V1
- Can it beinternal
?V2
- Can it beconstant
?V3
- Can it beimmutable
?V4
- Is its visbility set? (SWC-108)V5
- Is the purpose of the variable and other important information documented using natspec?V6
- Can it be packed with an adjacent storage variable?V7
- Can it be packed in a struct with more than 1 other variable?V8
- Use full 256 bit types unless packing with other variables.V9
- If it's a public array, is a seperate function provided to return the full array?V10
- Only useprivate
to intentionally prevent child contracts from accessing the variable, preferinternal
for flexibility.
S1
- Is a struct necessary? Can the variable be packed raw in storage?S2
- Are its fields packed together (if possible)?S3
- Is the purpose of the struct and all fields documented using natspec?
F1
- Can it beexternal
?F2
- Should it beinternal
?F3
- Should it bepayable
?F4
- Can it be combined with another similar function?F5
- Validate all parameters are within safe bounds bounds, even if the function can only be called by a trusted user.F6
- Is the checks before effects pattern followed? (SWC-107)F7
- Check for front-running possibilities, such as the approve function. (SWC-114)F8
- Is insufficient gas griefing possible? (SWC-126)F9
- Are the correct modifiers applied, such asonlyOwner
/requiresAuth
?F10
- Are return values are always assigned?F11
- Write down and test invariants about state before a function can run correctly.F12
- Write down and test invariants about the return or any changes to state after a function has run.F13
- Take care when naming functions, because people will assume behavior based on the name.F14
- If a function is intentionally unsafe (to save gas, etc), use an unwieldy name to draw attention to its risk.F15
- Are all arguments, return values, side effects and other information documented using natspec?F16
- If the function allows operating on another user in the system, do not assumemsg.sender
is the user being operated on.F17
- If the function requires the contract be in an uninitialized state, check an explicitinitialized
variable. Do not useowner == address(0)
or other similar checks as substitutes.F18
- Only useprivate
to intentionally prevent child contracts from calling the function, preferinternal
for flexibility.F19
- Usevirtual
if there are legitimate (and safe) instances where a child contract may wish to override the function's behavior.
M1
- Are no storage updates made (except in a reentrancy lock)?M2
- Are external calls avoided?M3
- Is the purpose of the modifier and other important information documented using natspec?
C1
- Using SafeMath or 0.8 checked math? (SWC-101)C2
- Are any storage slots read multiple times?C3
- Are any unbounded loops/arrays used that can cause DoS? (SWC-128)C4
- Useblock.timestamp
only for long intervals. (SWC-116)C5
- Don't use block.number for elapsed time. (SWC-116)C7
- Avoid delegatecall wherever possible, especially to external (even if trusted) contracts. (SWC-112)C8
- Do not update the length of an array while iterating over it.C9
- Don't useblockhash()
, etc for randomness. (SWC-120)C10
- Are signatures protected against replay with a nonce andblock.chainid
(SWC-121)C11
- Ensure all signatures use EIP-712. (SWC-117 SWC-122)C12
- Output ofabi.encodePacked()
shouldn't be hashed if using >2 dynamic types. (SWC-133)C13
- Careful with assembly, don't use any arbitrary data. (SWC-127)C14
- Don't assume a specific ETH balance. (SWC-132)C15
- Avoid insufficient gas griefing. (SWC-126)C16
- Private data isn't private. (SWC-136)C17
- Updating a struct/array in memory won't modify it in storage.C18
- Never shadow state variables. (SWC-119)C19
- Do not mutate function parameters.C20
- Is calculating a value on the fly cheaper than storing it?C21
- Are all state variables read from the correct contract (master vs. clone)?C22
- Are comparison operators used correctly (>
,<
,>=
,<=
), especially to prevent off-by-one errors?C23
- Are logical operators used correctly (==
,!=
,&&
,||
,!
), especially to prevent off-by-one errors?C24
- Always multiply before dividing, unless the multiplication could overflow.C25
- Are magic numbers replaced by a constant with an intuitive name?C26
- If the recipient of ETH had a fallback function that reverted, could it cause DoS? (SWC-113)C27
- Use SafeERC20 or check return values safely.C28
- Don't usemsg.value
in a loop.C29
- Don't usemsg.value
if recursive delegatecalls are possible (like if the contract inheritsMulticall
/Batchable
).C30
- Don't assumemsg.sender
is always a relevant user.C31
- Don't useassert()
unless for fuzzing or formal verification. (SWC-110)C32
- Don't usetx.origin
for authorization, (SWC-115)C33
- Don't useaddress.transfer()
oraddress.send()
. Use.call.value(...)("")
instead. (SWC-134)C34
- When using low-level calls, ensure the contract exists before calling.C35
- When calling a function with many parameters, use the named argument syntax.C36
- Do not use assembly for create2. Prefer the modern salted contract creation syntax.C37
- Do not use assembly to access chainif or contract code/size/hash. Prefer the modern Solidity syntax.C38
- Use thedelete
keyword when setting a variable to a zero value (0
,false
,""
, etc).C39
- Comment the "why" as much as possible.C40
- Comment the "what" if using obscure syntax or writing unconventional code.C41
- Comment explanations + example inputs/outputs next to complex and fixed point math.C42
- Comment explanations wherever optimizations are done, along with an estimate of much gas they save.C43
- Comment explanations wherever certian optimizations are purposely avoided, along with an estimate of much gas they would/wouldn't save if implemented.C44
- Useunchecked
blocks where overflow/underflow is impossible, or where an overflow/underflow is unrealistic on human timescales (counters, etc). Comment explanations whereverunchecked
is used, along with an estimate of how much gas it saves (if relevant).C45
- Do not depend on Solidity's arithmetic operator precedence rules. In addition to the use of parentheses to override default operator precedence, parentheses should also be used to emphasise it.C46
- Expressions passed to logical/comparsion operators (&&
/||
/>=
/==
/etc) should not have side-effects.C47
- When incrementing/decrementing a value, use++
/--
respectively to be explicit.C48
- Wherever arithmetic operations are performed that could result in precision loss, ensure it benefits the right actors in the system, and document it with comments.
X1
- Is an external contract call actually needed?X2
- If there is an error, could it cause DoS? LikebalanceOf()
reverting. (SWC-113)X3
- Would it be harmful if the call reentered into the current function?X4
- Would it be harmful if the call reentered into the another function?X5
- Is the result checked and errors dealt with? (SWC-104)X6
- What if it uses all the gas provided?
S1
- Is an external contract call actually needed?S2
- Is it actually marked as view in the interface?S3
- If there is an error, could it cause DoS? LikebalanceOf()
reverting. (SWC-113)S4
- If the call entered an infinite loop, could it cause DoS?
E1
- Should any fields be indexed?E2
- Is the creator of the relevant action included as an indexed field?E3
- Do not index dynamic types like strings or bytes.E4
- Is the when the event emitted and all fields documented using natspec?E5
- Are all users/ids that are opreated on in functions that emit the event stored as indexed fields?
T1
- Use an SPDX license identifier.T2
- Are events emitted for every storage mutating function?T3
- Check for correct inheritance, keep it simple and linear. (SWC-125)T4
- Use areceive() external payable
function if the contract should accept transferred ETH.T5
- Write down and test invariants about relationships between stored state.T6
- Is the purpose of the contract and how it interacts with others documented using natspec?T7
- The contract should be markedabstract
if another contract must inherit it to unlock its full functionality.T8
- Emit an appropriate event for any non-immutable variable set in the constructor that emits an event when mutated elsewhere.T9
- Avoid over-inheritance as it masks complexity and encourages over-abstraction.T10
- Always use the named import syntax to explictly declare which contracts are being imported from another file.T11
- Group imports by their folder/package. Seperate groups with an empty line. Groups of external dependencies should come first, then mock/testing contracts (if relevant), and finally local imports.T12
- Summarize the purpose and functionality of the contract with a@notice
natspec comment. Document how the contract interacts with other contracts inside/outside the project in a@dev
natspec comment.
P1
- Use the right license (you must use GPL if you depend on GPL code, etc).P2
- Unit test everything.P3
- Fuzz test as much as possible.P4
- Use the SMTChecker to prove invariants.P5
- Run Slither/Solhint and review all findings.
D1
- Check your assumptions about what other contracts do and return.D2
- Don't mix internal accounting with actual balances.D3
- Be careful of relying on the raw token balance of a contract to determine earnings.D4
- Don't use spot price from an AMM as an oracle.D5
- Use sanity checks to prevent oracle/price manipulation.D6
- Do not trade on AMMs without receiving a price target off-chain or via an oracle.D7
- Watch out for ERC-777 tokens. Even a token you trust could preform reentrancy if it's an ERC-777.D8
- Watch out for fee-on-transfer tokens. If they are unsupported, ensure that property is documented.D9
- Watch out for rebasing tokens. If they are unsupported, ensure that property is documented.D10
- Watch out for tokens that use too many or too few decimals. Ensure the max and min supported values are documented.