Feature: Enhance 1.4.1 contract checks for support proxy contract

[mmv08]

This PR:

• Closes Update 1.4.1 migration contract with storage slot 0 checks and proper documentation  #672 by implementing the same checks as in the 1.5.0 contract that validates that the address stored at the storage slot 0 is a contract (conventional storage proxy)

[github-actions]

Pull Request Test Coverage Report for Build 6610678646

• 3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.02%) to 95.2%

---

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

---

💛 - Coveralls

[nlordell]

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)?

[mmv08]

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 #652 (comment)

[nlordell]

AFAIU, this can be removed because isContract(singleton) implies that this was called through a DELEGATECALL (since this contract doesn't set storage itself).

If that is the case, shouldn't address public immutable MIGRATION_SINGLETON be removed as it is no longer used?

[nlordell]

Also, on a general note, is it OK to have this kind of check be "implicit" or should it be more explicit (at the cost of a small amount of additional gas)?

[mmv08]

If that is the case, shouldn't address public immutable MIGRATION_SINGLETON be removed as it is no longer used?

Good catch, thanks. I will remove it

Also, on a general note, is it OK to have this kind of check be "implicit" or should it be more explicit (at the cost of a small amount of additional gas)?

I think it's OK to have an implicit check if it's explicitly mentioned somewhere. I will try to improve the documentation.

[nlordell]

Thanks! LGTM.