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

Generalize Migration Contract #787

Closed
nlordell opened this issue Jul 5, 2024 · 0 comments · Fixed by #793
Closed

Generalize Migration Contract #787

nlordell opened this issue Jul 5, 2024 · 0 comments · Fixed by #793
Assignees

Comments

@nlordell
Copy link
Collaborator

nlordell commented Jul 5, 2024

Currently, the Safe 1.3.0 to 1.4.1 migration contract checks the addresses of the "target" contract. It requires the migration to either end up on the expected addresses of the Safe or SafeL2.

However, this does not work with zkSync contracts - since the address of the Safe is different (because of zkSync things).

We have two options here:

  1. Make a general "SafeMigration" library contract that does not check the target address matches a known copy.
  2. Check the target contract by code hash instead of address $^1$
  3. Add expected zkSync address and modify the address check to support it.

References

  1. Sample code hash check:
    bytes32 constant SAFE_CODE_HASH = keccak256(type(Safe).runtimeCode);
    bytes32 constant SAFEL2_CODE_HASH = keccak256(type(SafeL2).runtimeCode);
    
    function migrate(address singleton) public {
        bytes32 codeHash = singleton.codehash;
        require(codeHash == SAFE_CODE_HASH || codeHash == SAFEL2_CODE_HASH);
        
        // ...
    }

Other Considerations

Potentially provide a migrateUnsafe escape hatch.

@nlordell nlordell changed the title Add Support for zkSync Contracts to Migration Contract Generalize Migration Contract Jul 5, 2024
akshay-ap added a commit that referenced this issue Jul 16, 2024
akshay-ap added a commit that referenced this issue Jul 16, 2024
akshay-ap added a commit that referenced this issue Jul 16, 2024
akshay-ap added a commit that referenced this issue Jul 16, 2024
akshay-ap added a commit that referenced this issue Jul 16, 2024
…ntract tests, fix tests in Safe150Migration.spec.ts
akshay-ap added a commit that referenced this issue Jul 16, 2024
akshay-ap added a commit that referenced this issue Jul 16, 2024
akshay-ap added a commit that referenced this issue Jul 16, 2024
akshay-ap added a commit that referenced this issue Jul 16, 2024
akshay-ap added a commit that referenced this issue Jul 16, 2024
akshay-ap added a commit that referenced this issue Jul 17, 2024
Use BigInt instead of ethers.toBigInt
akshay-ap added a commit that referenced this issue Jul 19, 2024
akshay-ap added a commit that referenced this issue Jul 19, 2024
akshay-ap added a commit that referenced this issue Jul 19, 2024
akshay-ap added a commit that referenced this issue Jul 19, 2024
mmv08 added a commit that referenced this issue Jul 22, 2024
Fixes: #787

This PR adds a general migration contract that takes address of the
Safe, SafeL2 and fallback handler contracts during deployment. The
contract allows Safe to update the Singleton at `address(0)`.

- Uses error strings rather than error types because Solidity version
0.7.6 doesn't support it.

As of now tests cover below migration paths:
- 1.3.0 to 1.5.0
- 1.3.0 to 1.4.1
- 14.1 to 1.5.0

See `SafeMigration.spec.ts` to see how tests are organised. Do share if
you any thoughts to better run same tests on different migration paths.

The migration contract stores address of the Safe singletons and
fallback handler rather than using code hash and requiring the user to
provide singleton address as described in the issue. The reason being as
follows:

Checking codehash of the target singleton means user has to provide the
address of the target singleton. Also, checking code hash has higher gas
costs.

The only argument for using code hash for upgrades is that it also
allows unofficial singletons to be used for migration using official
migration contract. But, users/projects can also deploy their own
version of migration contract by providing singleton addresses in the
constructor and have similar security guarantees as the official
migration contract.

- Create a general migration contract which is not tightly bound to any
specific Safe version
- Update tests
- Remove other migration contract as this PR supersedes it

Unlike `Safe150Migration.sol`, this new contract does not check if
slot(0) of the contract stores an address having some non-empty code. I
think this check is not need because this contract is not intended to be
used in general by other proxy contracts and checking slot(0) value is
only a partially correct way. Would like to know thought of others.

---------

Co-authored-by: Nicholas Rodrigues Lordello <[email protected]>
Co-authored-by: Shebin John <[email protected]>
Co-authored-by: Mikhail <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants