Incorrect implementation of storage gaps in Midas contracts

Summary

The Midas contracts are meant to be upgradable. Because of this, the developers have added storage gaps so future upgrades of the contracts will not compromise their storage. These gaps are applied incorrectly.

Vulnerability Detail

DepositVault, RedemptionVault and mTBILL declare an empty array for storage gaps AFTER their storage variables.

    /**
     * @dev leaving a storage gap for futures updates
     */
    uint256[50] private __gap;

This achieves nothing, as all of these contracts are the most derived in the inheritance chain. Their parent contracts - Blacklistable, WithMidasAccessControl, Greenlistable, Pausable and ManagableVault - all lack storage gaps.

The developer assumptions that the gap declared in the three most derived contract protect the storage is incorrect and when an update that changes the storage happens, the whole layout will become corrupted.

Impact

Impact = High (storage corruption causing unexpected behavior) Likelihood = Medium (can happen on every upgrade that makes changes to the storage layout) Severity = High x Medium = High

Code Snippet

DepositVault RedemptionVault mTBILL

Tool used

Manual Review

Recommendation

Declare storage gaps in the parent contracts as well.

Duplicate of #109

sherlock-audit / 2024-05-midas-judging

ZdravkoHr. - Incorrect implementation of storage gaps in Midas contracts #144