diff --git a/development-guidelines/README.md b/development-guidelines/README.md index 9852852..d40a5ea 100644 --- a/development-guidelines/README.md +++ b/development-guidelines/README.md @@ -3,4 +3,5 @@ List of smart contract development best practices - [Development Guidelines](./development.md): What are the high-level development best-practices - [Implementation Guidelines](./implementation.md): How to implement clean code - [Post-development Guidelines](./post_development.md): What process to follow after development +- [Tokens Integration Checklist](./tokens_integration.md): What to check when interacting with arbitrary tokens diff --git a/development-guidelines/tokens_integration.md b/development-guidelines/tokens_integration.md new file mode 100644 index 0000000..3302a75 --- /dev/null +++ b/development-guidelines/tokens_integration.md @@ -0,0 +1,53 @@ +# Token Integration Checklist + +The following checklist provides recommendations when interacting with arbitrary tokens. +Every unchecked item should be justified and its associated risks understood. + +For convenience, all Slither [utilities](https://github.com/crytic/slither) can be run directly on a token address, such as: +```bash +slither-check-erc 0xdac17f958d2ee523a2206206994597c13d831ec7 TetherToken +``` + +## General Security Considerations +- [ ] **The contract has a security review.** Avoid interacting with contracts that lack a security review. Check the length of the assessment (aka “level of effort”), the reputation of the security firm, and the number and severity of the findings. +- [ ] **You have contacted the developers.** You may need to alert their team to an incident. Look for appropriate contacts on [blockchain-security-contacts](https://github.com/crytic/blockchain-security-contacts). +- [ ] **They have a security mailing list for critical announcements.** Their team should advise users (like you!) when critical issues are found or when upgrades occur. + +## ERC Conformity + +Slither includes a utility, [slither-check-erc](https://github.com/crytic/slither/wiki/ERC-Conformance), that reviews the conformance of a token to many related ERC standards. Use slither-check-erc to review that: + +- [ ] **Transfer and transferFrom return a boolean.** Several tokens do not return a boolean on these functions. As a result, their calls in the contract might fail. +- [ ] **The name, decimals, and symbol functions are present if used.** These functions are optional in the ERC20 standard and might not be present. +- [ ] **Decimals returns a uint8.** Several tokens incorrectly return a uint256. If this is the case, ensure the value returned is below 255. +- [ ] **The token mitigates the known [ERC20 race condition](https://github.com/ethereum/EIPs/issues/20#issuecomment-263524729).** The ERC20 standard has a known ERC20 race condition that must be mitigated to prevent attackers from stealing tokens. +- [ ] **The token is not an ERC777 token and has no external function call in transfer and transferFrom.** External calls in the transfer functions can lead to reentrancies. + +Slither includes a utility, [slither-prop](https://github.com/crytic/slither/wiki/Property-generation), that generates unit tests and security properties that can discover many common ERC flaws. Use slither-prop to review that: + +- [ ] **The contract passes all unit tests and security properties from slither-prop.** Run the generated unit tests, then check the properties with [Echidna](https://github.com/crytic/echidna) and [Manticore](https://manticore.readthedocs.io/en/latest/verifier.html). + +Finally, there are certain characteristics that are difficult to identify automatically. Review for these conditions by hand: + +- [ ] **Transfer and transferFrom should not take a fee.** Deflationary tokens can lead to unexpected behavior. +- [ ] **Potential interest earned from the token is taken into account.** Some tokens distribute interest to token holders. This interest might be trapped in the contract if not taken into account. + +## Contract Composition +- [ ] **The contract avoids unneeded complexity.** The token should be a simple contract; a token with complex code requires a higher standard of review. Use Slither’s [human-summary printer](https://github.com/crytic/slither/wiki/Printer-documentation#human-summary) to identify complex code. +- [ ] **The contract uses SafeMath.** Contracts that do not use SafeMath require a higher standard of review. Inspect the contract by hand for SafeMath usage. +- [ ] **The contract has only a few non–token-related functions.** Non–token-related functions increase the likelihood of an issue in the contract. Use Slither’s [contract-summary printer](https://github.com/crytic/slither/wiki/Printer-documentation#contract-summary) to broadly review the code used in the contract. + +## Owner privileges +- [ ] **The token is not upgradeable.** Upgradeable contracts might change their rules over time. Use Slither’s [human-summary printer](https://github.com/crytic/slither/wiki/Printer-documentation#contract-summary) to determine if the contract is upgradeable. +- [ ] **The owner has limited minting capabilities.** Malicious or compromised owners can abuse minting capabilities. Use Slither’s [human-summary printer](https://github.com/crytic/slither/wiki/Printer-documentation#contract-summary) to review minting capabilities, and consider manually reviewing the code. +- [ ] **The token is not pausable.** Malicious or compromised owners can trap contracts relying on pausable tokens. Identify pauseable code by hand. +- [ ] **The owner cannot blacklist the contract.** Malicious or compromised owners can trap contracts relying on tokens with a blacklist. Identify blacklisting features by hand. +- [ ] **The team behind the token is known and can be held responsible for abuse.** Contracts with anonymous development teams, or that reside in legal shelters should require a higher standard of review. + +## Token Scarcity +Reviews for issues of token scarcity requires manual review. Check for these conditions: + +- [ ] **No user owns most of the supply.** If a few users own most of the tokens, they can influence operations based on the token's repartition. +- [ ] **The total supply is sufficient.** Tokens with a low total supply can be easily manipulated. +- [ ] **The tokens are located in more than a few exchanges.** If all the tokens are in one exchange, a compromise of the exchange can compromise the contract relying on the token. +* [ ] **Users understand the associated risks of large funds or flash loans.** Contracts relying on the token balance must carefully take in consideration attackers with large funds or attacks through flash loans.