init delegatecall guidelines

development-guidelines/guidelines.md

@@ -7,6 +7,7 @@ Follow these high-level recommendations to build more secure smart contracts.
     - [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)
     - [Inheritance](#inheritance)
@@ -31,19 +32,38 @@ Documentation can be written at different levels, and should be updated while im
 
 ### 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 offchain, then only check its order onchain.
+- **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.
 
 ### Upgradeability
 
-We discussed the different upgradeability solutions in [our blogpost](https://blog.trailofbits.com/2018/09/05/contract-upgrade-anti-patterns/). 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:
+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:
 
 - **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 delegatecallproxy one.** If your project has a clear abstraction separation, upgradeability using data separation will necessitate only a few adjustments. The delegatecallproxy requires EVM expertise and is highly error-prone.
+- **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.
 
+#### 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.
+
+- **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.
+
+For more information regarding delegatecall proxies in general, reference 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.
+
+
 ##  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.
@@ -81,7 +101,7 @@ The architecture of your codebase should make your code easy to review. Avoid ar
 
 ### 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 ttoo young to be used in production and require additional time to mature.
+- **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.