Skip to content
This repository has been archived by the owner on Feb 24, 2023. It is now read-only.

Commit

Permalink
Add tokens integration checklist (#31)
Browse files Browse the repository at this point in the history
* Split the development guidelines into multiple files.
Add ToC

* Add token integration checklist
  • Loading branch information
montyly authored Aug 5, 2020
1 parent 7f7e6f6 commit 463c73c
Show file tree
Hide file tree
Showing 2 changed files with 54 additions and 0 deletions.
1 change: 1 addition & 0 deletions development-guidelines/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

53 changes: 53 additions & 0 deletions development-guidelines/tokens_integration.md
Original file line number Diff line number Diff line change
@@ -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.

0 comments on commit 463c73c

Please sign in to comment.