Mooniswap is a constant-product AMM created by 1Inch that uses virtual balances to reduce liquidity providers’ losses to arbitrageurs.
The 1inch team asked us to review the code for the 1inch Liquidity Protocol which is the upgraded and rebranded version of Mooniswap. We reviewed the code and here we publish our findings.
Overall, we are happy with the structure and implementation of the code and found it to be very efficient. Our main recommendation is to introduce thorough documentation and code comments to better communicate the expected behavior.
We audited commit b9c06335fb1d68b19054442547fc677f42795b44 of the 1inch-exchange/1inch-liquidity-protocol repo.
The following files were in scope:
The following files were out of scope:
The system behaves similarly to other popular AMMs, but with the exception that the prices used when swapping do not change instantaneously after a swap. Instead, after a swap, the prices used for swaps change gradually over a period of time (called the decayPeriod). The idea is that, after a large swap, this gradual price adjustment should result in arbitrage happening at a price closer to the real market price, and thus cause the liquidity providers to lose less money to arbitrageurs than similar AMMs that update the price instantaneously.
The decayPeriod, along with various fees, can be set by a governance module (within certain bounds). This governance module is controlled by 1Inch token holders. Voting happens in a continuous fashion, and the values adjust gradually towards the new values in much the same way as the AMM swap prices do after a swap.
In the constructor function of the MooniswapFactory contract, a poolOwner is set (this is immutable).
When a new Mooniswap pair contract is deployed via the MooniswapFactory.deploy function, the MooniswapDeployer contract sets the poolOwner as the owner of the new Mooniswap pair contract.
This owner of the Mooniswap pair contract has the ability to:
Mooniswap pair contract via the rescueFunds function.mooniswapFactoryGovernance value for the pair via the MooniswapGovernance.setMooniswapFactoryGovernance function — which in turn allows the owner to:feeCollector addresses.Mooniswap contract via the shutdown function, which disallows swaps.So it is important that the poolOwner value that is set in the constructor function of the MooniswapFactory contract be fully trusted.
Here we present our findings.
None. 🙂
None. 🙂
The MooniswapFactoryGovernance contract has a shutdown function that can be used to pause the contract and prevent any future swaps. However there is no function to unpause the contract. There is also no way for the factory contract to redeploy a Mooniswap instance for a given pair of tokens. Therefore, if a Mooniswap contract is ever shutdown/paused, it will not be possible for that pair of tokens to ever be traded on the Mooniswap platform again, unless a new factory contract is deployed.
Consider providing a way for Mooniswap contracts to be unpaused.
The MooniswapFactoryGovernance contract has an isFeeCollector mapping. Fee collectors can be added by the owner, but there is no way for the owner to remove fee collectors.
Consider providing a way for te owner to remove fee collectors.
Some functions (for example, the Mooniswap.depositFor function) use named return values and implicitly return those values. Consider removing all named return variables and explicitly returning all return values using a return statement. This should improve both explicitness and readability of the code, and will make the code less susceptible to regressions during future updates.
The MooniswapFactory.deploy function enforces that the tokens in the pair are properly ordered before deploying a new Mooniswap contract. That is, it enforces that token1 < token2.
However, the Mooniswap pair contract itself does not enforce the correct ordering in its constructor function. If the order is not correct, then the Mooniswap._getReturn function will not behave correctly.
Since the correctness of the ordering is crucial for the Mooniswap contract to operate correctly, consider having the Mooniswap contract’s constructor function enforce the correct token ordering. That way the correctness of the Mooniswap contract will be independent of the factory contract that deploys it.
The MooniswapFactory contract identifies the Mooniswap pair’s tokens as token1 and token2, whereas the Mooniswap contract itself identifies its tokens as token0 and token1. Consider making these token identifiers consistent.
The contracts and functions in the reviewed code base lack documentation. This hinders reviewers’ understanding of the code’s intention, which is fundamental to correctly assess not only security, but also correctness. Additionally, docstrings improve readability and ease maintenance. They should explicitly explain the purpose or intention of the functions, the scenarios under which they can fail, the roles allowed to call them, the values returned and the events emitted.
Consider thoroughly documenting all functions (and their parameters) that are part of the contracts’ public API. Functions implementing sensitive functionality, even if not public, should be clearly documented as well. When writing docstrings, consider following the Ethereum Natural Specification Format (NatSpec).
No critical or high severity issues were found. Some changes were proposed to follow best practices, and reduce the potential attack surface.