Platform: Code4rena
Start Date: 04/01/2023
Pot Size: $60,500 USDC
Total HM: 15
Participants: 105
Period: 5 days
Judge: gzeon
Total Solo HM: 1
Id: 200
League: ETH
Rank: 51/105
Findings: 2
Award: $94.77
🌟 Selected for report: 1
🚀 Solo Findings: 0
🌟 Selected for report: 0x52
Also found by: 0xSmartContract, Deivitto, Diana, IllIllI, Koolex, Rolezn, SleepingBugs, V_B, adriro, betweenETHlines, cryptostellar5, oyc_109, peanuts
58.274 USDC - $58.27
SmartAccount.sol inherits from contracts that are not stateless and don't contain storage gaps which can be dangerous when upgrading
When creating upgradable contracts that inherit from other contracts is important that there are storage gap in case storage variable are added to inherited contracts. If an inherited contract is a stateless contract (i.e. it doesn't have any storage) then it is acceptable to omit a storage gap, since these function similar to libraries and aren't intended to add any storage. The issue is that SmartAccount.sol inherits from contracts that contain storage that don't contain any gaps such as ModuleManager.sol. These contracts can pose a significant risk when updating a contract because they can shift the storage slots of all inherited contracts.
Manual Review
Add storage gaps to all inherited contracts that contain storage variables
#0 - c4-judge
2023-01-17T07:55:17Z
gzeon-c4 marked the issue as duplicate of #352
#1 - livingrockrises
2023-01-19T17:35:28Z
Regarding storage gaps I want to bring this up.
SmartAccount first storage is Singleton(first inherited contract) that says // singleton slot always needs to be first declared variable, to ensure that it is at the same location as in the Proxy contract.
"In case of an upgrade, adding new storage variables to the inherited contracts will collapse the storage layout." Is this still valid?
#2 - c4-sponsor
2023-01-19T17:35:38Z
livingrockrises requested judge review
#3 - c4-sponsor
2023-02-07T11:49:38Z
livingrockrises marked the issue as sponsor acknowledged
#4 - c4-judge
2023-02-10T12:36:42Z
gzeon-c4 marked the issue as satisfactory
#5 - c4-judge
2023-02-10T12:39:23Z
gzeon-c4 marked the issue as selected for report
#6 - livingrockrises
2023-02-12T09:36:57Z
would like to hear judge's views on fixing this and upgradeability as a whole.
#7 - gzeon-c4
2023-02-12T16:20:25Z
would like to hear judge's views on fixing this and upgradeability as a whole.
Immediately actionable items would be to use the upgradable variant of OZ contract when available; ModuleManager should also be modified to include a storage gap.
Since the proxy implementation is upgradable by the owner (which can be anyone aka not a dev), it would be nice to implement UUPS like safe-rail (as mentioned in #352) to prevent the user upgrading to a broken implementation irreversibly.
Will also add that lack of storage gap is not typically a Med issue, but considering this contract have a mix of storage gapped and non-storage gapped base contract, and a risky upgrade mechanism, it is considered as Med risk in this contest.