Security Audit
January 11th, 2023
Version 1.0.0
Presented by 0xMacro
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.
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.
Our understanding of the specification was based on the following sources:
The following source code was reviewed during the audit:
1657b7ed2f8b964783487a0d68f973c70036bdfb
Specifically, we audited the following contracts within this repository:
Source Code | SHA256 |
---|---|
contracts/vault/AbstractVault.sol |
|
contracts/vault/allocate/PeriodicAllocationAbstractVault.sol |
|
contracts/vault/allocate/AssetPerShareAbstractVault.sol |
|
contracts/vault/allocate/SameAssetUnderlyingsAbstractVault.sol |
|
contracts/vault/meta/PeriodicAllocationPerfFeeMetaVault.sol |
|
contracts/vault/fee/PerfFeeAbstractVault.sol |
|
contracts/vault/fee/FeeAdminAbstractVault.sol |
|
contracts/tokens/InitializableToken.sol |
|
contracts/tokens/InitializableTokenDetails.sol |
|
contracts/shared/VaultManagerRole.sol |
|
contracts/shared/ImmutableModule.sol |
|
contracts/shared/ModuleKeys.sol |
|
contracts/shared/SingleSlotMapper.sol |
|
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.
Click on an issue to jump to it, or scroll down to see them all.
removeVault()
will block the governor from removing underlying vaults
assetsPerShare
when underlying vault is paused
removeVault()
may block single source withdraws
chargePerformanceFee()
may change the charged fee for volatile assets
withdraw()
and redeem()
may revert with OOG error
performanceFee
is 0
We quantify issues in three parts:
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. |
removeVault()
will block the governor from removing underlying vaults
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
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
assetsPerShare
when underlying vault is paused
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.
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)
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
_asset.safeApprove(underlyingVault, 0)
A user is able to deposit assets for shares and redeem them subsequently by calling deposit
and redeem
, respectively. The redeem
function:
_previewRedeem
which uses the current assetsPerShare
assetsPerShare
using _checkAndUpdateAssetPerShare
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
_checkAndUpdateAssetPerShare(assets)
to before _previewRedeem(shares)
in _redeem
_checkAndUpdateAssetPerShare(assets)
to before _previewMint
in _mint
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
setPerformanceFee
in the _initialize
function require(_performanceFee <= FEE_SCALE, "Invalid fee");
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:
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.
removeVault()
may block single source withdraws
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
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
address(0)
check (similar to to the functionsetVaultManager
) for code consistency. However, if there is an emphasis on code size, this is not recommended.The following imports in PerfFeeAbstractVault.sol are not required and can be removed:
import { AbstractVault } from "../AbstractVault.sol"
import { VaultManagerRole } from "../../shared/VaultManagerRole.sol";
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
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.
chargePerformanceFee()
may change the charged fee for volatile assets
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()
.
withdraw()
and redeem()
may revert with OOG error
_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.
performanceFee
is 0
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.
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
.
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.
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.