chore(protocol): improve contracts and docs for auditing (#14565)

Co-authored-by: Daniel Wang <[email protected]> Co-authored-by: dantaik <[email protected]> Co-authored-by: dave | d1onys1us <[email protected]> Co-authored-by: adaki2004 <[email protected]> Co-authored-by: jeff <[email protected]> Co-authored-by: Francisco Ramos <[email protected]> Co-authored-by: David <[email protected]> Co-authored-by: Kenk <[email protected]> Co-authored-by: mfinestone <[email protected]> Co-authored-by: hideonbug <[email protected]> Co-authored-by: megumii <[email protected]> Co-authored-by: Tomaž <[email protected]> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Daniel Wang <[email protected]> Co-authored-by: Jeffery Walsh <[email protected]> Co-authored-by: cyberhorsey <[email protected]> Co-authored-by: Brecht Devos <[email protected]> Co-authored-by: d1onys1us <[email protected]> Co-authored-by: Roger <[email protected]> Co-authored-by: Korbinian <[email protected]> Co-authored-by: Daniel Wang <[email protected]>
taikoxyz · Sep 14, 2023 · 94e4201 · 94e4201
1 parent 740bc83
commit 94e4201
Show file tree

Hide file tree

Showing 15 changed files with 534 additions and 779 deletions.
diff --git a/packages/protocol/contracts/L1/TaikoToken.sol b/packages/protocol/contracts/L1/TaikoToken.sol
@@ -25,8 +25,8 @@ import { PausableUpgradeable } from
 import { Proxied } from "../common/Proxied.sol";
 
 /// @title TaikoToken
-/// @notice The TaikoToken (TKO) is used for proposing blocks and also for
-/// staking in the Taiko protocol. It is an ERC20 token with 8 decimal places of
+/// @notice The TaikoToken (TKO), in the protocol is used for prover collateral
+/// in the form of bonds. It is an ERC20 token with 18 decimal places of
 /// precision.
 contract TaikoToken is
     EssentialContract,
@@ -109,7 +109,7 @@ contract TaikoToken is
         uint256 amount
     )
         public
-        onlyFromNamed("erc20_vault")
+        onlyFromNamed2("erc20_vault", "taiko")
     {
         _burn(from, amount);
     }

diff --git a/packages/protocol/contracts/L1/libs/LibDepositing.sol b/packages/protocol/contracts/L1/libs/LibDepositing.sol
@@ -61,6 +61,9 @@ library LibDepositing {
             })
         );
 
+        // Unchecked is safe:
+        // - uint64 can store up to ~1.8 * 1e19, which can represent 584K years
+        // if we are depositing at every second
         unchecked {
             state.slotA.numEthDeposits++;
         }
@@ -106,6 +109,12 @@ library LibDepositing {
                 });
                 uint96 _fee =
                     deposits[i].amount > fee ? fee : deposits[i].amount;
+
+                // Unchecked is safe:
+                // - _fee cannot be bigger than deposits[i].amount
+                // - all values are in the same range (uint96) except loop
+                // counter, which obviously cannot be bigger than uint95
+                // otherwise the function would be gassing out.
                 unchecked {
                     deposits[i].amount -= _fee;
                     totalFee += _fee;
@@ -118,6 +127,10 @@ library LibDepositing {
             state.ethDeposits[state.slotA.numEthDeposits
                 % config.ethDepositRingBufferSize] =
                 _encodeEthDeposit(feeRecipient, totalFee);
+
+            // Unchecked is safe:
+            // - uint64 can store up to ~1.8 * 1e19, which can represent 584K
+            // years if we are depositing at every second
             unchecked {
                 state.slotA.numEthDeposits++;
             }
@@ -138,6 +151,13 @@ library LibDepositing {
         view
         returns (bool)
     {
+        // Unchecked is safe:
+        // - both numEthDeposits and state.slotA.nextEthDepositToProcess are
+        // indexes. One is tracking all deposits (numEthDeposits: unprocessed)
+        // and the next to be processed, so nextEthDepositToProcess cannot be
+        // bigger than numEthDeposits
+        // - ethDepositRingBufferSize cannot be 0 by default (validity checked
+        // in LibVerifying)
         unchecked {
             return amount >= config.ethDepositMinAmount
                 && amount <= config.ethDepositMaxAmount

diff --git a/packages/protocol/contracts/L1/libs/LibProposing.sol b/packages/protocol/contracts/L1/libs/LibProposing.sol
@@ -83,6 +83,7 @@ library LibProposing {
 
         TaikoToken tt = TaikoToken(resolver.resolve("taiko_token", false));
         if (state.taikoTokenBalances[assignment.prover] >= config.proofBond) {
+            // Safe, see the above constraint
             unchecked {
                 state.taikoTokenBalances[assignment.prover] -= config.proofBond;
             }
@@ -127,6 +128,11 @@ library LibProposing {
         uint256 reward;
         if (config.proposerRewardPerSecond > 0 && config.proposerRewardMax > 0)
         {
+            // Unchecked is safe:
+            // - block.timestamp is always greater than block.proposedAt
+            // (proposed in the past)
+            // - 1x state.taikoTokenBalances[addr] uint256 could theoretically
+            // store the whole token supply
             unchecked {
                 uint256 blockTime = block.timestamp
                     - state.blocks[(b.numBlocks - 1) % config.blockRingBufferSize]
@@ -152,6 +158,10 @@ library LibProposing {
         }
 
         // Init the metadata
+        // Unchecked is safe:
+        // - equation is done among same variable types
+        // - incrementation (state.slotB.numBlocks++) is fine for 584K years if
+        // we propose at every second
         unchecked {
             meta.id = b.numBlocks;
             meta.timestamp = uint64(block.timestamp);

diff --git a/packages/protocol/contracts/L1/libs/LibProving.sol b/packages/protocol/contracts/L1/libs/LibProving.sol
@@ -90,6 +90,9 @@ library LibProving {
         if (tid == 0) {
             tid = blk.nextTransitionId;
 
+            // Unchecked is safe:
+            // - Not realistic 2**32 different fork choice per block will be
+            // proven and none of them is valid
             unchecked {
                 ++blk.nextTransitionId;
             }

diff --git a/packages/protocol/contracts/L1/libs/LibTaikoToken.sol b/packages/protocol/contracts/L1/libs/LibTaikoToken.sol
@@ -25,7 +25,7 @@ library LibTaikoToken {
     {
         uint256 balance = state.taikoTokenBalances[msg.sender];
         if (balance < amount) revert L1_INSUFFICIENT_TOKEN();
-
+        // Unchecked is safe per above check
         unchecked {
             state.taikoTokenBalances[msg.sender] -= amount;
         }

diff --git a/packages/protocol/contracts/L1/libs/LibVerifying.sol b/packages/protocol/contracts/L1/libs/LibVerifying.sol
@@ -62,6 +62,10 @@ library LibVerifying {
                     >= type(uint96).max / config.ethDepositMaxCountPerBlock
         ) revert L1_INVALID_CONFIG();
 
+        // Unchecked is safe:
+        // - assignment is within ranges
+        // - block.timestamp will still be within uint64 range for the next
+        // 500K+ years.
         unchecked {
             uint64 timeNow = uint64(block.timestamp);
 
@@ -114,6 +118,11 @@ library LibVerifying {
         TaikoData.Transition memory tz;
 
         uint64 processed;
+
+        // Unchecked is safe:
+        // - assignment is within ranges
+        // - blockId and processed values incremented will still be OK in the
+        // next 584K years if we verifying one block per every second
         unchecked {
             ++blockId;
 

diff --git a/packages/protocol/contracts/L2/TaikoL2.sol b/packages/protocol/contracts/L2/TaikoL2.sol
@@ -277,6 +277,8 @@ contract TaikoL2 is EssentialContract, TaikoL2Signer, ICrossChainSync {
         returns (bytes32 prevPIH, bytes32 currPIH)
     {
         bytes32[256] memory inputs;
+
+        // Unchecked is safe because it cannot overflow.
         unchecked {
             // Put the previous 255 blockhashes (excluding the parent's) into a
             // ring buffer.
@@ -307,6 +309,10 @@ contract TaikoL2 is EssentialContract, TaikoL2Signer, ICrossChainSync {
         view
         returns (uint256 _basefee, uint64 _gasExcess)
     {
+        // Unchecked is safe because:
+        // - gasExcess is capped at uint64 max ever, so multiplying with a
+        // uint32 value is safe
+        // - 'excess' is bigger than 'issued'
         unchecked {
             uint256 issued = timeSinceParent * config.gasIssuedPerSecond;
             uint256 excess = (uint256(gasExcess) + parentGasUsed).max(issued);

diff --git a/packages/protocol/contracts/actors_privileges_deployments.md b/packages/protocol/contracts/actors_privileges_deployments.md
@@ -0,0 +1,67 @@
+# Actors, Privileges, and Upgradeable Procedures Documentation
+
+## Introduction
+
+This document provides a comprehensive overview of the actors involved in the smart contract system and outlines their respective privileges and roles.
+Different `roles` (we call them `domain`) are granted via `AddressManager` contract's `setAddress()` function. Idea is very similar Optimism's `AddressManager` except that we use the `chainId + domainName` as the key for a given address. We need so, because for bridging purposes, the destination chain's bridge address needs to be inculded signaling the messgae hash is tamper-proof.
+Every contract which needs some role-based authentication, needs to inherit from `AddressResolver` contract, which will serve as a 'middleman/lookup' by querying the `AddressManager` per given address is allowed to act on behalf of that domain or not.
+
+## 1. Domains (≈role per chainId)
+
+In the context of the smart contract system, various actors play distinct roles. Each actor is associated with specific responsibilities and privileges within the system. When there is a modifier called `onlyFromNamed` or `onlyFromNamed2`, it means we are checking access through the before mentioned contracts (`AddressResolver` and `AddressManager`), and one function maximum allows up to 2 domains (right now, but it might change when e.g.`DAO` is set up) can be given access.
+
+### 1.1 Taiko
+
+- **Role**: This domain role is given to TaikoL1 smart contract.
+- **Privileges**:
+  - Possibility to mint/burn the taiko token
+  - Possibility to mint/burn erc20 tokens (I think we should remove this privilege)
+
+### 1.2 Bridge
+
+- **Role**: This domain role is given to Bridge smart contracts (both chains).
+- **Privileges**:
+  - The right to trigger transfering/minting the tokens (on destination chain) (be it ERC20, ERC721, ERC1155) from the vault contracts
+  - The right to trigger releasing the custodied assets on the source chain (if bridging is not successful)
+
+### 1.3 ERCXXX_Vault
+
+- **Role**: This role is givne to respective token vault contracts (ERC20, ERC721, ERC1155)
+- **Privileges**:
+  - Part of token briding, the possibility to burn and mint the respective standard tokens (no autotelic minting/burning)
+
+## 2. Different access modifiers
+
+Beside the `onlyFromNamed` or `onlyFromNamed2` modifiers, we have others such as:
+
+### 2.1 onlyOwner
+
+- **Description**: Only owner can be granted access.
+- **Associated contracts**: TaikoToken, AddressManager, EtherVault
+
+### 2.2 onlyAuthorized
+
+- **Description**: Only authorized (by owner) can be granted access - the address shall be a smart contract. (`Bridge` in our case)
+- **Associated Actors**: EtherVault
+
+## 3. Upgradeable Procedures
+
+The smart contract system incorporates upgradeable procedures to ensure flexibility and security. These procedures adhere to the following principles:
+
+### 3.1 Deployment Scripts
+
+- Deployment scripts are visible in the `packages/protocol/scripts` folder, encompassing both deployment and upgrade scripts for easy reference and replication.
+
+### 3.2 Transparent Upgradeability
+
+- Upgradeability is based on the Transparent Upgradeability Proxy by OpenZeppelin, ensuring that contract upgrades are secure and transparent to all stakeholders.
+
+### 3.3 Ownership Transition
+
+- Currently, on testnets, some privileges (like `onlyOwner`) are assigned to externally owned accounts (EOAs) for easier testing. However, it is essential to note that `TimeLockController` contracts will be the owners at a later stage.
+
+## Conclusion
+
+Clear documentation of actors and their privileges, combined with robust upgradeable procedures, is essential for smart contract systems, especially for based rollups. This documentation ensures that all stakeholders understand their roles and responsibilities within the system and guarantees its adaptability and security over time.
+
+Please ensure that this document is kept up to date as changes are made to the smart contract system and its actors or privileges.
diff --git a/packages/protocol/contracts/libs/Lib1559Math.sol b/packages/protocol/contracts/libs/Lib1559Math.sol
@@ -13,6 +13,11 @@ import { SafeCastUpgradeable } from
 /// @title Lib1559Math
 /// @dev This library provides utilities related to the L2 EIP-1559
 /// implementation.
+/// See formulas described in the whitepaper
+/// https://taikoxyz.github.io/taiko-mono/taiko-whitepaper.pdf
+/// From section:  "9.6. Rate Limiting using EIP-1559."
+/// Additional info about the arithmetic formula:
+/// https://github.com/taikoxyz/taiko-mono/blob/main/packages/protocol/docs/L2EIP1559.md
 library Lib1559Math {
     using SafeCastUpgradeable for uint256;
 

diff --git a/packages/protocol/incident_response_plan.md b/packages/protocol/incident_response_plan.md
@@ -0,0 +1,57 @@
+# Incident Response Plan
+
+This document outlines the incident response plan for our smart contract system, addressing both ChainOps and SmartContract-related incidents. It provides a list of potential incidents and instructions on how to handle them effectively.
+
+## ChainOps-Related Incidents
+
+### 1. Congested Network
+
+**Description**: A congested network can lead to (slow transaction confirmations, higher gas fees, slashing provers) impacting the performance of the rollup.
+
+**Response**:
+
+1. Check Grafana Alerts: Monitor the Grafana dashboard at [Grafana Dashboard](https://grafana.test.taiko.xyz/) for alerts related to network congestion.
+2. Engineer on Duty: The engineer on duty should be alerted automatically through the monitoring system.
+3. Mitigation: If network congestion is detected, consider adjusting gas prices or scheduling transactions during off-peak times.
+
+### 2. Chain Head Number Stop Increasing
+
+**Description**: When the chain head stops, it indicates a potential issue with the operation of the network.
+
+**Response**:
+
+1. Grafana Alerts: Monitor Grafana for alerts regarding the chain head number.
+2. Engineer on Duty: The engineer on duty should receive automatic alerts.
+3. Investigation: Investigate the root cause by analyzing blockchain data and logs.
+4. Collaboration: Collaborate with blockchain network administrators if necessary for a solution.
+
+### 3. Latest Verified Block Number Stop Increasing
+
+**Description**: A halt in the increase of the latest verified block number may indicate a problem with the operation of the network.
+
+**Response**:
+
+1. Grafana Alerts: Keep an eye on Grafana alerts regarding the latest verified block number.
+2. Engineer on Duty: The engineer on duty should be automatically notified.
+3. Troubleshooting: Investigate the node's syncing process and take corrective actions to ensure it resumes.
+
+## SmartContract-Related Incidents
+
+### 1. Unforeseeable Smart Contract Issue
+
+**Description**: Unforeseeable issues with the smart contracts may arise, which were not identified during the audit.
+
+**Response**:
+
+1. Incident Report: Create a detailed incident report, including the symptoms, affected contracts, and any relevant transaction or event data.
+2. Escalation: Notify the development and audit teams for immediate attention.
+3. Isolation: If necessary, isolate the affected smart contracts or functions to prevent further damage.
+4. Analysis: Collaborate with the audit team to analyze and diagnose the issue.
+5. Resolution: Implement necessary fixes, upgrades, or rollbacks as per the audit team's recommendations.
+6. Communication: Keep stakeholders informed throughout the incident resolution process.
+
+## Conclusion
+
+This incident response plan ensures that potential incidents, whether related to ChainOps or SmartContracts, are promptly detected and addressed. The plan relies on monitoring tools like Grafana and the availability of an engineer on duty. In the case of unforeseeable smart contract issues, a systematic incident resolution process is in place to minimize the impact on the system's functionality and security.
+
+Regular testing and review of this plan are recommended to ensure its effectiveness in responding to incidents as the system evolves.
diff --git a/packages/protocol/script/upgrade/SetAddressManager.s.sol b/packages/protocol/script/upgrade/SetAddressManager.s.sol
@@ -0,0 +1,42 @@
+// SPDX-License-Identifier: MIT
+//  _____     _ _         _         _
+// |_   _|_ _(_) |_____  | |   __ _| |__ ___
+//   | |/ _` | | / / _ \ | |__/ _` | '_ (_-<
+//   |_|\__,_|_|_\_\___/ |____\__,_|_.__/__/
+
+pragma solidity ^0.8.20;
+
+import "forge-std/Script.sol";
+import "forge-std/console2.sol";
+import "@openzeppelin/contracts-upgradeable/utils/math/SafeCastUpgradeable.sol";
+import "./UpgradeScript.s.sol";
+
+interface IEssentialContract {
+    function setAddressManager(address newAddressManager) external;
+}
+/// @notice Each contract (which inherits EssentialContract) is having a
+/// setAddressManager() setter. In such case AddressManager needs to get
+/// changed, we need a quick way to update it.
+///
+/// Invokaction example:
+/// forge script SetAddressManager --sig "run(address,address)"  <address>
+/// <address>
+
+contract SetAddressManager is UpgradeScript {
+    function run(
+        address essentialContract,
+        address newAddressManager
+    )
+        external
+        setUp
+    {
+        IEssentialContract(essentialContract).setAddressManager(
+            newAddressManager
+        );
+        console2.log(
+            essentialContract,
+            " contract set a new AddressManagerAddress:",
+            address(newAddressManager)
+        );
+    }
+}
diff --git a/packages/protocol/script/upgrade/TransferOwnership.s.sol b/packages/protocol/script/upgrade/TransferOwnership.s.sol
@@ -0,0 +1,45 @@
+// SPDX-License-Identifier: MIT
+//  _____     _ _         _         _
+// |_   _|_ _(_) |_____  | |   __ _| |__ ___
+//   | |/ _` | | / / _ \ | |__/ _` | '_ (_-<
+//   |_|\__,_|_|_\_\___/ |____\__,_|_.__/__/
+
+pragma solidity ^0.8.20;
+
+import "forge-std/Script.sol";
+import "forge-std/console2.sol";
+import "@openzeppelin/contracts-upgradeable/utils/math/SafeCastUpgradeable.sol";
+import "./UpgradeScript.s.sol";
+
+interface IOwnable {
+    function transferOwnership(address newOwner) external;
+}
+/// @notice As "single" owner is not desirable for protocols we need to
+/// transfer ownership. BUT! Transferring ownership to a multisig also
+/// does not help too much if the protocol wants to give some time for
+/// the users to exit before an upgrade is effective. So implementing a
+/// delay (L2Beat prefers 7 days) is essential.
+/// So the usual approach is the following:
+/// 1. Transfer ownership to TimeLockController contract which enforces the
+/// delay
+/// 2. The ownership of the TimeLockController contract shall be a multisig/DAO
+
+/// Invokaction example:
+/// forge script TransferOwnership --sig "run(address,address)"  <address>
+/// <address>
+contract TransferOwnership is UpgradeScript {
+    function run(
+        address contractAddr,
+        address timeLockContract
+    )
+        external
+        setUp
+    {
+        IOwnable(contractAddr).transferOwnership(timeLockContract);
+        console2.log(
+            contractAddr,
+            " contract has a new owner:",
+            address(timeLockContract)
+        );
+    }
+}