Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[EPIC] Support migrations and recoveries in IBC contracts #12

Open
3 of 4 tasks
srdtrk opened this issue Aug 2, 2024 · 2 comments
Open
3 of 4 tasks

[EPIC] Support migrations and recoveries in IBC contracts #12

srdtrk opened this issue Aug 2, 2024 · 2 comments
Assignees
Labels
epic Large body of work towards a product feature type: feature Feature request

Comments

@srdtrk
Copy link
Member

srdtrk commented Aug 2, 2024

Description

solidity-ibc-eureka is built under the assumption that each Cosmos chain will have their own deployment, as such, the contracts should be migrated the governance of these chains. (Note that this means that in theory ICS02Client.sol can be removed.) There are two types of migrations we need to think about:

  1. Adding new features to IBC while IBC is functioning normally. (We call these upgrades.)
  2. Recovering from a broken state such as a frozen/expired light client, or any other issue that shuts down IBC.

These two cases should be handled separately because in the first case, we don't need to introduce any additional trust assumptions.

Upgrades

Since IBC is still functioning in this case, we can use IBC to upgrade the contracts. We can give the authority to upgrade IBC contracts to the validator set of the counterparty tendermint chain. This can be done via an interchain accounts or contract call IBC application.

Recovering

In this case, we cannot use an on-chain light client or any other type of trustless mechanism as we are working under the assumption that IBC itself is unusable. This means that we can no longer trust the light client representing the cosmos chain (for example, it may be frozen or expired). Then we propose that in this case, a security council (multisig) takeover the admin privileges. However, we don't want the security council to have the ability to steal any funds while IBC is functioning properly, this is why we propose:

  1. Security council only has admin rights if light client is frozen/expired or there hasn't been any successful transfers in 2 weeks.
  2. While IBC is active (i.e. light client is active and there are successful transfers), the Cosmos chain's governance module has the right to change the security council.

Proposal

We should study the docs and tools here to add migrations to our contracts. We should also probably move IBCStore to it's own contract

Tasks

@srdtrk srdtrk added enhancement Improvements type: feature Feature request and removed enhancement Improvements labels Aug 2, 2024
@srdtrk srdtrk added the needs discussion This issue needs more discussion before its implementation label Aug 20, 2024
@womensrights womensrights added the epic Large body of work towards a product feature label Nov 18, 2024
@womensrights womensrights moved this to Backlog in IBC-GO Eureka Nov 18, 2024
@srdtrk srdtrk changed the title Add proxy contracts and migrations to the IBC contracts Support migrations and recoveries in IBC contracts Nov 19, 2024
@womensrights womensrights moved this from Backlog to In progress in IBC-GO Eureka Dec 11, 2024
@sangier
Copy link
Contributor

sangier commented Dec 13, 2024

Reposting here for persistency:

This PoC PR introduces a possible design of a governance framework to address #12 for the solidity-ibc-eureka contracts using a multisig approach based on Safe. The key features implemented include:

  1. Multisig (Safe): Critical administrative actions (e.g., upgrades, ownership transfers, pause) require approval from a quorum of pre-defined signers.

  2. Upgradeable: The contracts use OpenZeppelin's Transparent Proxy for upgradeability, enabling secure and controlled updates to the system's logic without disrupting its state.

  3. Pausable: Leveraging OpenZeppelin's Pausable module, this feature allows authorized actors to temporarily halt IBC operations during emergencies for quick mitigation of potential exploits or failures.

These components together enhance the security, flexibility, and decentralization of the IBC Solidity contracts, laying the groundwork for a robust governance model.

PoC Architecture:

upgrades

Notes

  • the core of the PoC is the MultisigIBCUpgradeTest.t.sol. This test files shows how to wire all the new additions during deployment.
  • original safe contracts have been modified to be compiled at the current repo version 0.8.28 by introducing the clause assembly ("memory-safe") before assembly code.
  • IBC contracts changes:
  1. add Openzeppelin Pausable contract and whenNotPaused modifier for external functions.
  2. change the constructor and define an initialization functions for proxy functionalities.

TimeLock

The Timelock mechanism is not currently implemented in this PoC but could be introduced in the future to enhance transparency specifically for upgrades. By delaying the execution of administrative actions, it would allow stakeholders to review proposed changes and raise objections if necessary. This would work well for upgrades but should not be applied to emergency operations, such as pausing, to avoid delays in critical scenarios. Balancing the delay duration would be key to maintaining flexibility while ensuring community trust

Security Consideration

Using proxy contracts requires careful security consideration. The aspect to be considered includes but are not limited to:

  • Access Control - Clearly define a model of who should able to call which functions.
  • Storage Collision among proxy and implementatinos
  • Initialization (possible frontrunning and manipulation)

For an initial review of potential security concern refer to 1,2,3,4.

@sangier
Copy link
Contributor

sangier commented Dec 13, 2024

Some thoughts on the storage collision topic that can lead to very subtle vulnerabilities.

Storage Considerations in Upgradeable Proxy Contracts

In transparent upgradeable proxy contracts, the proxy and implementation contracts have distinct roles: the proxy manages persistent state and the upgrade logic, while the implementation provides the contract specific logic and the storage layout that is mapped into the proxy persistent state. This separation enables contract upgrades while maintaining the existing state. However, the design introduces specific storage considerations to ensure compatibility and prevent collisions during upgrades.

The proxy contract does not explicitly declare state variables for the implementation's data. Instead, when a function is executed via delegatecall, the implementation contract’s logic operates on the proxy contract's storage which is defined in the implementation contract. This works because delegatecall executes the called function in the context of the proxy, meaning:

  • The proxy’s storage is used for all state variables.
  • The implementation’s storage layout defines how the proxy’s storage slots are accessed.

For example, if the implementation declares a uint256 variable x as its first state variable, the proxy’s storage slot 0 is used to store x. When the proxy delegates a call to the implementation, any reads or writes to x affect slot 0 in the proxy’s storage.

Layout Consistency and Storage Collisions

Since the proxy contract persists the state, maintaining a consistent storage layout across upgrades is crucial. When upgrading to a new implementation, the new contract must follow the existing storage layout. Any mismatch can cause storage collisions, potentially overwriting critical data.

Storage collisions occur when variables in the proxy or implementation contracts conflict over the same storage slot. This can lead to corrupted or overwritten data, undermining contract functionality. To Avoid Storage Collisions:

  • Reserve storage slots in the initial implementation contract using gap variables.
  • Follow standards like EIP-1967, for storage slots management.

To safely introduce new state variables in future upgrades, you can reserve unused storage slots in the initial implementation. This approach ensures that new variables in upgraded implementations will not overwrite existing data.

Example: Reserved Storage Slots

LogicV1 (Initial Implementation):

contract LogicV1 {
    uint256 public x;  // Stored at slot 0
    uint256 public y;  // Stored at slot 1
    uint256[50] private __gap;  // Reserved slots 2-51
}

In LogicV1, the __gap variable reserves 50 storage slots for future use. This reservation ensures that new variables in future implementations will occupy these slots without affecting existing variables.

LogicV2 (Upgraded Implementation):

contract LogicV2 is LogicV1 {
    uint256 public z;  // Uses slot 2 from the reserved __gap // Can be any type but MUST Fit in the reserved slot. 
}

When upgrading to LogicV2, the proxy retains its existing storage:

  • Slot 0: x
  • Slot 1: y
  • Slot 2: z (new variable added in LogicV2)

Implication for ERC20 escrow contracts in ICS20Tranfser

The ICS20 Transfer implementation, under the Eureka upgrade model, stores the ERC-20 contract addresses directly in the proxy's storage, thus they will persist across upgrades. This is because the proxy contract retains the state, while the new implementation logic modifies how that state is interpreted or utilized. For example, if an ERC-20 contract address mapping was added in an earlier implementation, the addresses will remain intact post-upgrade. As a result, even after upgrading the logic, the functionality of existing transfers and interactions with those ERC-20 contracts will be preserved without needing to reinitialize or reconfigure the state.

However some consideration around ownership needs to be done:
If I am not wrong, the current implementation sets the ownership of ERC20 contracts to the ICS20Tranfser implementation. Some investigation is need to verify if this is a required approach for the correct functioning of the eureka contracts.

  • If needed : Consider that with this approach, the initialize function of the v2 logic should include the ERC20.tranfserOwenserhip("newICS20TranfeerLogic") for the erc20 to preserve the erc20 admin functionalities.
  • if not needed: Consider an alternative approach like assigning the ownership of these ERC20 contracts to the ICS20TransferProxy. This will not require to transfer ownership during the new logic initialization.

@womensrights womensrights assigned srdtrk and unassigned sangier Dec 18, 2024
@srdtrk srdtrk removed the needs discussion This issue needs more discussion before its implementation label Dec 20, 2024
@gjermundgaraba gjermundgaraba changed the title Support migrations and recoveries in IBC contracts [EPIC] Support migrations and recoveries in IBC contracts Jan 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
epic Large body of work towards a product feature type: feature Feature request
Projects
Status: In progress
Development

No branches or pull requests

3 participants