Reach out for an audit or to learn more about Macro
or Message on Telegram

mStable A-1

Security Audit

January 11th, 2023

Version 1.0.0

Presented by 0xMacro

Table of Contents

Introduction

This document includes the results of the security audit for mStable's smart contract code as found in the section titled ‘Source Code’. The security audit was performed by the Macro security team from November 28, 2022 to December 9, 2022.

The purpose of this audit is to review the source code of certain mStable Solidity contracts, and provide feedback on the design, architecture, and quality of the source code with an emphasis on validating the correctness and security of the software in its entirety.

Disclaimer: While Macro’s review is comprehensive and has surfaced some changes that should be made to the source code, this audit should not solely be relied upon for security, as no single audit is guaranteed to catch all possible bugs.

Overall Assessment

The following is an aggregation of issues found by the Macro Audit team:

Severity Count Acknowledged Won't Do Addressed
High 2 - - 2
Medium 4 - 1 3
Low 2 - 1 1
Code Quality 3 - - 3
Informational 3 3 - -
Gas Optimization 3 - - 3

mStable was quick to respond to these issues.

Specification

Our understanding of the specification was based on the following sources:

Source Code

The following source code was reviewed during the audit:

Specifically, we audited the following contracts within this repository:

Source Code SHA256
contracts/vault/AbstractVault.sol

7623701c2234742bc043b96016f9ef5508dba5f626091df6bbb1b74935dace0c

contracts/vault/allocate/PeriodicAllocationAbstractVault.sol

d6b319599500c2f0b9acff26e83cafdd9fd6742410078d4286b7d99ad8e4e9a1

contracts/vault/allocate/AssetPerShareAbstractVault.sol

483ec1a9f1c6d398441e5d3886ff1bcd5d76b9a6c4c29abc0a1bf8e735544875

contracts/vault/allocate/SameAssetUnderlyingsAbstractVault.sol

418d33126adb711c39ae23d58428acb0c378e2ad5c28d0147b1ecdb64c1f04bb

contracts/vault/meta/PeriodicAllocationPerfFeeMetaVault.sol

4507a3b321737ce9e6f75b3a10a0de036c3657f3d15e9ece2870bdb0c42c64f1

contracts/vault/fee/PerfFeeAbstractVault.sol

c4a567f1443925365da35f93bf567dc78e3ec8e787ec9d8485cccb675a963fa8

contracts/vault/fee/FeeAdminAbstractVault.sol

2879b80ea10984690d544958564bfcf509487ce47f224e433843097d3160c4b5

contracts/tokens/InitializableToken.sol

f8ed1ea18ab2a002d0f156727bd6b4e52756a2344186e925de271e7a151241cd

contracts/tokens/InitializableTokenDetails.sol

c319eb4157eca3baa168448351d1f550af10d46b79ca1ec61d38f71af44b88de

contracts/shared/VaultManagerRole.sol

e7d069e0cb24297ed75bfe8e97837ed08af69cc20472a3620290a3c478fffef1

contracts/shared/ImmutableModule.sol

726f6c9ac03a59fc88a2d9743feb6a729b5055a93a5ec8a266634c10839d17a5

contracts/shared/ModuleKeys.sol

53e5a9cef26a8ef34009b14cacadf20d9426240b22909d876992f63ee76c3054

contracts/shared/SingleSlotMapper.sol

6b29ace1609c275696b6a795a8eb37b50dfd04a7ffdbf12dab9c0ad6725d5285

Note: This document contains an audit solely of the Solidity contracts listed above. Specifically, the audit pertains only to the contracts themselves, and does not pertain to any other programs or scripts, including deployment scripts.

Issue Descriptions and Recommendations

Click on an issue to jump to it, or scroll down to see them all.

Security Level Reference

We quantify issues in three parts:

  1. The high/medium/low/spec-breaking impact of the issue:
    • How bad things can get (for a vulnerability)
    • The significance of an improvement (for a code quality issue)
    • The amount of gas saved (for a gas optimization)
  2. The high/medium/low likelihood of the issue:
    • How likely is the issue to occur (for a vulnerability)
  3. The overall critical/high/medium/low severity of the issue.

This third part – the severity level – is a summary of how much consideration the client should give to fixing the issue. We assign severity according to the table of guidelines below:

Severity Description
(C-x)
Critical

We recommend the client must fix the issue, no matter what, because not fixing would mean significant funds/assets WILL be lost.

(H-x)
High

We recommend the client must address the issue, no matter what, because not fixing would be very bad, or some funds/assets will be lost, or the code’s behavior is against the provided spec.

(M-x)
Medium

We recommend the client to seriously consider fixing the issue, as the implications of not fixing the issue are severe enough to impact the project significantly, albiet not in an existential manner.

(L-x)
Low

The risk is small, unlikely, or may not relevant to the project in a meaningful way.

Whether or not the project wants to develop a fix is up to the goals and needs of the project.

(Q-x)
Code Quality

The issue identified does not pose any obvious risk, but fixing could improve overall code quality, on-chain composability, developer ergonomics, or even certain aspects of protocol design.

(I-x)
Informational

Warnings and things to keep in mind when operating the protocol. No immediate action required.

(G-x)
Gas Optimizations

The presented optimization suggestion would save an amount of gas significant enough, in our opinion, to be worth the development cost of implementing it.

Issue Details

H-1

removeVault() will block the governor from removing underlying vaults

Topic
Protocol Design
Status
Impact
High
Likelihood
High

In SameAssetUnderlyingsAbstractVault.sol, removeVaults() accepts a vaultIndex to remove the corresponding vault from _activeUnderlyingVaults, and the index is required to be less than the length of _activeUnderlyingVaults.

uint256 newUnderlyingVaultsLen = _activeUnderlyingVaults.length - 1;
require(vaultIndex <= newUnderlyingVaultsLen, "Invalid from vault index");

We also noticed the same vaultIndex is used to get the underlyingVaultIndex from the vaultIndexMap later.

uint256 underlyingVaultIndex = vaultIndexMapMem.map(vaultIndex);

This means if the governor wants to remove the nth vault in the vaultMap, it requires having at least n active vaults. However, this is not always the case, as previously removed vaults will still occupy their indexes on the map, and that will block the governor from removing active vaults. Since there is a limit of total active vaults, the issue will also block the governor from adding new vaults once after adding 15 vaults

Remediations to Consider

  • Change the require statement at line 251 to use the map’s indexes count and not active underlying vaults length.
H-2

Meta vault doesn’t fully comply with ERC-4626 standard

Topic
Coding Standards
Status
Impact
Spec Breaking
Likelihood
High

EIP-4626 states the following (see here):

Finally, EIP-4626 Vault implementers should be aware of the need for specific, opposing rounding directions across the different mutable and view methods, as it is considered most secure to favor the Vault itself during calculations over its users:

  • If (1) it’s calculating how many shares to issue to a user for a certain amount of the underlying tokens they provide or (2) it’s determining the amount of the underlying tokens to transfer to them for returning a certain amount of shares, it should round down.
  • If (1) it’s calculating the amount of shares a user has to supply to receive a given amount of the underlying tokens or (2) it’s calculating the amount of underlying tokens a user has to provide to receive a certain amount of shares, it should round up.

As stated above, ERC-4626 expects the result returned from the following functions of AbstractVault.sol to be rounded up:

function _previewWithdraw(uint256 assets) 
    internal view virtual 
    returns (uint256 shares)

function _previewMint(uint256 shares) 
    internal view virtual 
    returns (uint256 assets)

Due to how Solidity division works, the current implementation of those two functions currently rounds the result down - thus not conforming to ERC-4626 standard. The other function behave as expected by the ERC-4626 standard.

Not fully conforming to ERC-4626 might cause some integration problems for other protocols that can lead to wide range of issues for both parties.

Remediations to Consider

  • Consider to change the _previewWithdraw and _previewMint function of AbstractVault.sol (and any overriden functions) accordingly, so that the result is rounded up. A reference implementation from OpenZeppelin can be found here. Check out line 114 and 119 where the results are rounded up.
M-1

Incorrect calculation of assetsPerShare when underlying vault is paused

Topic
Use Cases
Status
Wont Do
Impact
High
Likelihood
Low

In SameAssetUnderlyingsAbstractVault.sol, the function totalAssets() calculates the number of assets in the underlying vaults via .maxWithdraw function.

See _totalUnderlyingAssets function:

function _totalUnderlyingAssets() internal view returns (uint256 totalUnderlyingAssets) {
        // Get the assets held by this vault in each of in the active underlying vaults
        uint256 len = _activeUnderlyingVaults.length;

        for (uint256 i = 0; i < len; ) {
            totalUnderlyingAssets += _activeUnderlyingVaults[i].maxWithdraw(address(this));
            unchecked {
                ++i;
            }
        }

If an underlying vault is paused, .maxWithdraw returns 0, thus lowering the number of total assets calculated in the totalAssets() function. As totalAssets() is used to calculate the assetsPerShare value, pausing an underlying vault affects the asset-to-share ratio. In particular, e.g. a deposit will return an increased portion of shares.
Thus, while one of the underlying vaults is paused, a user can deposit assets and will receive an increased number of shares as opposed to when all underlying vaults are unpaused.

Remediations to Consider

Dont rely on .maxWithdraw function when calculating the total number of asset in the underlying vaults. Instead, the .previewRedeem function could be used to retrieve the actual number of assets for an underlying vault.

Response by mStable

Since we control the pause functionality over the underlying vaults in the current scope, this is not an issue. We will pause/un-pause meta vaults and underlying vaults simultaneously. At the same time, we appreciate Macro for pointing this out because this is important for future meta vaults (where we can have 3rd party underlying vaults)

M-2

Meta vaults cannot re-add underlying vaults after removal

Topic
Protocol Design
Status
Impact
Medium
Likelihood
High

In line 238 of SameAssetUnderlyingsAbstractVault.sol, assets are approved to the max uint256 amount using SafeERC20.safeApprove. According to the documentation within SafeERC20.sol:

safeApprove should only be called when setting an initial allowance,
or when resetting it to zero. To increase and decrease it, use
'safeIncreaseAllowance' and 'safeDecreaseAllowance'

If the vault manager removes an underlying vault and tries to re-add it, the transaction will revert.

Remediation to Consider

  • When removing a vault, consider resetting the allowance to 0 using _asset.safeApprove(underlyingVault, 0)
M-3

Redemption and mint may return an incorrect amount

Topic
Incentive Design
Status
Impact
Medium
Likelihood
Medium

A user is able to deposit assets for shares and redeem them subsequently by calling deposit and redeem , respectively. The redeem function:

  1. Calculates the assets to redeem by calling _previewRedeem which uses the current assetsPerShare
  2. Updates the assetsPerShare using _checkAndUpdateAssetPerShare
  3. Transfer assets and burn the shares

In the _previewRedeem function, the shares-to-assets amount is calculated using _convertToAssets :

function _convertToAssets(uint256 shares)
        internal
        view
        virtual
        override
        returns (uint256 assets)
  {
        assets = (shares * assetsPerShare) / ASSETS_PER_SHARE_SCALE;
  }

The issue occurs when the underlying vaults assets change but the metavault does not update assetsPerShare. When this happens, and no actions occur in which _checkAndUpdateAssetPerShare gets called, users will be redeeming assets proportional to an outdated assetsPerShare.

A similar issue can also be found in the _mint function that warrants a remedy as well:

function _mint(uint256 shares, address receiver)
    internal
    virtual
    override
    returns (uint256 assets)
{
    assets = _previewMint(shares);
    _checkAndUpdateAssetPerShare(assets);
    _transferAndMint(assets, shares, receiver, false);
}

This issue is rated as medium taking into account the scenario that the assetPerShare fails to update.

Remediation to Consider

  • Move _checkAndUpdateAssetPerShare(assets) to before _previewRedeem(shares) in _redeem
  • Move _checkAndUpdateAssetPerShare(assets) to before _previewMint in _mint
M-4

No upper bound for the initialization of performance fee

Topic
Input Validation
Status
Impact
High
Likelihood
Low

In line 45 of PerfFeeAbstractVault.sol, the performanceFee can be initialized with a uint256 value. The store performanceFee should be a number scaled to 6 decimals to represent the percentage shares to mint as a fee. It should have max value of 1000000 , which represents 100%.

Currently, there is no check to make sure that value initialized for performanceFee is less than 1000000 (similar to what is in setPerformanceFee).

The impact of this issue is rated high because if the fee is set sufficiently high, it may not be changeable. This is due to line 118 where _chargePerformanceFee is called before a new fee is set. The function _chargePerformanceFee may use the erroneously high performanceFee in an attempt to calculate feeShares. This will cause an overflow and revert.

Remediation to Consider

  • Adding a similar require check as the setPerformanceFee in the _initialize function
        require(_performanceFee <= FEE_SCALE, "Invalid fee");
    
L-1

Paused vault can be removed even if it holds assets

Topic
Use Cases
Status
Wont Do
Impact
Medium
Likelihood
Low

In SameAssetUnderlyingsAbstractVault.sol, the removeVault function includes the following:

// Withdraw all assets from the underlying vault being removed.
uint256 underlyingShares = _activeUnderlyingVaults[underlyingVaultIndex].maxRedeem(   
    address(this)
);
if (underlyingShares > 0) {    
    _activeUnderlyingVaults[vaultIndex].redeem(
        underlyingShares,
        address(this),
        address(this)
    );
}

In case the underlying vault is paused, maxRedeem returns 0 and thus redeeming the assets will be skipped. Subsequently, the vault will be removed.

As a consequence, paused vaults will be removed but the underlying assets will not be redeemed. This results in locked assets in the underlying vault and an increased assetsPerShare value so that users will receive an increased portion of shares when depositing.

Remediations to Consider

Note that the issue can be manually remedied by re-adding the vault, unpausing it, and then removing the unpaused vault. This lowers the issue from High to Medium severity.

Consider the following for the issue to not occur at all:

  • Only allow vault removal for unpaused vaults
Response by mStable

Very much on the tangent of [M-1]. Furthermore, as Macro noted, this can be fixed by re-adding the vault, unpausing and removing. Also, we want to interact with underlying vaults using only IERC4626 methods.

L-2

removeVault() may block single source withdraws

Topic
Protocol Design
Status
Impact
Medium
Likelihood
Low

In SameAssetUnderlyingsAbstractVault.sol, removeVaults() accepts a vaultIndex to remove the corresponding vault from _activeUnderlyingVaults, but there is no check that assures the removed vault is not the single source vault.

In PeriodicAllocationAbstractVault.sol, _sourceAssets() is used to withdraw assets from underlying vaults, and amounts smaller or equal to singleVaultSharesThreshold are withdrawn from the single source vault.

Since resolveVaultIndex() reverts for non-active vaults, deactivating that vault will block all single source withdraws.

Remediations to Consider

  • Consider checking if the removed vault is the single source vault.
Q-1

Inconsistent zero address check for vault manager

Topic
Code Quality
Status
Quality Impact
Low

Generally, it is not required to check for address zero. However, in VaultManagerRole.sol, the functionsetVaultManager checks for address(0), but _initialize does not.

Remediation to Consider

  • Line 66 can be removed to increase code consistency and decrease code size, or
  • Adding an address(0) check (similar to to the functionsetVaultManager) for code consistency. However, if there is an emphasis on code size, this is not recommended.
Q-2

Unused imports

Topic
Code Quality
Status
Quality Impact
Low

The following imports in PerfFeeAbstractVault.sol are not required and can be removed:

import { AbstractVault } from "../AbstractVault.sol"
import { VaultManagerRole } from "../../shared/VaultManagerRole.sol";
Q-3

Misleading comment

Topic
Code Quality
Status
Quality Impact
Low

In SameAssetUnderlyingsAbstractVault.sol, rebalance() is explained as follows

/**
 * @notice `VaultManager` rebalances the assets in the underlying vaults.
 * This can be moving assets between underlying vaults, moving assets in underlying
 * vaults back to this vault, or moving assets in this vault to underlying vaults.
 */

However, the function can not be used for moving assets in underlying vaults back to this vault, or moving assets in this vault to underlying vaults. In our discussion with mStable team, they confirmed this by stating:

When withdrawing assets from one vault to the other , the metavault is only a transit point

I-1

Storage can get corrupted on upgrade

Topic
Upgradability
Status
Acknowledged
Impact
Informational

For upgradable contracts, it is recommended to implement storage gaps (__gap).

Storage gaps are a convention for reserving storage slots in a base contract, allowing future versions of that contract to use up those slots without affecting the storage layout of child contracts.

In particular, if a new variable is added in a parent contract, the additional variable can overwrite the beginning of the storage layout of the child contract.

Lets consider the following example:

abstract contract AssetPerShareAbstractVault is AbstractVault {
    /// @notice Scale of the assets per share. 1e26 = 26 decimal places
    uint256 public constant ASSETS_PER_SHARE_SCALE = 1e26;

    /// @notice Assets per share scaled to 26 decimal places.
    uint256 public assetsPerShare;

AssetPerShareAbstractVault inherits from AbstractVault, but AbstractVault is not upgrade-safe as it doesn’t define storage gaps. If we now want to upgrade AbstractVault.sol by e.g. adding an additional state variable, the additional variable can potentially overwrite any of the variables defined in AssetPerShareAbstractVault or any other child contracts.

For further explanation see here: https://docs.openzeppelin.com/upgrades-plugins/1.x/writing-upgradeable#storage-gaps

Note: Upgrading current deployed contracts to support storage gaps is not recommended as it might corrupt the storage layout. However, consider to define storage gaps for any new contracts being deployed.

As stated here, OpenZeppelin recommends storage gaps to avoid storage collisions between different versions of logic contracts. An alternative approach to avoid storage collisions is to utilise the unstructured pattern. The unstructured pattern is usually used to avoid storage collisions between proxy contract and logic contract, but can also be extended to avoid storage collisions between different versions of logic contracts. Thus, when a parent contract needs to be upgraded by adding an additional state variable, the new variable needs to be stored on a sufficiently random storage slot instead of using Solidity’s regular storage slots.

I-2

Call frequency of chargePerformanceFee() may change the charged fee for volatile assets

Topic
Incentive Design
Status
Acknowledged
Impact
Informational

chargePerformanceFee() in PerfFeeAbstractVault.sol can be freely called by the Vault Manager. There are no problems with the current vaults because they have yield-bearing tokens as the underlying asset, and assets per share should never decrease. After noticing the comments in PerfFeeAbstractVault.sol and having a discussion with the mStable team, we learned more about future plans for having vaults with riskier strategies. With these vaults, it is possible to see occasional decreases in assets per share. We want to inform the team and the readers that for such vaults, the frequency of the fee collection may dramatically change the fee charged, and it may make sense to introduce a cooldown to chargePerformanceFee().

I-3

withdraw() and redeem() may revert with OOG error

Topic
Gas Limit
Status
Acknowledged
Impact
Informational

_withdraw() and _redeem() in PeriodicAllocationAbstractVault.sol are gas-intensive functions, and they may revert with OOG (out-of-gas) errors in certain setups, blocking some users from withdrawing their assets. It is quite difficult to guess which underlying vaults will be used in the future, but since these underlying vaults will keep most of the assets in their own underlying vaults, they also need to withdraw from them to support bigger withdrawals. When a big enough withdrawal request is received in a setup with a large number of underlying vaults (currently limited to 15), and all these vaults keeping assets in their own underlying vaults, it will cause cascading balance calculation, withdrawal, and rebalance operations. This situation is something to be aware of when adding new underlying vaults into the system.

G-1

No check if performanceFee is 0

Topic
Gas Optimization
Status
Gas Savings
Low

In PerfFeeAbstractVault.sol, performanceFee can be set to 0 either via the _initialize or via the setPerformanceFee function. chargePerformanceFee doesn’t check for performanceFee == 0 so it goes through the function of _chargePerformanceFee, and only returns on line

if (feeShares > 0) {

Consider to only call _chargePerformanceFee when performanceFee > 0.

G-2

Unnecessary use of uint256 for performanceFee

Topic
Gas Optimization
Status
Gas Savings
Low

In PerfFeeAbstractVault.sol, the state variable performanceFee is meant to be less than FEE_SCALE (1e6) so a uint24 will be sufficient. Consider changing performanceFee to uint24.

G-3

Unnecessary control variable initialization of for loops

Topic
Gas Optimization
Status
Gas Savings
Low

In each for loop, the control variable (e.g. i) is initialized to 0. Since it is always initialized to 0 upon creation, this is redundant. Consider removing the assignment from various points in the contract to save gas.

Disclaimer

Macro makes no warranties, either express, implied, statutory, or otherwise, with respect to the services or deliverables provided in this report, and Macro specifically disclaims all implied warranties of merchantability, fitness for a particular purpose, noninfringement and those arising from a course of dealing, usage or trade with respect thereto, and all such warranties are hereby excluded to the fullest extent permitted by law.

Macro will not be liable for any lost profits, business, contracts, revenue, goodwill, production, anticipated savings, loss of data, or costs of procurement of substitute goods or services or for any claim or demand by any other party. In no event will Macro be liable for consequential, incidental, special, indirect, or exemplary damages arising out of this agreement or any work statement, however caused and (to the fullest extent permitted by law) under any theory of liability (including negligence), even if Macro has been advised of the possibility of such damages.

The scope of this report and review is limited to a review of only the code presented by the mStable team and only the source code Macro notes as being within the scope of Macro’s review within this report. This report does not include an audit of the deployment scripts used to deploy the Solidity contracts in the repository corresponding to this audit. Specifically, for the avoidance of doubt, this report does not constitute investment advice, is not intended to be relied upon as investment advice, is not an endorsement of this project or team, and it is not a guarantee as to the absolute security of the project. In this report you may through hypertext or other computer links, gain access to websites operated by persons other than Macro. Such hyperlinks are provided for your reference and convenience only, and are the exclusive responsibility of such websites’ owners. You agree that Macro is not responsible for the content or operation of such websites, and that Macro shall have no liability to your or any other person or entity for the use of third party websites. Macro assumes no responsibility for the use of third party software and shall have no liability whatsoever to any person or entity for the accuracy or completeness of any outcome generated by such software.