Corruptible Upgradability Pattern

Summary

Storage of vault contracts (e.g. DepositVault, RedemptionVault, ...) contracts might be corrupted during an upgrade.

Vulnerability Detail

Following is the inheritance of the DepositVault/RedemptionVault contracts.

Note: The contracts highlighted in Orange mean that there are no gap slots defined. The contracts highlighted in Green mean that gap slots have been defined

graph BT;
    classDef nogap fill:#f96;
    classDef hasgap fill:#99cc00;
    DepositVault:::hasgap-->ManageableVault:::hasgap
    RedemptionVault:::hasgap-->ManageableVault:::hasgap
    ManageableVault:::hasgap-->Pausable:::nogap
    ManageableVault:::hasgap-->Greenlistable:::nogap
    ManageableVault:::hasgap-->Blacklistable:::nogap
    ManageableVault:::hasgap-->WithSanctionsList:::nogap
    Pausable:::nogap-->WithMidasAccessControl:::gap
    Greenlistable:::nogap-->WithMidasAccessControl:::gap
    Blacklistable:::nogap-->WithMidasAccessControl:::gap
    WithSanctionsList:::nogap-->WithMidasAccessControl:::hasgap

The vault contracts are meant to be upgradeable. However, it inherits contracts that are not upgrade-safe.

The gap storage has been implemented on the DepositVault/RedemptionVault/ManageableVault/WithMidasAccessControl.

However, no gap storage is implemented on Pausable/Greenlistable/Blacklistable/WithSanctionsList. Among these contracts, Pausable/Greenlistable/WithSanctionsList are contracts with defined variables (non pure-function), and they should have gaps as well.

Without gaps, adding new storage variables to any of these contracts can potentially overwrite the beginning of the storage layout of the child contract, causing critical misbehaviors in the system.

Note that during the last sherlock audit, this was also reported as an issue. It was fixed by adding gaps to the non-pure contracts. However, since that audit, new contracts and new variables are introduced, so this issue occurs again.

Also, CustomAggregatorV3CompatibleFeed does not have gaps but is inherited by MBasisCustomAggregatorFeed/MTBillCustomAggregatorFeed. If the feed wants to be upgradeable, CustomAggregatorV3CompatibleFeed should also have gaps.

Impact

Storage of vault contracts might be corrupted during upgrading.

Code Snippet

Tool used

Manual review

Recommendation

Add gaps for non pure-function contracts: Pausable/Greenlistable/WithSanctionsList/CustomAggregatorV3CompatibleFeed.

sherlock-admin2 commented 3 months ago

1 comment(s) were left on this issue during the judging contest.

merlinboii commented:

Known Issue from the past audit contest and as the past issue there is no gap for several functions and the platform should to fix with the contract that they is decided to more potential to be changes. Assume that it is known issue and sponspor intended to design which contracts that they shoul introduce the gap.