Checks while Hacks

This repository contains my ultimate solidity attack vectors compilation.
I will be compiling all solidity attack vectors(the general approach) that I come across with.

inspired from transmisions11/Solcurity

To-Do List

Audit Reports :

Sections

Approach : Contains the general points during auditing.
General Entity : Common questions arise while observing specific term in the smart contract.
Variables : Points related to state variables.
Structs : Points related to structs.
Functions : Points related to functions.
Modifiers : Points related to modifiers.
Code : Some good practices to avoid any vulnerability.
Unexpected outputs : Some unexpected implementations and outputs related to token standards
External Call : Points related to External Call.
Static Call : Points related to Static Call.
Events : Points related to Events.
Contract : Some points related to the whole contract
Project : Best practices during building a project.
Defi : Contains some DeFi related vulnerabilities.
After Transaction : Security issues that can arise after the transaction has been submitted to the mempool.
NFT : Issues specific to NFTs.

Approach

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.
While going through a contract, make sure to write all the assumptions(about external contracts) and expectations(from external contracts) and questions related to it.
Compare the architecture to your mental model. Look into areas that are surprising.
Identify relevant global and state variables, functions, equations that are involved in the contract.
List all the invariants related to them and try to find a way to break them to get a loop hole in the implementation.
Look at areas that interface with external contracts and ensure all assumptions about them are valid.
Try to get what are the possibilities during different states of the contract
- when it is freshly deployed,
- when it has high amount of each token and every combination,
- when it is has some bool value come to true which is changing the state of the contract,
- try to switch between every if and else condition and also try in all ranges of the variables present
- what if I swap all the tokens in the pool
- what if I borrow all the asset tokens
Do a generic line-by-line review of the contracts.
While approaching a function, it is important that we take atleast one pair of input and calculate its output. This hardens our mental model.
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 static analysers and review their output.
Look at related projects and their audits to check for any similar issues or oversights.
Try to figure out as many as expected invariants in the contract after getting its context.
Try to avoid transaction order dependence in the code or find a way to deal with it.
Try to anticipate what will occur when governance turns evil (this may be the case of the RUG PULL, EXIT SCAMS).
Comment the "why" as much as possible.
Comment the "what" if using obscure syntax or writing unconventional code.
Comment explanations + example inputs/outputs next to complex and fixed point math.
Comment explanations wherever optimizations are done, along with an estimate of much gas they save.
Comment explanations wherever certain optimizations are purposely avoided, along with an estimate of much gas they would/wouldn't save if implemented.
We should always note all the privileges that are provided to any role and what actually the role can do, any difference in these two will be a vulnerability.
Its a centralisation attack when there is given power to the owner to control funds of users in any case.

Common questions to ask when we come across any general entity

Will the contract run the same if this entity is removed?
Will this entity be replaced with some alternative code?
Will this entity be used by the admin to do some exploit(making the protocol apparently centralised)?
Is the entity opening a path for arbitrary interaction with the contract?

Variables

When was the variable initialized and how?
Is the visibility set? Can it be more specific such as external, internal, private?
Can it be constant, immutable?
Dealing with the state which is stored off-chain can cause the inconsistency as its not possible to PULL data onchain.
Is the purpose of the variable and other important information documented using natspec?
Can it be packed with an adjacent storage variable? this is applicable to both local, state and calldata variables.
Can it be packed in a struct with more than 1 other variable?
Use full 256 bit types unless packing with other variables.
If it's a public array, is a separate function provided to return the full array?
While trying to delete something from the array, there are two ways : one is to replace that from the last entry and then pop, and another is to delete that index and shift every succeding element backwards by one.
Check that the size of the array to be limited, otherwise it may lead to gas shortage to complete the transaction.
Do the order of the entries in an array matter?
Mappings can be made traversable by the use of linked list(every entry is pointing to the next entry).
Enums can be used instead of seperate and related constants.
Only use private to intentionally prevent child contracts from accessing the variable, prefer internal for flexibility.
Uninitialized local storage variables(variables that take their value from a state variable) can point to unexpected storage locations in the contract, which can lead to intentional or unintentional vulnerabilities, so mark them as memory, calldata and storage as per the requirement.
The variables that store value of the past should also have the functionality to have it removed as well otherwise the gas fee for the operations will be increasing as the variable storing values increase in cases of array as we have to traverse all the previous entries also.
Variables that need to be very precise(number of months/year elapsed) should not get the precision error(as 1.99 month should not be considered as 1 month although 1.99 day can be considered as 1 because of the time error).
Ethereum incentivize the efficient use of storage. When we delete a variable, there is a gas refund that appears in the transaction. The refund is capped to the half of the gas that was spent in the transaction. and before that, it refunds 15e3 for resetting a storage slot and 10e3 for freeing the slot from storage usage.
Always remember that while comparing two uints, don't use a-b > 0, as this will either return true or will revert, as a - b will not be an uint.

Structs

Is a struct necessary? Can the variable be packed raw in storage?
Are its fields packed together (if possible)?
Is the purpose of the struct and all fields documented using natspec?

Functions

Should it be external or internal? For instance, the function not called by the same contract should be marked as external.
Should it be payable?
Do this function need a counter part. Example : Deposit function has Withdraw function.
Can the function be front-runned?
In what conditions the function can be stopped unfortunately.
Can it be combined with another similar function?
If the test coverage is not 100% then also check for the parameters ordering during function calls.
Validate all parameters are within safe bounds, even if the function can only be called by a trusted users.
Always make sure that the argument passed is a valid argument/ behaves as expected in its full range of taking values.
Are the multiple arrays taken have same length?
What if the two arguments provided to be same, i.e. sender == receiver, source = destination etc.
Is the checks before effects pattern followed? (SWC-107)
Is the update before call pattern followed? (Reentrancy) Sometimes even the modifier can not save from reentrancy.
Are the correct modifiers applied, such as onlyOwner/requiresAuth?
Are the modifiers(if more than one) written in function in correct order, because the change in order will change the code?
What if I call the function one time with the value of X and Y times with the value X/Y.
Write down and test invariants about state before a function can run correctly.
Write down and test invariants about the return or any changes to state after a function has run and try to include all edge cases as input.
Take care when naming functions, because people will assume behaviour based on the name.
If a function is intentionally unsafe (to save gas, etc), use an unwieldy name to draw attention to its risk.
Are all arguments, return values, side effects and other information documented using natspec?
Only use private to intentionally prevent child contracts from calling the function, prefer internal for flexibility.
Use virtual if there are legitimate (and safe) instances where a child contract may wish to override the function's behaviour.
Are return values always assigned?, sometimes not assigning values is better.
Try not to use msg.value, after its value has been used as this can cause the loss of funds of the contract. msg.value can be used in case of fees payment which is very small and protocol exclusive.
block.timestamp remains same during a single transaction even if any complex operation is done.
Its better to store the values of state variables in local variables when the state variables are called multiple times, as MLOAD is cheaper than SLOAD. This process is called variable caching.
Try to provide the values of state variables as parameter to internal functions as this will minimize SLOAD which is expensive than CALLDATACOPY.
OnlyOwner function should be marked as payable, this will lower cost for legitimate callers due to avoidance of CALLVALUE, DUP1, JUMPI, REVERT, POP, JUMPDEST. (Practice this iff the security is not sacrificed).
transferFrom function should be a mandatory function in every token implementation if they support arbitrary addresses as there are smart contracts that can only approve the tokens but not initiate the transaction using the transfer function.

Modifiers

Are no storage updates made (except in a reentrancy lock)?
Are external calls avoided?
Is the purpose of the modifier and other important information documented using natspec?
Always remember that modifiers increase the Codesize so use them wisely.
Duplicated require/revert statement should be refactored to a modifier or function.
Best practice to have the nonReentrant modifier come at the first of all the modifiers.

Code

Using SafeMath or 0.8 checked math? (SWC-101)
Are any storage slots read multiple times?
Implementation should be consistent with the documentation, whats written in docs should be implemented in the contract.
Are any unbounded loops/arrays used that can cause DoS? (SWC-128)
Always remember there is present a discontinuity in the formula for division in solidity.
TO avoid division while in an equation, transfer the denominator to the other side to make it a multiplication.
Use block.timestamp only for long intervals. (SWC-116)
Don't use block.number for elapsed time. (SWC-116)
Do not update the length of an array while iterating over it.
Try not to hardcode any parameter related to chain as this can fail after fork.
Don't use blockhash(), etc for randomness. (SWC-120)
Are signatures protected against replay with a nonce and block.chainid? (SWC-121)
Ensure all signatures use EIP-712. (SWC-117 SWC-122)
Output of abi.encodePacked() shouldn't be hashed if using >2 dynamic types. Prefer using abi.encode() in general. (SWC-133)
Don't use any arbitrary data while using the assembly. (SWC-127)
Don't assume a specific ETH balance. (SWC-132)
Private data isn't private, it can be accessed. (SWC-136)
Updating a struct/array in memory won't modify it in storage.
Never shadow state variables. (SWC-119)
Try not to mutate function parameters.
Is calculating a value on the fly cheaper than storing it?
Are all state variables read from the correct contract (master vs. clone)?
Are comparison operators used correctly (>, <, >=, <=), especially to prevent off-by-one errors?
Are logical operators used correctly (==, !=, &&, ||, !), especially to prevent off-by-one errors?
Always multiply before dividing, unless the multiplication could overflow.
Are magic numbers replaced by a constant with an intuitive name?
If the recipient of ETH had a fallback function that reverted, could it cause DoS? (SWC-113)
Use SafeERC20 or check return values safely.
Don't use msg.value if recursive Delegate Calls are possible (like if the contract inherits Multicall/Batchable).
Don't assume msg.sender is always a relevant user.
Don't use assert() unless for fuzzing or formal verification. (SWC-110)
Don't use tx.origin for authorization. (SWC-115)
Don't use address.transfer() or address.send() or limiting the gas in low-level call. Use .call.value(...)("") instead. As these were used to save from reentrancy attacks since using them gives a constant supply of 2300 gas. But they have a problem that if in future, during some hardfork the gas is decreased then this will lead to the failure of the transaction as if the fallback function is a little bit of gas consuming then this will cause the transaction to fail.
Also don't try to limit the gas too close for some common operations, the problem with this is that previous Ethereum hardforks have changed gas costs of commonly used opcodes (for example with EIP-150)
It is also recommended to not use the transfer or send for transfering the native ETH while interacting with a smart contract.
Prefer using safeTransferFrom, safeMint for ERC20 and ERC721 tokens
When using low-level calls, ensure the contract exists before calling.
When calling a function with many parameters, use the named argument syntax.
Do not use assembly for create2. Prefer the modern salted contract creation syntax.
Do not use assembly to access chainId or contract code/size/hash. Prefer the modern Solidity syntax.
Use the delete keyword when setting a variable to a zero value (0, false, "", etc).
Comment explanations wherever unchecked is used, along with an estimate of how much gas it saves (if relevant).
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.
Expressions passed to logical/comparison operators (&&/||/>=/==/etc) should not have side-effects.
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.
Document the reason why a reentrancy lock is necessary whenever it's used with an inline or @dev natspec comment.
When fuzzing functions that only operate on specific numerical ranges use modulo to tighten the fuzzer's inputs (such as x = x % 10000 + 1 to restrict from 1 to 10,000).
Use ternary expressions to simplify branching logic wherever possible.
When operating on more than one address, ask yourself what happens if they're the same.
Can someone without spending other then gas fees change the state of the contract.
Always check the number of loop iterations should be bounded by a small finite number other wise the transaction will run out of gas.
Always check for the return datatype of the called contract function. Example: in ERC20 implementations, the transfer functions are not consistent with the value they return(some return the bool while others revert which can cause problems)
You can always convert bool to revert by using require and revert to bool by using try/catch statement.
Similar to the above, global transfer method reverts while the send gives the bool value which sometimes causes problems
Don't use extcodesize to gain the knowledge of whether the msg.sender is EOA as any contract calling the function while staying in the constructor can easily act as an EOA.
Always try to be consistent with the interface contract otherwise the call will lead to the fallback.
Making a new owner is a crucial thing, so a new function to accept the ownership should be made so that the ownership don't go in the hands of some wrong person or a smart contract which can not do anything.
In Solidity any address can be casted into specific contract, even if the contract at the address is not the one being casted. This can be exploited to hide malicious code.
Don't use ecrecover and signature in general to verify the user as these cause signature malleability.
While using ecrocover, the return value should also be checked to be non-zero(zero states invalid signature).
delete every entry of the mapping before deleting the mapping itself, otherwise the getter function will still work by giving all the mapping values
Look out for signature replay attacks.
Use underscores or constants for number literals for better readability and also for unexpected human error.
Use bytes.concat() instead of abi.encodePacked(), since this is preferred since 0.8.4
Any inconsistency in formula for calculation may cause the loss of the funds and also minting additional funds,
example can be use of Math.min(a, b) which change suddenly when the condition changes.
Don't assume the implementations of ERC20, ERC721 tokens in their contracts, such as decimals, approve functions etc., coding using this assumption will lead to the casting errors
Look for the statements that can be skipped and still takes to the same blockchain state, for example some external call without any return values, some non-relevant require statements.
Try to read all the ERC20 Implementations in scope as their definitions can be different from what is expected.
Round Up should be done while taking the tokens in so that no one can be privileged while depositing a lower amount.
Round down should be done while transfering tokens from protocol to user so that no user can get the same value while having lower deposit.
Use PULL over PUSH while updating the state variables to mitigate the inclusion of blacklisted entities to become active. This also uses gas only whenever necessary
Try not to use the percentage, because it introduces the division and then rounding occurs. Also include a 100% cap while including a percentage.
It is necessary to make the lines in constructor in proper order, this really affect the initial state of the protocol. Example. a function called inside the constructor takes value of an uninitialized variable, hence will fail to give correct output.
A good practice while dealing with nonReentrant modifier. Try not to make the state variable public, instead make a public getter by yourself with a nonReentrant modifier.
Some good practices to save some gas involve :
- using ++i instead of i++
- using calldata to load the info instead of memory
- not equating with boolean inside the if-else statement
- use !=0 instead of > while putting non-zero condition to a uint
- use bytes32 instead of string to declare a string variable
- use bit manipulation instead of doing operation with the powers of 2
- use multiple require statements instead of a single require statement containing the conditions seprated with the && operator
- not use safeMath everywhere as some simple operations like division and multiplication can be done easily without it
- internal functions not used should be removed before deployment to save some gas
- don't assign default values to save gas (example: uint i = 0, bool x = false)
- multiple mapping from same datatype can be then grouped to a single (from first datatype to the struct containing all destination datatypes)
- revert error(); is better than require(error statement);
- Try to use the unchecked to increment and decrement to an inrange number
- Try not to call the full struct if only one or two variables of it are used in the function. Just cache the values of them in a local variable.
- cheap way to store constants is to store them in library as an internal variable.
- better to use uint256 instead of bool for using as a switch
- use e instead writing power of 10 as **
The constants in solidity when assigned to a mathematical expression have a property to calculate its value everytime they are written to give the value. While the immutables don't have this property so this gives immutables a gas saving advantage while assigned to a mathematical expression e.g. 2*10e12
If oracle is accessed using block number(to get historical data), then it should be mandatory to set the values only once per block.
Lower size uints are actually less gas efficient. One exception is when packing the variables.
Better not to use the dynamic array to save some space that would have been filled to store the length of that array.
We can also simulate a transaction in a contract by making a function that simulates what is needed and also reverting on every result but with a custom message for each reason and can then be used in a try and catch statement to be able to have the results of the simulated transaction.
The implementation of the IERC interfaces should be strictly following the standards of ERC, otherwise this will be difficult to integrate this with the other contracts. For example : tokenURI(uint256 tokenId) function in ERC721 should revert(and not just return) if the tokenId is not minted yet.

Unexpected implementations and Outputs from already deployed contracts

Most price feeds use Chainlink as their price feed which sometimes return the values at 8 decimal numbers, so while scaling the output with a general formula using 1e12 or 1e18 will not be applicable.
Some tokens like PAXG, USDT have fee-on-transfer in-built which makes them transfer less tokens than the argument passed. So to get the exact value of transfer tokens we have to fetch the balance of the receiving contract twice(one before transfer and one after) and also a non-reentrant to protect against ERC-777 tokens
Tokens like USDT, KNC have a approval-race-protection mechanism which uses allowance which is either set to 0 or type(uint256).max, at any other value, the safe-approval will revert instead of giving a bool. So we have to use forceApproval Allowance to mitigate this problem and to generalize any token approval in our protocol. USDT also don't have an increaseAllowance() function.
Decimals are not fixed to 18 for all ERC20 implementations, such as GUSD-Gemini Dollar has only 2 decimals.
There are implementations of ERC721 that revert when calling the setApprovalForAll function more than one times, this is because the function has a check require(_tokenOperator[msg.sender][_operator] != _approved). Example is Axie ERC721 Token.
Chainlink's latestRoundData() is used, then there should be a check if the return value indicates old data. Otherwise this could lead to old prices according to the Chainlink documentation. Also if any variable is used to make sure that the data is not outdated, then while using the two different price feeds, we have to make sure that these two price feeds are updated at comparable amounts of time other wise the difference between their update time will lead to unexpected changes.
Different chains have different block mining time which poses a vulnerability when writing the same code for all the chains while relating the number of blocks and the timestamp.
Using solmate safeTransferLib, one should also make a function to check whether the token contract exist or not, because this is not included in that library
A problem with using only approve function but not the increaseAllowance is that, If A approves B 5 tokens and B don't use them, Now, If A approves B 10 tokens to increase the approve value from 5 to 10 tokens, so that B can spend 10 tokens, now B can front run that 10 token transaction to spend both 5 and 10 tokens. So use increaseAllowance, decreaseAllowance instead of approve and similiar for safe versions of those functions.
While receiving arbitrary NFT, extend ERC721Holder from OpenZeppelin to handle the case when the NFT contract is using safeTransferFrom method as this method checks for onERC721Received method present in receiver contract or the reciever is an EOA.
While using transfer or transferFrom to send the arbitrary tokens:
- Some tokens don't revert on failure, like ZRX and EURS
- Some don't return bool value on function call, like USDT, BNB, OMG
- Even the implementation of USDT is different on Polygon and Ethereum.
Chain reorgs is another event for rearrangement of transactions and can even removal of transactions. This event is very common in the chains where the time between consecutive blocks is very less and can reach upto high depths of blocks. Mitigation involves waiting for enough blocks after the transaction has become successful, otherwise reorg can remove that transaction.
The view functions of CURVE price oracle are not locked by the reentrancy modifier, so always check for the reentrancy modifier while using the curve oracle view function.
The cost of withdrawing the ether from Arbitrum to Ethereum is very high since Arbitrum uses a rollup architecture, which requires users to pay gas fees to transfer assets between the rollup and the Ethereum mainnet.
There are the copies of NFTs in different chains after a hardfork is done. This lead to sometimes double value a single entity when the protocol functions cross-chain.
There is no guarantee for no revert of the transaction that is made to get the symbol, decimals for ERC20 if it is not inheriting the IERC20Metadata.sol, and tokenURI for ERC721 if it is not inheriting the IERC721Metadata.sol
Consider supporting old NFTs and tokens before the ERC20 and ERC721 standard arrived. For example ERC20: WETH , ERC721: CryptoPunks, EtherRocks
Solidity version 0.8.13 & 0.8.14 have a security vulnerability related to assembly blocks that write to memory, which are present in ERC20Plugins contract. The issue is fixed in version 0.8.15 and is explained here.
The optimization done by making the functions payable(so no opcode is written to check whether the transaction has any value) also need a withdraw function other wise funds will be stuck. There are implementations of functions in @openzeppelin that are payable but not supposed to paid any value. Such as ERC721AUpgradeable, ERC1967Proxy, BeaconProxy etc. This can be mitigated using a check require(msg.value==0)
WETH contracts differ on different chains: transferFrom will work without allowance on Ethereum chain if the sender is an address that executes the function. But it will revert on some other chains like polygon due to the fact that they always subtract the allowance
Aptos uses 32-byte addresses. similiarly non-EVM chains use the different address sizes, better not to hardcode the address size in case of bridges.

External Calls

Is an external contract call actually needed?
Avoid Delegatecall wherever possible, especially to external (even if trusted) contracts. (SWC-112)
If there is an error, could it cause DoS? Like balanceOf() reverting. (SWC-113)
Would it be harmful if the call reentered into the current function?
Would it be harmful if the call reentered into another function?
Is the result checked and errors dealt with? (SWC-104)
What if it uses all the gas provided?
Could it cause an out-of-gas in the calling contract if it returns a massive amount of data?
If you are calling a particular function, do not assume that success implies that the function exists (phantom functions).
Its best to be stateless while doing an external delegate call.
Always assume that the external call will fail, now code accordingly.
Try avoiding taking arbitrary input or calldata input for a function that does external call which can make the EOA make the calls in the behalf of the contract.
The external calls from a contract can be made to be failed and still be made the function continue if the external call returns a bool, the attacker can just give very enough gas to make the sub-call(call from a contract function to another contract) fail.(Insufficient Gas Griefing)
Always check for the contrat existence before doing a low-level call.
Limit the number of iterations in the for-loop if making an external call due to gas shortage.
Multiple subsequent(one external call is related to the other external call depending on previous one) external calls should be checked to conserve the msg.value.

Static Calls

Is an external contract call actually needed?
Is it actually marked as view in the interface?
If there is an error, could it cause DoS? Like balanceOf() reverting. (SWC-113)
If the call entered an infinite loop, could it cause DoS?
Is this call supporting Reentrancy? Is this call reading the updated values OR outdated values?

Events

Should any fields be indexed?
Is the creator of the relevant action included as an indexed field?
Do not index dynamic types like strings or bytes.
Is when the event emitted and all fields documented using natspec?
Are all users/ids that are operated on in functions that emit the event stored as indexed fields?
Avoid function calls and evaluation of expressions within event arguments. Their order of evaluation is unpredictable.
Events should be made for every important change in state made through the contract, so they can be read off-chain.
Avoid event spamming where there are events emitted even when there is e.g. zero claim amount, zero token transfer, as this will affect off-chain event tracking.
Always remember that events also change the state and also can contain state changing functions as their arguments.

Contract

Use an SPDX license identifier.
A protocol whose fake code is present online which by mistake gets imported by another protocol can be unexpected. This happend while having duplicate npm packeage names but different code inside.
Are events emitted for every storage mutating function?
Check for correct inheritance, keep it simple and linear. (SWC-125)
Interface contracts generally don't contain the function which is internal as internal functions are made to support the public functions in their logic.
Use a receive() external payable function if the contract should accept transferred ETH and don't use if the contract don't accept the ether otherwise the funds could be stucted.
Its sometimes a valid finding if any comment(with a warning) is written and is not implemented in the code as this ia a vulnerability with low likelihood.
Some contracts are not able to call the transfer function but can call the approve function. This makes the need for the transferFrom function to be present in all the token implementations whether erc721, or erc20 or any.
Write down and test invariants about relationships between stored state.
Is the purpose of the contract and how it interacts with others documented using natspec?
The contract should be marked abstract if another contract must inherit it to unlock its full functionality.
Emit an appropriate event for any non-immutable variable set in the constructor that emits an event when mutated elsewhere.
Avoid over-inheritance as it masks complexity and encourages over-abstraction.
Always use the named import syntax to explicitly declare which contracts are being imported from another file.
Group imports by their folder/package. Separate groups with an empty line. Groups of external dependencies should come first, then mock/testing contracts (if relevant), and finally local imports.
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.
Malicious actors can use the Right-To-Left-Override unicode character to force RTL text rendering and confuse users as to the real intent of a contract.
Try to take into account the c3 linearization(MRO(method resolution order)) when inheriting from two contracts that contain same function with different implementations (diamond problem)
The callable functions in a contract are not only the ones visible in the contract code but also the ones which are inherited but are not mentioned in the code itself.
Its a good practice to include the headers
The functions should be grouped in the following order as given in the solidity style guide for the auditing process should be smooth
{ constructor, receive function (if exists), fallback function (if exists), external, public, internal, private, view and pure functions last }
Always check for the presence of the presence of calling function in the contract that is owner of the callee contract's only owner function.
Always look for making an extra function(claim) if there is possibility of the funds to be stuck in the contract or the contract is having a receive or fallback function. This can be seen in the case of airdrops that are generally landed on the protocol contract and a claim function should be made to retrieve them. For example : The fee extraction function is not present in the owner contract.
In the beginning after deployment of the contract, the state variables are easy to manipulate(especially in defi) since there is not much of the funds locked in the contract, and hence not very much of the funds are required to manipulate the state of the contract, this can lead to the contract being more vulnerable in start
If the contract is an implementation of an another protocol, then to maintain the consistency, we should check all the formulas to be same in both. This can happen in the strategy protocols that makes strategy for another defi protocols but lacks giving the users same values or outputs.
While using the proxy, Initialize the contract in the same transaction as initialization needs a call to initialize function.
Using same data feed of two related tokens is vulnerable, e.g. using datafeed for USDC for DAI will be vulnerable as if one depegs, then the other price will also be affected in the protocol.
Is the contract upgradeable? If yes, then are there any storage slots reserved?
Try not to use the hardcoded address everywhere.
Contract should implement a way to take out the left over dust.
Contracts that use WETH instead of ETH(or any two interchangeably used tokens) may have the invariant of ETH balance to be 0 or different logic to handle it. This can be a knob to exploit the invariants related to them.
Is all the information that is stored conserved after any major change is happened. What happens to the state stored in the contracts that are stored in the contract address which is updated after some operation. For example the manager contract address if updated in the engine contract then what happens to all the information that is stored in the manager contract.
EVM reverts if anyone redeploys a contract to the same address, however if the implementation contract contains self destruct logic, then attaackers can redeploy a new contract witt different bytecode to the same address through cloneDeterministic.

Project

Use the right license (you must use GPL if you depend on GPL code, etc).
Unit test everything.
Fuzz test as much as possible.
Use symbolic execution where possible.
Run Slither/Solhint and review all findings.
The coverage for the tests should be 100%
Using timelock and multisig using the governance actions on the protocol is a good way so that a user can monitor what the governance is going to change in the protocol and can take actions according to it before the change has been made.

DeFi

defi has many vulnerabilities outside solidity, so familiarize yourself with the crypto space and its trends includes : structuring to avoid AML/CTF, token inflation, fake trends, smurfing, Interlocking Directorate

Check your assumptions about what other contracts do and return.
Don't mix internal accounting with actual balances.
Don't use spot price from an AMM as an oracle.
Do not trade on AMMs without receiving a price target off-chain or via an oracle.
Use sanity checks to prevent oracle/price manipulation.
Watch out for rebasing tokens. If they are unsupported, ensure that property is documented.
Watch out for ERC-777 tokens. Even a token you trust could preform reentrancy if it's an ERC-777. ERC721 are also vulnerable.
Watch out for fee-on-transfer tokens. If they are unsupported, ensure that property is documented.
Different tokens used in any contract should also have a different price fluctuations, time of oracle data fetching and decimals storages due to different implementations of them.
Watch out for tokens that use too many or too few decimals. Ensure the max and min supported values are documented.
Staking, lending in a protocol is different(even if it seems very same)
Always try to see what a dust amount can do, as the dust amount can make anything to non-zero and somethings that work only for absolute zero variable will fail.
Be careful of relying on the raw token balance of a contract to determine earnings. Contracts which provide a way to recover assets sent directly to them can mess up share price functions that rely on the raw Ether or token balances of an address.
If your contract is a target for token approvals, do not make arbitrary calls from user input.
Always set a minimum deposit balance to revoke the privilege given to people depositing zero amount
One of the best optimisations can be decreasing the impermanent loss(maybe divide the loss among more people since the overall loss can not be decreased as this will affect the price impact on the AMM)
Check out for whether governance given to an EOA has infinite minting or approval power(to avoid rug pull, exit scams, circulating price impact)
Look out for slippage tolerance in Defi DEX protocol, this saves from unexpected results and even protects from front running
There is slippage cap in the functions in AMMs but there should also be the deadline set as the slippage cap gives the person assets in a specified range but the real value of the asset can be changed with time, so even if getting the same amount of token, but not at proper time can lead to bad trade.
The main concern while swapping is getting the expected price, so during very high fluctuations, using slippage in form of percentage or deviation from the current price is not a good idea since during high fluctuations, even inside the deadline the price may be very unexpected, so the best way to use swaps is (deadline + expected price) you want rather slippage percentage or absolute difference from the current price.
Try not to approve the token contracts which have onlyOwner functions which have the power to move the funds.
Watch out what if someone with very much money can do(in cases of auction), in these cases a flashloan attack is likely to happen
Functions without any protection(like onlyOwner) are vulnerable to frontrunning so consider what will happen if they are frontrunned.
Fees is a part of many protocols, watch out for the msg.sender, fee payer, funds receiver as different users.
In general, Treasury, admin, manager are the different entities that can receive the incentives or fees collected, try to differentiate between them.
In case of protocols having subscriptions, unregistered, de-registered, expired entries are also different, these should be acting according to the documentation.
Inflation attack : It is the attack in which the pool is submitted the tokens externally and now the liquidity is very high and the total supply of mint tokens is very low and hence the formula will give the minimum amount to deposit to be very high and hence DOSing for people with low money.
maxSlippage value should not be fixed, because in case of emergency where the price is constantly dropping or increasing, the withdraw function or swap function will revert due to crossing of the maxSlippage. But, at that time the transaction should pass otherwise the funds will be stuck forever as the slippage will never come to low.
In a lending and borrowing protocol, this can be a valid finding if at some point of time, the borrower is freeze to borrow the funds or is limited to borrow comparably less funds but is able and have tokens to give collateral, as this will significantly decrease the yield of the lender.
Watch out for all entry points for a position in a protocol for example in case of a protocol build on uniswap will have two entry points for adding liquidity, one of them is the protocol and another is through the pool. Try to investigate all the entry points and how can an entry points be used for unintended behaviour.
Oracle saving block.timestamp of every transaction in the pool will be vulnerable since if a smart contract doing multiple operation in the pool in a single transaction will result in same timestamp for all of them.
FlashLoan protection is done by using the condition lastTimestamp != block.timestamp, this reduces multiple calls in a single transaction and even in a single block. But this also makes the protocol vulnerable to DOS, since if any valid transaction can be frontrunned, then that transaction can not be included in that block, and multiple attacks of this kind on consecutive blocks will cause DOS. So, an extra front-running protection should be there.
Some protocols give a choice of the withdrawal token but the same value, this actually reduced capital efficiency if there is no proper ratio of all the tokens is maintained, as the ratio will be not equal but the value provided is equal.
It is a good practice for having a partial tokens withdrawal as if the contract have not enough funds, then by the margin of a very small amount, the funds of a user can get stuck.
Good practice is to give the claim authority to the owner or to the recepient themselves.
Some safety factor checks that are included in the protocol during withdrawal, oracle updating, liquidating a position are made to be protected against the flashloan attacks. But they should not be hardcoded as the adverse conditions in the De-Fi can cross the limits and the withdraw function will not work due to the conditions and hence the funds are stuck and the conditions are depleting :(
Always check and confirm as the expected behaviour whether the voting power is weighted according to the token holding or uniformly distributed. IMO it should be weighted.
Governance protocols should be checked between the clash in the priviledges of two different roles so as to have a race condition. And also what are the perks to open functions(public use)
Any extra perk other than the fees for the lender is an opportunity for them to be the borrower themselves as the funds will get returned to them in any case along with those perks.
Better to have a view function or the return variable of the deposit function so that the user know how much they will receive. They don't have to calculate by themselves and something unexpected don't happen.
Just-In-Time is actually doing a transaction not just before but just before, which means that the transaction should be just before the transaction of the victim. This can be used during the distribution of the yield to the LPs on the basis of their percent share in the pool. If a whale deposits a very big amount in the pool Just-in-Time before the distribution reward called by the owner, then the whale will have the great yield. But he don't deserve it since his tokens are never used by the protocol to earn the yield. Now, after receiving the yield, he just takes out his liquidity from the pool. The mitigation for this can be using epochs for batching the users and rewarding them after a certain time period.
Exchanging tokens is not always straight forward(A 🔄 B) either due to lack of the pool between them, or due to the minimization of the overall fees for the exchange(so can go through different path e.g. A 🔄 D 🔄 B), so any protocol fixing a path or the fees will cause a vulnerability.
FlashLoan can be related to a zero duration loan in borrowing and lending protocol if the interest amount is starts from zero, but the collateral will be required to take the loan.
Always take care of the dust amount as it can make the equality to inequality and also makes the system DOS while playing around the corners of the inequality. Example
The receiver of the collateral should not be always same as the recepient of the collateral or not always be specified by the one who pays the loan as this will make any person to pay the debt and take away the collateral as the collateral contains more value.
In case of implementing the timelock in the fast chains such as Arbitrum(blocktime ~= 0.26 seconds), it is infeasible to set the delay in terms of number of blocks.

After Transaction

The transaction data can be seen by anyone reading the mempool, so don't use things like password in the transactions.
While auditing measures related to pre-exploit and post-exploit should be taken since the codebase can not be claimed to be hack proof very easily and hence post-exploit measures should also be included inside the codebase, both should count as a valid finding. So, try to think, what will happen if a contract is exploited, how will it affect the whole DeFi space.

NFT

Any smart contract using NFT contracts as input should also include a function to blacklist NFTs so that anyone can not use NFT contracts as inputs that are theft in the past.
NFT SVGs can be used for XSS-Attack(Cross site scripting) if can be changeable by the attacker. So to visually misguide the users.

0xprinc/checks-while-hacks