safe-global / safe-smart-account

Safe allows secure management of blockchain assets.
https://safe.global
GNU Lesser General Public License v3.0
1.84k stars 907 forks source link

Feature: Enhance 1.4.1 contract checks for support proxy contract #683

Closed mmv08 closed 11 months ago

mmv08 commented 11 months ago

This PR:

github-actions[bot] commented 11 months ago

Pull Request Test Coverage Report for Build 6610678646


Totals Coverage Status
Change from base Build 6589591326: 0.02%
Covered Lines: 368
Relevant Lines: 377

💛 - Coveralls
mmv08 commented 11 months ago

Code looks good to me, as for the rationale, is it just we don't want to accidentally run this migration on a proxy contract that isn't the standard Safe proxy contract (and overwrite some important storage that the proxy contract uses for other things)?

Yes and no. A correct proxy implementation should use something other than this storage slot because things may break. In this case, it's more about running a migration transaction without migrating anything. For version 1.5.0, though, it gets more critical because the guard interface changes there to add a module transaction check. The contract also implements migration methods for such cases (upgrade singleton together with a guard), so it was needed to avoid an issue where the singleton stays the same, but the guard updates. See https://github.com/safe-global/safe-contracts/pull/652#discussion_r1331387586