diff --git a/development-guidelines/README.md b/development-guidelines/README.md index 84168f0f..07052b34 100644 --- a/development-guidelines/README.md +++ b/development-guidelines/README.md @@ -1,6 +1,6 @@ -List of smart contract development best practices +List of Best Practices for Smart Contract Development -- [High-level best practices](./guidelines.md): High-level best-practices for all smart contracts -- [Token integration checklist](./token_integration.md): What to check when interacting with arbitrary tokens -- [Incident Response Recommendations](./incident_response.md): Guidelines on how to formulate an incident response plan -- [Secure development workflow](./workflow.md): A rough, high-level process to follow while you write code +- [High-Level Best Practices](./guidelines.md): Essential high-level best practices for all smart contracts +- [Token Integration Checklist](./token_integration.md): Important aspects to consider when interacting with various tokens +- [Incident Response Recommendations](./incident_response.md): Guidelines on establishing an effective incident response plan +- [Secure Development Workflow](./workflow.md): A recommended high-level process to adhere to while writing code diff --git a/development-guidelines/guidelines.md b/development-guidelines/guidelines.md index 64e00cb0..b74fb8db 100644 --- a/development-guidelines/guidelines.md +++ b/development-guidelines/guidelines.md @@ -3,113 +3,113 @@ Follow these high-level recommendations to build more secure smart contracts. - [Development Guidelines](#development-guidelines) - - [Design guidelines](#design-guidelines) - - [Documentation and specifications](#documentation-and-specifications) - - [On-chain vs off-chain computation](#on-chain-vs-off-chain-computation) + - [Design Guidelines](#design-guidelines) + - [Documentation and Specifications](#documentation-and-specifications) + - [On-chain vs Off-chain Computation](#on-chain-vs-off-chain-computation) - [Upgradeability](#upgradeability) - [Delegatecall Proxy](#delegatecall-proxy-pattern) - - [Implementation guidelines](#implementation-guidelines) - - [Function composition](#function-composition) + - [Implementation Guidelines](#implementation-guidelines) + - [Function Composition](#function-composition) - [Inheritance](#inheritance) - [Events](#events) - - [Avoid known pitfalls](#avoid-known-pitfalls) + - [Avoid Known Pitfalls](#avoid-known-pitfalls) - [Dependencies](#dependencies) - - [Testing and verification](#testing-and-verification) + - [Testing and Verification](#testing-and-verification) - [Solidity](#solidity) - - [Deployment guidelines](#deployment-guidelines) + - [Deployment Guidelines](#deployment-guidelines) -## Design guidelines +## Design Guidelines -The design of the contract should be discussed ahead of time, prior writing any line of code. +Discuss the design of the contract ahead of time, before writing any code. -### Documentation and specifications +### Documentation and Specifications -Documentation can be written at different levels, and should be updated while implementing the contracts: +Write documentation at different levels and update it as you implement the contracts: -- **A plain English description of the system**, describing what the contracts do and any assumptions on the codebase. -- **Schema and architectural diagrams**, including the contract interactions and the state machine of the system. [Slither printers](https://github.com/crytic/slither/wiki/Printer-documentation) can help to generate these schemas. -- **Thorough code documentation**, the [Natspec format](https://solidity.readthedocs.io/en/develop/natspec-format.html) can be used for Solidity. +- **A plain English description of the system**, describing the contracts' purpose and any assumptions about the codebase. +- **Schema and architectural diagrams**, including contract interactions and the system's state machine. Use [Slither printers](https://github.com/crytic/slither/wiki/Printer-documentation) to help generate these schemas. +- **Thorough code documentation**. Use the [Natspec format](https://solidity.readthedocs.io/en/develop/natspec-format.html) for Solidity. -### On-chain vs off-chain computation +### On-chain vs Off-chain Computation -- **Keep as much code as you can off-chain.** Keep the on-chain layer small. Pre-process data with code off-chain in such a way that verification on-chain is simple. Do you need an ordered list? Sort the list off-chain, then only check its order onchain. +- **Keep as much code off-chain as possible.** Keep the on-chain layer small. Pre-process data off-chain in a way that simplifies on-chain verification. Need an ordered list? Sort it off-chain, then check its order on-chain. ### Upgradeability -We discussed the different upgradeability solutions in [our blog post](https://blog.trailofbits.com/2018/09/05/contract-upgrade-anti-patterns/). In particular, if you are using delegatecall to achieve upgradability, carefully review all items of the above delegatecall proxy guidance. Make a deliberate choice to support upgradeability or not prior to writing any code. The decision will influence how you structure our code. In general, we recommend: +Refer to [our blog post](https://blog.trailofbits.com/2018/09/05/contract-upgrade-anti-patterns/) for different upgradeability solutions. If you are using delegatecall to achieve upgradability, carefully review all items of the delegatecall proxy guidance. Decide whether or not to support upgradeability before writing any code, as this decision will affect your code's structure. Generally, we recommend: -- **Favoring [contract migration](https://blog.trailofbits.com/2018/10/29/how-contract-migration-works/) over upgradeability.** Migration system have many of the same advantages than upgradeable, without their drawbacks. -- **Using the data separation pattern over the delegatecall proxy one.** If your project has a clear abstraction separation, upgradeability using data separation will necessitate only a few adjustments. The delegatecall proxy requires EVM expertise and is highly error-prone. -- **Document the migration/upgrade procedure before the deployment.** If you have to react under stress without any guidelines, you will make mistakes. Write the procedure to follow ahead of time. It should include: - - The calls that initiate the new contracts - - Where are stored the keys and how to access them - - How to check the deployment! Develop and test a post-deployment script. +- **Favoring [contract migration](https://blog.trailofbits.com/2018/10/29/how-contract-migration-works/) over upgradeability.** Migration systems offer many of the same advantages as upgradeable systems but without their drawbacks. +- **Using the data separation pattern instead of the delegatecall proxy pattern.** If your project has a clear abstraction separation, upgradeability using data separation will require only a few adjustments. The delegatecall proxy is highly error-prone and demands EVM expertise. +- **Document the migration/upgrade procedure before deployment.** Write the procedure to follow ahead of time to avoid errors when reacting under stress. It should include: + - The calls that initiate new contracts + - The keys' storage location and access method + - Deployment verification: develop and test a post-deployment script. #### Delegatecall Proxy Pattern -The delegatecall opcode is a very sharp tool that must be used carefully. Many high-profile exploits utilize little-known edge cases and counter-intuitive aspects of the delegatecall proxy pattern. This section aims to outline the most important risks to keep in mind while developing such smart contract systems. Trail of Bits developed the [slither-check-upgradability](https://github.com/crytic/slither/wiki/Upgradeability-Checks) tool to aid in the development of secure delegatecall proxies, it performs safety checks relevant to both upgradable and immutable delegatecall proxies. +The delegatecall opcode is a sharp tool that must be used carefully. Many high-profile exploits involve little-known edge cases and counter-intuitive aspects of the delegatecall proxy pattern. To aid the development of secure delegatecall proxies, utilize the [slither-check-upgradability](https://github.com/crytic/slither/wiki/Upgradeability-Checks) tool, which performs safety checks for both upgradable and immutable delegatecall proxies. -- **Storage layout**: The storage layout of the proxy and implementation must be the same. Do not try to define the same state variables on each contract. Instead, both contracts should inherit all of their state variables from one shared base contract. -- **Inheritance**: If the base storage contract is split up, beware that the order of inheritance impacts the final storage layout. For example, `contract A is B,C` and `contract A is C,B` will not yield the same storage layout if both B and C define state variables. -- **Initialization**: Make sure that the implementation is immediately initialized. Well-known disasters (and near disasters) have featured an uninitialized implementation contract. A factory pattern can help ensure that contracts are deployed & initialized correctly while also mitigating front-running risks that might otherwise open up between contract deployment & initialization. -- **Function shadowing**: If the same method is defined on the proxy and the implementation, then the proxy’s function will not be called. Be aware of `setOwner` and other administration functions that commonly exist on proxies. -- **Direct implementation usage**: Consider configuring the implementation’s state variables with values that prevent it from being used directly, such as by setting a flag during construction that disables the implementation and causes all methods to revert. This is particularly important if the implementation also performs delegatecall operations because this opens up the possibility of unintended self destruction of the implementation. -- **Immutable and constant variables**: These are embedded into the bytecode and can therefore get out of sync between the proxy and implementation. If the implementation has an incorrect immutable variable, this value may still be used even if the same variables are correctly set in the proxy’s bytecode. -- **Contract Existence Checks**: All [low-level calls](https://docs.soliditylang.org/en/latest/control-structures.html?highlight=existence#error-handling-assert-require-revert-and-exceptions), not just delegatecall, will return true against an address with empty bytecode. This might cause callers to be misled to think that a call performed a meaningful operation when it did not or it might result in important safety checks being silently skipped. Be aware that while a contract’s constructor is running, its bytecode remains empty until the end of constructor execution. We recommend that you rigorously verify that all low-level calls are properly protected against nonexistent contracts, keeping in mind that most proxy libraries (such as the one written by Openzeppelin) do not perform contract existence checks automatically. +- **Storage layout**: Proxy and implementation storage layouts must be the same. Instead of defining the same state variables for each contract, both should inherit all state variables from a shared base contract. +- **Inheritance**: Be aware that the order of inheritance affects the final storage layout. For example, `contract A is B, C` and `contract A is C, B` will not have the same storage layout if both B and C define state variables. +- **Initialization**: Immediately initialize the implementation. Well-known disasters (and near disasters) have featured an uninitialized implementation contract. A factory pattern can help ensure correct deployment and initialization and reduce front-running risks. +- **Function shadowing**: If the same method is defined on the proxy and the implementation, then the proxy’s function will not be called. Be mindful of `setOwner` and other administration functions commonly found on proxies. +- **Direct implementation usage**: Configure implementation state variables with values that prevent direct use, such as setting a flag during construction that disables the implementation and causes all methods to revert. This is particularly important if the implementation also performs delegatecall operations, as this may lead to unintended self-destruction of the implementation. +- **Immutable and constant variables**: These variables are embedded in the bytecode and can get out of sync between the proxy and implementation. If the implementation has an incorrect immutable variable, this value may still be used even if the same variable is correctly set in the proxy’s bytecode. +- **Contract Existence Checks**: All [low-level calls](https://docs.soliditylang.org/en/latest/control-structures.html?highlight=existence#error-handling-assert-require-revert-and-exceptions), including delegatecall, return true for an address with empty bytecode. This can mislead callers into thinking a call performed a meaningful operation when it did not or cause crucial safety checks to be skipped. While a contract’s constructor runs, its bytecode remains empty until the end of execution. We recommend rigorously verifying that all low-level calls are protected against nonexistent contracts. Keep in mind that most proxy libraries (such as Openzeppelin's) do not automatically perform contract existence checks. -For more information regarding delegatecall proxies in general, reference our blog posts and presentations: +For more information on delegatecall proxies, consult our blog posts and presentations: -- [Contract Upgradability Anti-Patterns](https://blog.trailofbits.com/2018/09/05/contract-upgrade-anti-patterns/): Describes the difference between a downstream data contract and delegatecall proxies which use an upstream data contract and how these patterns impact upgradability. -- [How the Diamond standard falls short](https://blog.trailofbits.com/2020/10/30/good-idea-bad-design-how-the-diamond-standard-falls-short/): This post dives deep into delegatecall risks which apply to all contracts, not just those that follow the diamond standard. -- [Breaking Aave Upgradeability](https://blog.trailofbits.com/2020/12/16/breaking-aave-upgradeability/): A write-up describing a subtle problem that we discovered in Aave `AToken` contracts that resulted from the interplay between delegatecall proxies, contact existence checks, and unsafe initialization. -- [Contract Upgrade Risks and Recommendations](https://youtu.be/mebA5Qz9zeQ?t=353): A talk by Trail of Bits describing best-practices for developing upgradable delegatecall proxies. The section starting at 5:49 describes some general risks that also apply to non-upgradable proxies. +- [Contract Upgradability Anti-Patterns](https://blog.trailofbits.com/2018/09/05/contract-upgrade-anti-patterns/): Describes the differences between downstream data contracts and delegatecall proxies with upstream data contracts and how these patterns affect upgradability. +- [How the Diamond Standard Falls Short](https://blog.trailofbits.com/2020/10/30/good-idea-bad-design-how-the-diamond-standard-falls-short/): Explores delegatecall risks that apply to all contracts, not just those following the diamond standard. +- [Breaking Aave Upgradeability](https://blog.trailofbits.com/2020/12/16/breaking-aave-upgradeability/): Discusses a subtle problem we discovered in Aave `AToken` contracts, resulting from the interplay between delegatecall proxies, contract existence checks, and unsafe initialization. +- [Contract Upgrade Risks and Recommendations](https://youtu.be/mebA5Qz9zeQ?t=353): A Trail of Bits talk on best practices for developing upgradable delegatecall proxies. The section starting at 5:49 describes general risks for non-upgradable proxies. -## Implementation guidelines +## Implementation Guidelines -**Strive for simplicity.** Always use the simplest solution that fits your purpose. Any member of your team should be able to understand your solution. +**Aim for simplicity.** Use the simplest solution that meets your needs. Any member of your team should understand your solution. -### Function composition +### Function Composition -The architecture of your codebase should make your code easy to review. Avoid architectural choices that decrease the ability to reason about its correctness. +Design your codebase architecture to facilitate easy review and allow testing individual components: -- **Split the logic of your system**, either through multiple contracts or by grouping similar functions together (for example, authentication, arithmetic, ...). -- **Write small functions, with a clear purpose.** This will facilitate easier review and allow the testing of individual components. +- **Divide the system's logic**, either through multiple contracts or by grouping similar functions together (e.g. authentication, arithmetic). +- **Write small functions with clear purposes.** ### Inheritance -- **Keep the inheritance manageable.** Inheritance should be used to divide the logic, however, your project should aim to minimize the depth and width of the inheritance tree. -- **Use Slither’s [inheritance printer](https://github.com/crytic/slither/wiki/Printer-documentation#inheritance-graph) to check the contracts’ hierarchy.** The inheritance printer will help you review the size of the hierarchy. +- **Keep inheritance manageable.** Though inheritance can help divide logic you should aim to minimize the depth and width of the inheritance tree. +- **Use Slither’s [inheritance printer](https://github.com/crytic/slither/wiki/Printer-documentation#inheritance-graph) to check contract hierarchies.** The inheritance printer can help review the hierarchy size. ### Events -- **Log all crucial operations.** Events will help to debug the contract during the development, and monitor it after deployment. +- **Log all critical operations.** Events facilitate contract debugging during development and monitoring after deployment. -### Avoid known pitfalls +### Avoid Known Pitfalls -- **Be aware of the most common security issues.** There are many online resources to learn about common issues, such as [Ethernaut CTF](https://ethernaut.openzeppelin.com/), [Capture the Ether](https://capturetheether.com/), or [Not so smart contracts](https://github.com/crytic/not-so-smart-contracts/). -- **Be aware of the warnings sections in the [Solidity documentation](https://solidity.readthedocs.io/en/latest/).** The warnings sections will inform you about non-obvious behavior of the language. +- **Be aware of common security issues.** Many online resources can help, such as [Ethernaut CTF](https://ethernaut.openzeppelin.com/), [Capture the Ether](https://capturetheether.com/), and [Not So Smart Contracts](https://github.com/crytic/not-so-smart-contracts/). +- **Review the warnings sections in the [Solidity documentation](https://solidity.readthedocs.io/en/latest/).** These sections reveal non-obvious language behaviors. ### Dependencies -- **Use well-tested libraries.** Importing code from well-tested libraries will reduce the likelihood that you write buggy code. If you want to write an ERC20 contract, use [OpenZeppelin](https://github.com/OpenZeppelin/openzeppelin-contracts/tree/master/contracts/token/ERC20). -- **Use a dependency manager; avoid copy-pasting code.** If you rely on an external source, then you must keep it up-to-date with the original source. +- **Use well-tested libraries.** Importing code from well-tested libraries reduces the likelihood of writing buggy code. If writing an ERC20 contract, use [OpenZeppelin](https://github.com/OpenZeppelin/openzeppelin-contracts/tree/master/contracts/token/ERC20). +- **Use a dependency manager instead of copying and pasting code.** Always keep external sources up-to-date. -### Testing and verification +### Testing and Verification -- **Write thorough unit-tests.** An extensive test suite is crucial to build high-quality software. -- **Write [Slither](https://github.com/crytic/slither) and [Echidna](https://github.com/crytic/echidna) custom checks and properties.** Automated tools will help ensure your contract is secure. Review the rest of this guide to learn how to write efficient checks and properties. +- **Create thorough unit tests.** An extensive test suite is essential for developing high-quality software. +- **Develop custom [Slither](https://github.com/crytic/slither) and [Echidna](https://github.com/crytic/echidna) checks and properties.** Automated tools help ensure contract security. Review the rest of this guide to learn how to write efficient checks and properties. ### Solidity -- **Favor Solidity versions outlined in our [Slither Recommendations](https://github.com/crytic/slither/wiki/Detector-Documentation#incorrect-versions-of-solidity)** In our opinion, older Solidity are more secure and have better built-in practices. Newer Solidity versions may be too young to be used in production and require additional time to mature. -- **Use a stable release to compile; use the latest release to check for warnings.** Check that your code has no reported issues with the latest compiler version. However, Solidity has a fast release cycle and has a history of compiler bugs, so we do not recommend the latest version for deployment (see Slither’s [solc version recommendation](https://github.com/crytic/slither/wiki/Detector-Documentation#incorrect-versions-of-solidity)). -- **Do not use inline assembly.** Assembly requires EVM expertise. Do not write EVM code if you have not _mastered_ the yellow paper. +- **Favor Solidity versions outlined in our [Slither Recommendations](https://github.com/crytic/slither/wiki/Detector-Documentation#incorrect-versions-of-solidity)**. We believe older Solidity versions are more secure and have better built-in practices. Newer versions may be too immature for production and need time to develop. +- **Compile using a stable release, but check for warnings with the latest release.** Verify that the latest compiler version reports no issues with your code. However, since Solidity has a fast release cycle and a history of compiler bugs, we do not recommend the newest version for deployment. See Slither’s [solc version recommendation](https://github.com/crytic/slither/wiki/Detector-Documentation#incorrect-versions-of-solidity). +- **Avoid inline assembly.** Assembly requires EVM expertise. Do not write EVM code unless you have _mastered_ the yellow paper. -## Deployment guidelines +## Deployment Guidelines -Once the contract has been developed and deployed: +After developing and deploying the contract: -- **Monitor your contracts.** Watch the logs, and be ready to react in case of contract or wallet compromise. -- **Add your contact info to [blockchain-security-contacts](https://github.com/crytic/blockchain-security-contacts).** This list helps third-parties contact you if a security flaw is discovered. -- **Secure the wallets of privileged users.** Follow our [best practices](https://blog.trailofbits.com/2018/11/27/10-rules-for-the-secure-use-of-cryptocurrency-hardware-wallets/) if you store keys in hardware wallets. -- **Have a response to incident plan.** Consider that your smart contracts can be compromised. Even if your contracts are free of bugs, an attacker may take control of the contract owner's keys. +- **Monitor contracts.** Observe logs and be prepared to respond in the event of contract or wallet compromise. +- **Add contact information to [blockchain-security-contacts](https://github.com/crytic/blockchain-security-contacts).** This list helps third parties notify you of discovered security flaws. +- **Secure privileged users' wallets.** Follow our [best practices](https://blog.trailofbits.com/2018/11/27/10-rules-for-the-secure-use-of-cryptocurrency-hardware-wallets/) for hardware wallet key storage. +- **Develop an incident response plan.** Assume your smart contracts can be compromised. Possible threats include contract bugs or attackers gaining control of the contract owner's keys. diff --git a/development-guidelines/incident_response.md b/development-guidelines/incident_response.md index c2b34b9c..61016860 100644 --- a/development-guidelines/incident_response.md +++ b/development-guidelines/incident_response.md @@ -1,30 +1,30 @@ # Incident Response Recommendations -Here, we provide recommendations around the formulation of an incident response plan. +In this section, we provide recommendations for formulating a robust incident response plan. -- [ ] **Identify who (either specific people or roles) is responsible for carrying out the mitigations (deploying smart contracts, pausing contracts, upgrading the front end, etc.).** - - Specifying these roles will strengthen the incident response plan and ease the execution of mitigating actions when necessary. -- [ ] **Document internal processes for situations in which a deployed remediation does not work or introduces a new bug.** - - Consider adding a fallback scenario that describes an action plan in the event of a failed remediation. -- [ ] **Clearly describe the intended process of contract deployment.** -- [ ] **Consider whether and under what circumstances your company will make affected users whole after certain issues occur.** - - Some scenarios to consider include an individual or aggregate loss, a loss resulting from user error, a contract flaw, and a third-party contract flaw. -- [ ] **Document how you plan to keep up to date on new issues, both to inform future development and to secure the deployment toolchain and the external on-chain and off-chain services that the system relies on.** - - For each language and component, describe the noteworthy sources for vulnerability news. Subscribe to updates for each source. Consider creating a special private Discord/Slack channel with a bot that will post the latest vulnerability news; this will help the team keep track of updates all in one place. Also consider assigning specific team members to keep track of the vulnerability news of a specific component of the system. -- [ ] **Consider scenarios involving issues that would indirectly affect the system.** -- [ ] **Determine when and how the team would reach out to and onboard external parties (auditors, affected users, other protocol developers, etc.).** - - Some issues may require collaboration with external parties to efficiently remediate them. -- [ ] **Define contract behavior that is considered abnormal for off-chain monitoring.** - - Consider adding more resilient solutions for detection and mitigation, especially in terms of specific alternate endpoints and queries for different data as well as status pages and support contacts for affected services. -- [ ] **Combine issues and determine whether new detection and mitigation scenarios are needed.** -- [ ] **Perform periodic dry runs of specific scenarios in the incident response plan to find gaps and opportunities for improvement and to develop muscle memory.** - - Document the intervals at which the team should perform dry runs of the various scenarios. For scenarios that are more likely to happen, perform dry runs more regularly. Create a template to be filled in after a dry run to describe the improvements that need to be made to the incident response. +- [ ] **Identify specific individuals or roles responsible for carrying out the mitigations (deploying smart contracts, pausing contracts, upgrading the front end, etc.).** + - Defining these roles will enhance the incident response plan and facilitate the execution of mitigation actions when necessary. +- [ ] **Document internal processes in cases where deployed remediation fails or introduces new bugs.** + - Consider developing a fallback plan that outlines an action strategy for failed remediation attempts. +- [ ] **Provide a clear description of the intended contract deployment process.** +- [ ] **Consider whether and under what circumstances your company will compensate affected users in the event of certain issues.** + - Some situations to consider include individual or aggregate losses, losses resulting from user error, contract flaws, and third-party contract flaws. +- [ ] **Outline a plan for staying informed about new issues, so as to inform future development and enhance the security of the deployment toolchain and the external on-chain and off-chain services your system depends on.** + - For each language and component, identify reputable sources of vulnerability news. Subscribe to updates for each source. Consider creating a private Discord or Slack channel with a bot that posts the latest vulnerability news to help your team stay informed in a centralized location. Additionally, consider assigning specific team members to track vulnerability news for particular system components. +- [ ] **Examine scenarios involving issues that would indirectly affect the system.** +- [ ] **Decide when and how the team should seek assistance from or collaborate with external parties (auditors, affected users, other protocol developers, etc.).** + - Some problems may necessitate cooperation with external parties for efficient resolution. +- [ ] **Define abnormal contract behavior for off-chain monitoring purposes.** + - Consider implementing more robust detection and mitigation solutions, including specific alternate endpoints, queries for diverse data, status pages, and support contacts for impacted services. +- [ ] **Combine issues to evaluate whether new detection and mitigation scenarios are necessary.** +- [ ] **Conduct periodic dry runs of specific scenarios in the incident response plan to identify gaps and improvement opportunities, and build muscle memory.** + - Establish intervals for performing dry runs for each scenario. Conduct more frequent dry runs for scenarios with higher likelihoods of occurrence. Create a template to document improvements required after each dry run for the incident response plan. ## Incident Response Plan Resources - [How to Hack the Yield Protocol](https://docs.yieldprotocol.com/#/operations/how_to_hack) - [Emergency Steps – Yearn](https://github.com/yearn/yearn-devdocs/blob/master/docs/developers/v2/EMERGENCY.md) -## Well-handled IR Incidents +## Examples of Well-Handled Incidents - [Yield Protocol](https://medium.com/yield-protocol/post-mortem-of-incident-on-august-5th-2022-7bb70dbb9ada) diff --git a/development-guidelines/token_integration.md b/development-guidelines/token_integration.md index 569a89bb..aee8158a 100644 --- a/development-guidelines/token_integration.md +++ b/development-guidelines/token_integration.md @@ -1,15 +1,15 @@ -# Token integration checklist +# Token Integration Checklist -The following checklist provides recommendations for interactions with arbitrary tokens. Every unchecked item should be justified, and its associated risks, understood. +This checklist offers recommendations for interacting with arbitrary tokens. Ensure that every unchecked item is justified and that its risks are understood. -For convenience, all Slither [utilities](https://github.com/crytic/slither#tools) can be run directly on a token address, such as the following: +For convenience, all Slither [utilities](https://github.com/crytic/slither#tools) can be run directly on a token address, as shown below: ```bash slither-check-erc 0xdac17f958d2ee523a2206206994597c13d831ec7 TetherToken --erc erc20 slither-check-erc 0x06012c8cf97BEaD5deAe237070F9587f8E7A266d KittyCore --erc erc721 ``` -To follow this checklist, use the below output from Slither for the token: +Use the following Slither output for the token to follow this checklist: ```bash - slither-check-erc [target] [contractName] [optional: --erc ERC_NUMBER] @@ -18,79 +18,79 @@ To follow this checklist, use the below output from Slither for the token: - slither-prop . --contract ContractName # requires configuration, and use of Echidna and Manticore ``` -## General considerations +## General Considerations -- [ ] **The contract has a security review.** Avoid interacting with contracts that lack a security review. Check the length of the assessment (i.e., the 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. +- [ ] **The contract has a security review.** Avoid interacting with contracts that lack a security review. Assess the review's duration (i.e., the level of effort), the reputation of the security firm, and the number and severity of findings. +- [ ] **You have contacted the developers.** If necessary, alert their team to incidents. Locate 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!) on critical issues or when upgrades occur. -## Contract composition +## 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. -- [ ] **The token only has one address.** Tokens with multiple entry points for balance updates can break internal bookkeeping based on the address (e.g., `balances[token_address][msg.sender]` may not reflect the actual balance). +- [ ] **The contract avoids unnecessary complexity.** The token should be a simple contract; tokens with complex code require 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 manually for `SafeMath` usage. +- [ ] **The contract has only a few non-token-related functions.** Non-token-related functions increase the likelihood of issues 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. +- [ ] **The token has only one address.** Tokens with multiple entry points for balance updates can break internal bookkeeping based on the address (e.g., `balances[token_address][msg.sender]` might not reflect the actual balance). -## Owner privileges +## Owner Privileges -- [ ] **The token is not upgradeable.** Upgradeable contracts may change their rules over time. Use Slither’s [`human-summary` printer](https://github.com/crytic/slither/wiki/Printer-documentation#contract-summary) to determine whether 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 pausable 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 teams that reside in legal shelters require a higher standard of review. +- [ ] **The token is not upgradeable.** Upgradeable contracts may 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 pausable code manually. +- [ ] **The owner cannot blacklist the contract.** Malicious or compromised owners can trap contracts relying on tokens with a blacklist. Identify blacklisting features manually. +- [ ] **The team behind the token is known and can be held responsible for abuse.** Contracts with anonymous development teams or teams situated in legal shelters require a higher standard of review. -## ERC20 tokens +## ERC20 Tokens -### ERC20 conformity checks +### ERC20 Conformity Checks -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 the following: +Slither includes the [`slither-check-erc`](https://github.com/crytic/slither/wiki/ERC-Conformance) utility that checks a token's conformance to various ERC standards. Use `slither-check-erc` to review the following: -- [ ] **`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. +- [ ] **`Transfer` and `transferFrom` return a boolean.** Some tokens do not return a boolean for these functions, which may cause their calls in the contract to fail. - [ ] **The `name`, `decimals`, and `symbol` functions are present if used.** These functions are optional in the ERC20 standard and may not be present. -- [ ] **`Decimals` returns a `uint8`.** Several tokens incorrectly return a `uint256`. In such cases, ensure that 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. +- [ ] **`Decimals` returns a `uint8`.** Some tokens incorrectly return a `uint256`. In these cases, ensure the returned value 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 race condition that must be mitigated to prevent attackers from stealing tokens. -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 the following: +Slither includes the [`slither-prop`](https://github.com/crytic/slither/wiki/Property-generation) utility, which generates unit tests and security properties to find many common ERC flaws. Use slither-prop to review the following: -- [ ] **The contract passes all unit tests and security properties from `slither-prop`.** Run the generated unit tests and then check the properties with [Echidna](https://github.com/crytic/echidna) and [Manticore](https://manticore.readthedocs.io/en/latest/verifier.html). +- [ ] **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). ### Risks of ERC20 Extensions -The behavior of certain contracts may differ from the original ERC specification. Conduct a manual review of the following conditions: +The behavior of certain contracts may differ from the original ERC specification. Review the following conditions manually: - [ ] **The token is not an ERC777 token and has no external function call in `transfer` or `transferFrom`.** External calls in the transfer functions can lead to reentrancies. - [ ] **`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 may be trapped in the contract if not taken into account. +- [ ] **Consider any interest earned from the token.** Some tokens distribute interest to token holders. If not taken into account, this interest may become trapped in the contract. -### Token scarcity +### Token Scarcity -Reviews of token scarcity issues must be executed manually. Check for the following conditions: +Token scarcity issues must be reviewed manually. Check for the following conditions: -- [ ] **The supply is owned by more than a few users.** If a few users own most of the tokens, they can influence operations based on the tokens’ repartition. +- [ ] **The supply is owned by more than a few users.** If a few users own most of the tokens, they can influence operations based on the tokens' distribution. - [ ] **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 could compromise the contract relying on the token. -- [ ] **Users understand the risks associated with a large amount of funds or flash loans.** Contracts relying on the token balance must account for attackers with a large amount of funds or attacks executed through flash loans. -- [ ] **The token does not allow flash minting.** Flash minting can lead to substantial swings in the balance and the total supply, which necessitate strict and comprehensive overflow checks in the operation of the token. +- [ ] **The tokens are located in more than a few exchanges.** If all tokens are in one exchange, compromising the exchange could compromise the contract relying on the token. +- [ ] **Users understand the risks associated with large funds or flash loans.** Contracts relying on the token balance must account for attackers with large funds or attacks executed through flash loans. +- [ ] **The token does not allow flash minting.** Flash minting can lead to drastic changes in balance and total supply, requiring strict and comprehensive overflow checks in the token operation. -## ERC721 tokens +## ERC721 Tokens ### ERC721 Conformity Checks -The behavior of certain contracts may differ from the original ERC specification. Conduct a manual review of the following conditions: +The behavior of certain contracts may differ from the original ERC specification. Review the following conditions manually: -- [ ] **Transfers of tokens to the 0x0 address revert.** Several tokens allow transfers to 0x0 and consider tokens transferred to that address to have been burned; however, the ERC721 standard requires that such transfers revert. -- [ ] **`safeTransferFrom` functions are implemented with the correct signature.** Several token contracts do not implement these functions. A transfer of NFTs to one of those contracts can result in a loss of assets. +- [ ] **Transfers of tokens to the 0x0 address revert.** Some tokens allow transfers to 0x0 and consider tokens sent to that address to have been burned; however, the ERC721 standard requires that such transfers revert. +- [ ] **`safeTransferFrom` functions are implemented with the correct signature.** Some token contracts do not implement these functions. Transferring NFTs to one of those contracts can result in a loss of assets. - [ ] **The `name`, `decimals`, and `symbol` functions are present if used.** These functions are optional in the ERC721 standard and may not be present. -- [ ] **If it is used, `decimals` returns a `uint8(0)`.** Other values are invalid. +- [ ] **If used, `decimals` returns a `uint8(0)`.** Other values are invalid. - [ ] **The `name` and `symbol` functions can return an empty string.** This behavior is allowed by the standard. -- [ ] **The `ownerOf` function reverts if the `tokenId` is invalid or is set to a token that has already been burned.** The function cannot return 0x0. This behavior is required by the standard, but it is not always properly implemented. +- [ ] **The `ownerOf` function reverts if the `tokenId` is invalid or refers to a token that has already been burned.** The function cannot return 0x0. This behavior is required by the standard but may not always be implemented correctly. - [ ] **A transfer of an NFT clears its approvals.** This is required by the standard. - [ ] **The token ID of an NFT cannot be changed during its lifetime.** This is required by the standard. ### Common Risks of the ERC721 Standard -To mitigate the risks associated with ERC721 contracts, conduct a manual review of the following conditions: +Mitigate the risks associated with ERC721 contracts by conducting a manual review of the following conditions: - [ ] **The `onERC721Received` callback is taken into account.** External calls in the transfer functions can lead to reentrancies, especially when the callback is not explicit (e.g., in [`safeMint`](https://www.paradigm.xyz/2021/08/the-dangers-of-surprising-code/) calls). -- [ ] **When an NFT is minted, it is safely transferred to a smart contract.** If there is a minting function, it should behave similarly to safeTransferFrom and properly handle the minting of new tokens to a smart contract. This will prevent a loss of assets. -- [ ] **The burning of a token clears its approvals.** If there is a burning function, it should clear the token’s previous approvals. +- [ ] **When an NFT is minted, it is safely transferred to a smart contract.** If a minting function exists, it should behave similarly to `safeTransferFrom` and handle the minting of new tokens to a smart contract properly, preventing asset loss. +- [ ] **Burning a token clears its approvals.** If a burning function exists, it should clear the token’s previous approvals. diff --git a/development-guidelines/workflow.md b/development-guidelines/workflow.md index a4dac4d0..35e8e8dd 100644 --- a/development-guidelines/workflow.md +++ b/development-guidelines/workflow.md @@ -1,43 +1,43 @@ -# Secure development workflow +# Secure Development Workflow -Here's a high-level process we recommend following while you write your smart contracts. +Follow this high-level process while developing your smart contracts for enhanced security: -Check for known security issues: +1. **Check for known security issues:** -- [ ] Review your contracts with [Slither](https://github.com/crytic/slither). It has more than 70 built-in detectors for common vulnerabilities. Run it on every check-in with new code and ensure it gets a clean report (or use triage mode to silence certain issues). +- [ ] Review your contracts using [Slither](https://github.com/crytic/slither), which has over 70 built-in detectors for common vulnerabilities. Run it on every check-in with new code and ensure it gets a clean report (or use triage mode to silence certain issues). -Consider special features of your contract: +2. **Consider special features of your contract:** -- [ ] Are your contracts upgradeable? Review your upgradeability code for flaws with [`slither-check-upgradeability`](https://github.com/crytic/slither/wiki/Upgradeability-Checks) or [Crytic](https://blog.trailofbits.com/2020/06/12/upgradeable-contracts-made-safer-with-crytic/). We've documented 17 ways upgrades can go sideways. -- [ ] Do your contracts purport to conform to ERCs? Check them with [`slither-check-erc`](https://github.com/crytic/slither/wiki/ERC-Conformance). This tool instantly identifies deviations from six common specs. -- [ ] Do you have unit tests in Truffle? Enrich them with [`slither-prop`](https://github.com/crytic/slither/wiki/Property-generation). It automatically generates a robust suite of security properties for features of ERC20 based on your specific code. -- [ ] Do you integrate with 3rd party tokens? Review our [token integration checklist](./token_integration.md) before relying on external contracts. +- [ ] If your contracts are upgradeable, review your upgradeability code for flaws using [`slither-check-upgradeability`](https://github.com/crytic/slither/wiki/Upgradeability-Checks) or [Crytic](https://blog.trailofbits.com/2020/06/12/upgradeable-contracts-made-safer-with-crytic/). We have documented 17 ways upgrades can go sideways. +- [ ] If your contracts claim to conform to ERCs, check them with [`slither-check-erc`](https://github.com/crytic/slither/wiki/ERC-Conformance). This tool instantly identifies deviations from six common specs. +- [ ] If you have unit tests in Truffle, enrich them with [`slither-prop`](https://github.com/crytic/slither/wiki/Property-generation). It automatically generates a robust suite of security properties for features of ERC20 based on your specific code. +- [ ] If you integrate with third-party tokens, review our [token integration checklist](./token_integration.md) before relying on external contracts. -Visually inspect critical security features of your code: +3. **Visually inspect critical security features of your code:** -- [ ] Review Slither's [inheritance-graph](https://github.com/trailofbits/slither/wiki/Printer-documentation#inheritance-graph) printer. Avoid inadvertent shadowing and C3 linearization issues. -- [ ] Review Slither's [function-summary](https://github.com/trailofbits/slither/wiki/Printer-documentation#function-summary) printer. It reports function visibility and access controls. -- [ ] Review Slither's [vars-and-auth](https://github.com/trailofbits/slither/wiki/Printer-documentation#variables-written-and-authorization) printer. It reports access controls on state variables. +- [ ] Review Slither's [inheritance-graph](https://github.com/trailofbits/slither/wiki/Printer-documentation#inheritance-graph) printer to avoid inadvertent shadowing and C3 linearization issues. +- [ ] Review Slither's [function-summary](https://github.com/trailofbits/slither/wiki/Printer-documentation#function-summary) printer, which reports function visibility and access controls. +- [ ] Review Slither's [vars-and-auth](https://github.com/trailofbits/slither/wiki/Printer-documentation#variables-written-and-authorization) printer, which reports access controls on state variables. -Document critical security properties and use automated test generators to evaluate them: +4. **Document critical security properties and use automated test generators to evaluate them:** -- [ ] Learn to [document security properties for your code](../program-analysis/). It's tough as first, but it's the single most important activity for achieving a good outcome. It's also a prerequisite for using any of the advanced techniques in this tutorial. -- [ ] Define security properties in Solidity, for use with [Echidna](https://github.com/crytic/echidna) and [Manticore](https://manticore.readthedocs.io/en/latest/verifier.html). Focus on your state machine, access controls, arithmetic operations, external interactions, and standards conformance. -- [ ] Define security properties with [Slither's Python API](../program-analysis/slither). Focus on inheritance, variable dependencies, access controls, and other structural issues. +- [ ] Learn to [document security properties for your code](../program-analysis/). Although challenging at first, it is the single most important activity for achieving a good outcome. It is also a prerequisite for using any advanced techniques in this tutorial. +- [ ] Define security properties in Solidity for use with [Echidna](https://github.com/crytic/echidna) and [Manticore](https://manticore.readthedocs.io/en/latest/verifier.html). Focus on your state machine, access controls, arithmetic operations, external interactions, and standards conformance. +- [ ] Define security properties with [Slither's Python API](../program-analysis/slither). Concentrate on inheritance, variable dependencies, access controls, and other structural issues. -Finally, be mindful of issues that automated tools cannot easily find: +5. **Be mindful of issues that automated tools cannot easily find:** -- Lack of privacy: everyone else can see your transactions while they're queued in the pool -- Front running transactions -- Cryptographic operations -- Risky interactions with external DeFi components +- Lack of privacy: Transactions are visible to everyone else while queued in the pool. +- Front running transactions. +- Cryptographic operations. +- Risky interactions with external DeFi components. ## Ask for help -[Office Hours](https://calendly.com/trail-of-bits-sales/trail-of-bits-office-hours) run every Tuesday afternoon. These 1-hour, 1-on-1 sessions are an opportunity to ask us any questions you have about security, troubleshoot using our tools, and get feedback from experts about your current approach. We will help you work through this guide. +[Office Hours](https://calendly.com/trail-of-bits-sales/trail-of-bits-office-hours) are held every Tuesday afternoon. These one-hour, one-on-one sessions provide an opportunity to ask questions about security, troubleshoot tool usage, and receive expert feedback on your current approach. We will help you work through this guide. -Join our Slack: [Empire Hacking](https://join.slack.com/t/empirehacking/shared_invite/zt-h97bbrj8-1jwuiU33nnzg67JcvIciUw). We're always available in the #crytic and #ethereum channels if you have any questions. +Join our Slack: [Empire Hacking](https://join.slack.com/t/empirehacking/shared_invite/zt-h97bbrj8-1jwuiU33nnzg67JcvIciUw). We are always available in the #crytic and #ethereum channels if you have questions. ## Security is about more than just smart contracts -Review our quick tips for [general application and corporate security](https://docs.google.com/document/d/1-_0Wlwch_vtkPM4F-SdEXLjQYaYT7KoPlU2rjt7tkLQ/edit?usp=sharing). It's most important that your code on-chain is secure, but lapses in off-chain security may be just as severe, especially where owner keys are concerned. +Review our quick tips for [general application and corporate security](https://docs.google.com/document/d/1-_0Wlwch_vtkPM4F-SdEXLjQYaYT7KoPlU2rjt7tkLQ/edit?usp=sharing). While it is crucial to ensure on-chain code security, off-chain security lapses can be equally severe, especially regarding owner keys.