feat: deploy multisig wallets and timelock contract to address centralization related risks #92
Conversation
WalkthroughThis pull request introduces a new submodule for the "safe-smart-account" library, enhancing the project's modularity. It also adds governance documentation for a smart contract system, detailing a two-tier governance model, and introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MultisigWallet
participant CustomTimelockController
participant SafeSmartAccount
User->>MultisigWallet: Propose Action
MultisigWallet->>CustomTimelockController: Submit Proposal
CustomTimelockController->>SafeSmartAccount: Execute Action
SafeSmartAccount-->>User: Action Completed
Assessment against linked issues
Possibly related PRs
Poem
Tip New review modelWe have updated our review workflow to use the Anthropic's Claude family of models. Please share any feedback in the discussion post on our Discord. New featuresWalkthrough comment now includes:
Notes:
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
For reviewer's convenience, I list some of the main changes:
|
v1.3.0-libs.0
adu-web3
requested review from
magj2006,
bwhour,
cloud8little,
mikebraver and
MaxMustermann2
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (20)
- .gitmodules (1 hunks)
- docs/contract-governance.md (1 hunks)
- lib/safe-smart-account (1 hunks)
- remappings.txt (1 hunks)
- script/12_RedeployClientChainGateway.s.sol (1 hunks)
- script/14_CorrectBootstrapErrors.s.sol (1 hunks)
- script/15_DeploySafeMulstisigWallet.s.sol (1 hunks)
- script/2_DeployBoth.s.sol (1 hunks)
- script/7_DeployBootstrap.s.sol (1 hunks)
- script/BaseScript.sol (1 hunks)
- script/deployedMultisigWallets.json (1 hunks)
- script/integration/1_DeployBootstrap.s.sol (1 hunks)
- script/safe_contracts_on_exocore.json (1 hunks)
- src/storage/BootstrapStorage.sol (1 hunks)
- src/utils/CustomTimelockController.sol (1 hunks)
- test/foundry/ExocoreDeployer.t.sol (1 hunks)
- test/foundry/Governance.t.sol (1 hunks)
- test/foundry/unit/Bootstrap.t.sol (2 hunks)
- test/foundry/unit/ClientChainGateway.t.sol (1 hunks)
- test/foundry/unit/CustomProxyAdmin.t.sol (1 hunks)
Files skipped from review due to trivial changes (12)
- lib/safe-smart-account
- script/12_RedeployClientChainGateway.s.sol
- script/14_CorrectBootstrapErrors.s.sol
- script/2_DeployBoth.s.sol
- script/7_DeployBootstrap.s.sol
- script/BaseScript.sol
- script/integration/1_DeployBootstrap.s.sol
- src/storage/BootstrapStorage.sol
- test/foundry/ExocoreDeployer.t.sol
- test/foundry/unit/Bootstrap.t.sol
- test/foundry/unit/ClientChainGateway.t.sol
- test/foundry/unit/CustomProxyAdmin.t.sol
Additional context used
Gitleaks
script/safe_contracts_on_exocore.json
2-2: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
LanguageTool
docs/contract-governance.md
[uncategorized] ~36-~36: Possible missing article found.
Context: ...sig wallet, and the timelock period for timelock controller.(AI_HYDRA_LEO_MISSING_THE)
Additional comments not posted (14)
script/deployedMultisigWallets.json (1)
1-18: LGTM!The JSON file is well-structured and contains the necessary information about the multisig wallets and signers for the holesky and exocore networks. The changes are approved.
remappings.txt (1)
12-13: LGTM!The addition of the new mapping entry for
@safe-contractsis a good change that enhances the project's modularity by allowing the inclusion of safe smart contract functionalities. The changes are approved.script/safe_contracts_on_exocore.json (2)
1-12: LGTM!The JSON file is well-structured and contains the necessary information about the smart contract addresses deployed on the exocore network. The changes are approved.
Tools
Gitleaks
2-2: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
2-2: Ignore the static analysis warning.The static analysis tool has incorrectly flagged the Ethereum address at line 2 as a potential generic API key. This is a false positive and can be safely ignored.
Tools
Gitleaks
2-2: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
.gitmodules (1)
25-27: LGTM!The changes to add a new submodule for the "safe-smart-account" library are approved. This enhances the project's modularity by integrating an external library.
src/utils/CustomTimelockController.sol (1)
1-37: LGTM!The
CustomTimelockControllercontract is correctly implemented and enhances the governance model by introducing a circuit breaker role. The changes are approved.script/15_DeploySafeMulstisigWallet.s.sol (1)
1-72: LGTM!The script to deploy a Gnosis Safe multisig wallet on the Exocore testnet is correctly implemented. It sets up the owners, threshold, and other parameters correctly, reads the deployed Safe contracts from a JSON file, and creates a new Safe proxy. The changes are approved.
docs/contract-governance.md (1)
1-36: Excellent documentation of the governance model!The document provides a clear and comprehensive overview of the two-tier governance model using a custom timelock controller and multisig wallets. Key points:
- Privileged functions are accessible only to the contract owner and are used for configuration, pause/unpause, enabling messaging, and upgradeability.
- The two-tier governance structure consists of a
CustomTimelockControllerthat owns the business contract and Safe Multisig wallets acting as proposer/canceler, executor, circuit breaker, and admin.- The
CustomTimelockControllerinherits from OpenZeppelin'sTimelockControllerand introduces a circuit breaker role for emergency contract pauses without waiting for the timelock period.- Safe Multisig wallets are created using the Safe contracts, with the addresses provided in the deployment JSON file.
- Governance tests using fuzzing ensure the expected behavior of the governance model.
- In production, the Safe Multisig wallets will be set up with the appropriate roles, thresholds, signers, and timelock period.
The document is well-structured, easy to understand, and provides a solid foundation for implementing the governance model.
Tools
LanguageTool
[uncategorized] ~36-~36: Possible missing article found.
Context: ...sig wallet, and the timelock period for timelock controller.(AI_HYDRA_LEO_MISSING_THE)
test/foundry/Governance.t.sol (6)
35-128: LGTM: Contract structure and setup look good!The
GovernanceTestcontract is well-structured and imports the necessary dependencies. The state variables andsetUpfunction provide a solid foundation for the governance tests by:
- Defining
PlayerandSignaturestructs for signers and signatures.- Initializing state variables for signers, Gnosis Safe contracts, a custom timelock controller, and other contract instances.
- Deploying and setting up the necessary contracts, including the Gnosis Safe contracts, custom timelock controller, and client chain gateway.
The setup process is clear and comprehensive, ensuring that the contract is ready for testing the governance model.
130-158: LGTM: Client chain gateway deployment looks good!The
_deployClientChainGatewayfunction is responsible for deploying and setting up the necessary contracts for the client chain gateway. It follows a clear sequence of deployments and initializations:
- Deploys a beacon oracle by calling
_deployBeaconOracle.- Deploys vault and capsule implementations.
- Deploys vault and capsule beacons using the respective implementations.
- Deploys a beacon proxy bytecode contract.
- Deploys an ERC20 token with a fixed supply.
- Deploys a mock LayerZero endpoint.
- Deploys the client chain gateway logic contract with the necessary parameters.
- Deploys a proxy for the client chain gateway using the TransparentUpgradeableProxy pattern.
- Initializes the client chain gateway with the owner address.
The function is well-structured and sets up the client chain gateway correctly.
160-181: LGTM: Beacon oracle deployment looks good!The
_deployBeaconOraclefunction is responsible for deploying the beacon oracle contract. It follows these steps:
- Determines the genesis block timestamp based on the current chain ID. It supports mainnet, Goerli, Sepolia, and Holesky testnets.
- Reverts if the chain ID is not supported.
- Deploys a new instance of
EigenLayerBeaconOraclewith the determined genesis block timestamp.- Returns the deployed beacon oracle instance.
The function correctly handles the genesis block timestamp for different chains and deploys the beacon oracle contract with the appropriate timestamp.
183-234: LGTM: Fuzzing test for immediate pausing looks good!The
testFuzz_MultisigCanPauseImmediatelyfunction tests the scenario where the multisig can pause the gateway immediately using the circuit breaker role. It uses fuzzing to test different combinations of signers. The test follows these steps:
- Assumes that the
signersMaskis between 1 and 7 (inclusive) to ensure at least one signer and constrain to 3 bits.- Selects the Holesky testnet fork.
- Prepares a multisig transaction to call the
pausefunction on the timelock contract.- Selects the signers based on the
signersMaskusing theselectSignersfunction.- Signs the transaction using the selected signers.
- If there are enough signers (at least 2), it executes the multisig transaction and checks if the gateway is paused.
- If there are not enough signers, it expects the transaction to revert and checks that the gateway is not paused.
The test is well-structured and covers the cases where there are enough signers and where there are not enough signers. It ensures that the multisig can pause the gateway immediately using the circuit breaker role.
236-354: LGTM: Fuzzing test for timelock delay on unpausing looks good!The
testFuzz_MultisigNeedsDelayToUnpausefunction tests the scenario where the multisig needs to wait for the timelock delay to unpause the gateway. It uses fuzzing to test different combinations of signers for pausing, scheduling, and executing the unpause transaction. The test follows these steps:
- Assumes that each of the
pauseSignersMask,scheduleSignersMask, andexecuteSignersMaskis between 1 and 7 (inclusive).- Selects the Holesky testnet fork.
- Pauses the gateway using the
pauseSignersMaskand checks if the gateway is paused. If there are not enough signers, it expects the transaction to revert and exits the test.- Prepares an unpause transaction and schedules it using the
scheduleSignersMask. If there are not enough signers, it expects the transaction to revert and exits the test.- Tries to execute the unpause transaction immediately and expects it to revert.
- Waits for the timelock delay using
vm.warp.- Executes the unpause transaction using the
executeSignersMask.- If there are enough signers for execution, it checks if the gateway is unpaused. Otherwise, it expects the transaction to revert and checks that the gateway is still paused.
The test is well-structured and covers the cases where there are enough signers and where there are not enough signers for each step. It ensures that the multisig needs to wait for the timelock delay to unpause the gateway.
356-416: LGTM: Signer selection and multisig transaction signing look good!The
selectSignersandsignMultisigTransactionfunctions are well-implemented and serve their intended purposes in the governance tests.The
selectSignersfunction:
- Takes a
signersMaskparameter and returns an array of selectedPlayerinstances based on the mask.- Initializes an array of 3
Playerinstances and asignerCountvariable.- Checks each bit of the
signersMaskand adds the correspondingPlayerinstance to theselectedSignersarray if the bit is set.- Resizes the
selectedSignersarray to match the actual number of selected signers using assembly, avoiding unnecessary memory allocation.The
signMultisigTransactionfunction:
- Takes the transaction details and an array of
Playerinstances and returns the packed signature bytes.- Calculates the transaction hash using the
getTransactionHashfunction of the multisig contract.- Sorts the
signersarray based on their addresses using bubble sort.- Generates the sorted signatures by iterating over the sorted
signersarray, signing the transaction hash with each signer's private key, and packing the signatures.Both functions are correctly implemented and contribute to the effective testing of the governance model.
Description
closes: #81
To address centralization related risks, we should transfer ownership to a multi-sig wallet to avoid single point failure, and even utilize timelock contract to make important operations like upgrading a contract transparent for community.
here are the steps needed to achieve the goal:
CustomTimelockControllerSummary by CodeRabbit
New Features
Documentation
Bug Fixes
Tests