No Storage Gap For Upgradeable Contracts

Summary

Storage of DepositVault and RedemptionVault vaults might be corrupted during an upgrade.

Vulnerability Detail

Upgradeability involves inheritance but the inherited contract does not have a storage gap, only the contract that is inheriting has. The storage gap should be used in the base contract rather than in the child contract. That's because in solidity, in case of inheritance, the base contract's storage is the one that comes last, not first. First come the inherited contracts storage from left to right in the order of inheritance. https://github.com/sherlock-audit/2024-05-midas/blob/main/midas-contracts/contracts/DepositVault.sol#L26 In that example the ManageableVault storage comes first and DepositVault storage last

Impact

Storage collision in the child contracts in case the base contracts were upgraded, the vulnerable contracts are those that inherit from the base contracts that do not use a gap. The vulnerable contracts might not behave as expected.

List of the vulnerable child contracts:
- RedemptionVault
- DepositVault
- mTBILL
- Blacklistable
- Greenlistable
- Pausable
- ManageableVault
- DataFeed
List of the base contracts that lack a gap:
- Blacklistable
- ManageableVault
- WithMidasAccessControl
- Greenlistable
- Pausable (Although I doubt it will be upgraded)
  Code Snippet
  
  Here's an occurence of the vulnerability:

abstract contract ManageableVault is Greenlistable, Pausable, IManageableVault {
    using EnumerableSet for EnumerableSet.AddressSet;
    using DecimalsCorrectionLibrary for uint256;
    using SafeERC20 for IERC20;

     //...
    /**
     * @notice address to which USD and mTokens will be sent
     */
    address public tokensReceiver;

    /**
     * @dev tokens that can be used as USD representation
     */
    EnumerableSet.AddressSet internal _paymentTokens;

    ///@audit should declare a gap variable here to prevent froml storage collision

    /**
     * @dev checks that msg.sender do have a vaultRole() role
     */
    modifier onlyVaultAdmin() {
        _onlyRole(vaultRole(), msg.sender);
        _;
    }

    /**
     * @dev upgradeable pattern contract`s initializer
     * @param _ac address of MidasAccessControll contract
     * @param _mTBILL address of mTBILL token
     * @param _tokensReceiver address to which USD and mTokens will be sent
     */
    // solhint-disable func-name-mixedcase
    function __ManageableVault_init(
        address _ac,
        address _mTBILL,
        address _tokensReceiver
    ) internal onlyInitializing {
        require(_mTBILL != address(0), "zero address");
        require(_tokensReceiver != address(0), "zero address");
        require(_tokensReceiver != address(this), "invalid address");

        mTBILL = IMTbill(_mTBILL);
        __Greenlistable_init(_ac);
        __Pausable_init(_ac);

        tokensReceiver = _tokensReceiver;
    }

Tool used

Manual Review

Recommendation

Fix the bug by declaring a gap storage variable to prevent from storage collision

Duplicate of #109

sherlock-audit / 2024-05-midas-judging

0xjarix - No Storage Gap For Upgradeable Contracts #48