This security assessment was prepared by OpenZeppelin.
We audited the mountainprotocol/tokens repository at the d548aa6f037d9c7ed653a8004f83949b70133c7a commit.
The fixes to our findings were finalised in the same repository at the 2f0e2f08362fc44360357627b390d21b42031265 commit.
In scope were the following contracts:
contracts
└── USDM.sol
USDM is a stablecoin, intended to be valued at 1 USD, backed by short-term US Treasury bills. The Mountain Protocol team will retain a small fraction of the corresponding yield, with the rest distributed to USDM token holders through rebasing. In this way, the price of USDM should remain stable while token holders see their balance of USDM increase daily.
The Mountain Protocol hosts the primary market that establishes the price peg. In particular, registered customers can deposit collateral (either USDC tokens or fiat currency) in exchange for USDM, redeem USDM for collateral, or withdraw USDM to any Ethereum address. Since they are offered an exchange rate of one dollar per USDM in both directions, any price movement in the external (secondary) market can be arbitraged in the primary market to re-establish the peg.
The secondary market includes any external system that deals with USDM. The Mountain Protocol intends to establish a USDC-USDM Curve Pool on Ethereum.
The Mountain Protocol team intends to rebase USDM daily to account for the investment yield. This means that during normal operation, the USDM balance of all token holders should steadily increase. To avoid potential loss of funds, users should ensure that any smart contracts that receive USDM tokens are capable of handling changing balances.
The primary market is run by the Mountain Protocol team, and the secondary market also depends on its operation. Therefore, users must implicitly trust the team to maintain the availability of the market, invest the collateral safely and rebase USDM in line with the yields.
Users also implicitly assume that short-term US Treasury bills continue functioning as a safe investment.
Lastly, there are several privileged roles in the system, all managed by the Mountain Protocol team:
The blocklist feature is meant to prevent some users from transferring USDM tokens. The list of blocked addresses is managed by any address with the BLOCKLIST_ROLE role.
All token transfers only ensure that the source of the tokens is not blocked. This has several implications:
mint and transfer/transferFrom functions, but they cannot transfer them out.transferFrom the blocked address.Consider whether this is the expected behavior in all four functionalities (mint, transfer, burn and approvals) and whether the code should be updated accordingly. Moreover, consider explicitly documenting the behavior in all functions' docstrings.
Update: Resolved in commit 92584df. The Mountain Protocol confirmed that this is the desired behavior and expanded the smart contract documentation accordingly. They also stated:
In an effort to avoid increased gas fees on all users, a verification of destination blocked address has purposefully not been set in place. Users are expected to know parties they are transacting with and avoiding blocked addresses.
Rebases occur by discontinuously increasing all user balances. This implies that a USDM token purchased immediately before the rebase transaction is worth more than one purchased immediately afterwards. In principle, this means that the price of USDM tokens should follow a sawtooth wave.
In practice, the effect size will be minimal because the daily interest rate is expected to be less than 0.02%. Nevertheless, we believe it is worth considering because:
Consider whether it is worth the extra complexity to smooth the discontinuity. This would involve distributing the daily yield over time rather than in one step.
Update: Acknowledged, not resolved. The Mountain Protocol team stated:
Given the minimal effect size, smoothing the rebases is not worth the extra complexity. However, we will describe the edge case in our risk documentation.
There are some places in the codebase where comments are either inaccurate or misleading.
addRewardMultiplier has a comment that says that the input parameter _rewardMultiplier is the new rewardMultiplier value, but this is not true since it is actually an increment on top of it.setRewardMultiplier comment says it can be called by ADMIN_ROLE but it is actually DEFAULT_ADMIN_ROLE._mint function says "Internal function" but it should be "Private function" instead.To improve correctness and readability, consider fixing these examples and reviewing the codebase in case other comments can benefit from rephrasing.
Update: Resolved in commit fdaf392.
To accommodate rebasing, the user-facing functions describe token amounts while the contract's own logic uses the equivalent amount of shares. Both conversion functions (convertToShares and convertToAmount) round down, which could lead to minor discrepancies.
For example, 0.1e18 USDM tokens at a rewardMultiplier of 1.2e18 will convert to 83333333333333333 shares, which will convert back to 99999999999999999 USDM tokens.
In the interest of safety, all rounding errors should be in favor of the protocol, and the contract correctly implements this heuristic. Nevertheless, the consequence is that all token transfers (including mint and burn operations) may transfer slightly less than expected. If a user attempted to transfer their entire balance, they could retain some shares in their wallet, corresponding to a fraction of a token.
Consider documenting this behavior in the docstrings so that users can be aware of it. In addition, consider whether a transferShares function (or possible transferAll function) should be included so that knowledgeable users can have finer control.
Update: Resolved in commit 4606704.
The _setRewardMultiplier function of the USDM contract ensures the parameter is not less than 1e18. This correctly represents 100%, since it is the same numeric value as the multiplier precision constant. Nevertheless, in the interest of predictability and local reasoning, consider using the existing constant in the validation.
Update: Resolved in commit 2e9b94c.
The USDM contract implements ERC-20 metadata functions but does not inherit the corresponding interface. Consider explicitly inheriting relevant interfaces to ensure consistency and to document the intended behavior.
Update: Resolved in commit a532c49.
There are some names used throughout the codebase that might benefit from a change:
convertToAmount function should be convertToToken. Both conversion functions use the term "amount" to describe both shares and tokens and it might be confusing._rewardMultiplier input parameter of the addRewardMultiplier function should be something like _rewardMultiplierIncrement instead._blocklist mapping has an accurate name since it represents a list of blocked accounts, the blocklistAccounts and unblocklistAccounts functions might be just blockAccounts and unblockAccounts, since blocklist is not a verb. The same applies to the AccountBlocklisted and AccountUnblocklisted events.Consider adopting the suggested naming conventions to improve the readability and clarity of the codebase.
Update: Resolved in commit 500a2c9.
The USDM contract reimplements a lot of functionality that is already available in the OpenZeppelin ERC20PermitUpgradeable contract with minor modifications to account for the rebasing logic. However, it would be simpler to inherit from the contract and introduce the modifications by overriding the relevant functions. Specifically, our suggestion is:
_shares mapping and _totalShares variable respectively. Since these are private values, the original names never need to be shown to the user._mint, _burn and _transfer functions to simply override and invoke the OpenZeppelin version, first converting the amount parameter to the equivalent number of shares.transfer function and bottom third of the contract.In this way, the USDM contract would only focus on the new functionality that it introduces.
Update: Acknowledged, not resolved. The Mountain Protocol team stated:
In the interest of code clarity and avoiding confusion, we do not want to record shares in the
_balancesmapping.
The USDM contract contains a storage gap of 50 storage slots to account for potential future changes to the storage layout. However, the convention (as noted in the referenced document) is to choose the size so the contract's variables, including the __gap, use a combined 50 storage slots. In this way, future versions of the contract will have a computable gap without reference to the original version.
Consider changing the size of the variable to correctly follow the convention.
Update: Resolved in commit de984d1.
The USDM contract uses the _setupRole function, but this function has been deprecated. Consider using _grantRole instead.
Update: Resolved in commit 45a85a3.
A medium-severity and other lower-severity issues have been found. Several fixes were suggested to improve the readability (or clarity) of the codebase and facilitate future audits, integrations and development. The Mountain Protocol team was very responsive and provided extensive documentation about the project.
Below we provide our recommendations for monitoring important activities in order to detect and prevent potential bugs.
While audits help in identifying code-level issues in the current implementation and potentially the code deployed in production, the Mountain Protocol team is encouraged to consider incorporating monitoring activities in the production environment. Ongoing monitoring of deployed contracts helps identify potential threats and issues affecting production environments. With the goal of providing a complete security assessment, the monitoring recommendations section raises several actions addressing trust assumptions and out-of-scope components that can benefit from on-chain monitoring.
It is recommended to monitor all sensitive actions that affect the token contract, including:
mint and burn functions to ensure they are expected, match the daily net deposit/withdrawal amounts, and are within a reasonable range.blocklistAccounts and unblocklistAccounts functions to ensure they are expected.addRewardMultiplier and setRewardMultiplier functions to ensure they are expected and within a reasonable range.It is also recommended to monitor the secondary markets, including the health of the Curve USDC-USDM pool, to ensure: