Mochi contest - jonah1005's results

Handle

jonah1005

Vulnerability details

Impact

It's similar to the issue "misuse amount as increasing debt in the vault contract". Similar issue in a different place that leads to different exploit patterns and severity.

When users borrow usdm from a vault, the debt increased by the amount * 1.005.

    uint256 increasingDebt = (_amount * 1005) / 1000;

However, when the contract records the total debt it users _amount instead of increasingDebt.

        details[_id].debtIndex =
            (details[_id].debtIndex * (totalDebt)) /
            (details[_id].debt + _amount);
        details[_id].debt = totalDebt;
        details[_id].status = Status.Active;
        debts += _amount;

MochiVault.sol#L242-L249

The contract's debt is inconsistent with the total sum of all users' debt. The bias increases overtime and would break the vault at the end.

For simplicity, we assume there's only one user in the vault. Example:

1. User deposit 1.2 M worth of BTC and borrow 1M USDM.
2. The user's debt (details[_id].debt) would be 1.005 M as there's a .5 percent fee.
3. The contract's debt is 1M.
4. BTC price decrease by 20 percent
5. The liquidator tries to liquidate the position.
6. The liquidator repays 1.005 M and the contract tries to sub the debt by 1.005 M
7. The transaction is reverted as details[_id].debt -= _usdm; would raise exception.

inaccuracy accounting would lead to serious issues. I consider this is a high-risk issue.

Proof of Concept

This is a web3.py script that a liquidation may fail.

deposit_amount = 10**18
big_deposit = deposit_amount * 100000
minter.functions.mint(user, big_deposit).transact()

dai.functions.approve(vault.address, big_deposit + deposit_amount).transact()

# create two positions
vault.functions.mint(user, zero_address).transact()
vault.functions.mint(user, zero_address).transact()

# # borrow max amount
vault.functions.increase(0, big_deposit, big_deposit, zero_address, '').transact()
vault.functions.increase(1, deposit_amount, deposit_amount, zero_address, '').transact()

vault_debt = vault.functions.debts().call()

# ## This would clear out all debt in vault.
repay_amount = vault_debt + 10**18
usdm.functions.approve(vault.address, repay_amount).transact()

vault.functions.repay(0, repay_amount).transact()

print('debt left:', vault.functions.debts().call())
# ## All the positions would not be liquidated from now on

dai_price = cssr_factory.functions.getPrice(dai.address).call()
cssr_factory.functions.setPrice(dai.address, dai_price[0] // 10).transact() 

## this would revert
liquidator.functions.triggerLiquidation(dai.address, 1).transact()

Tools Used

None

Recommended Mitigation Steps

I believe this is a mistake. Recommend to check the contract to make sure increasingDebt is used consistently.

Handle

jonah1005

Vulnerability details

Impact

The permissionless function registerAsset only checks current liquidity. There's no sanity check whether the asset has registered as another asset class. An attacker can set an asset into AssetClass.Sigma.

Unexpected liquidation would happen if an Alpha asset is set to Sigma asset. I consider this is a high-risk issue

Proof of Concept

MochiProfileV0.sol#L58-L62

A possible exploit pattern would be

1. register DAI vault as AssetClass.sigma set the collateralization ratio from 90 to 40
2. Liquidates the debt

Tools Used

None

Recommended Mitigation Steps

Do a sanity check whether it's an existed asset.

Handle

jonah1005

Vulnerability details

treasury is vulnerable to sandwich attack.

Impact

There's a permissionless function veCRVlock in MochiTreasury. Since everyone can trigger this function, the attacker can launch a sandwich attack with flashloan to steal the funds. MochiTreasuryV0.sol#L73-L94

Attackers can possibly steal all the funds in the treasury. I consider this is a high-risk issue.

Proof of Concept

MochiTreasuryV0.sol#L73-L94

Here's an exploit pattern

1. Flashloan and buy CRV the uniswap pool
2. Trigger veCRVlock()
3. The treasury buys CRV at a very high price.
4. Sell CRV and pay back the loan.

Tools Used

None

Recommended Mitigation Steps

Recommend to add onlyOwner modifier.

Handle

jonah1005

Vulnerability details

Impact

MochiEngine allows the operator to change the NFT contract. MochiEngine.sol#L91-L93 All the vaults would point to a different NFT address. As a result, users would not be access their positions. The entire protocol would be broken.

IMHO, A function that would break the entire protocol shouldn't exist.

I consider this is a high-risk issue.

Proof of Concept

MochiEngine.sol#L91-L93

Tools Used

None

Recommended Mitigation Steps

Remove the function.

Handle

jonah1005

Vulnerability details

Impact

There's a permissionless function distributeMochi in FeePoolV0. Since everyone can trigger this function, an attacker can launch a sandwich attack with flashloan to steal the funds. FeePoolV0.sol#L55-L62 The devs have mentioned this concern in the comment. An attacker can steal the funds with a flash loan attack.

Attackers can steal all the funds in the pool. I consider this is a high-risk issue.

Proof of Concept

FeePoolV0.sol#L55-L62

Please refer to yDai Incident to check the severity of a harvest function without slippage control.

Please refer to Mushrooms-finance-theft to check how likely this kind of attack might happen.

Tools Used

None

Recommended Mitigation Steps

If the dev wants to make this a permissionless control, the contract should calculate a min return based on TWAP and check the slippage.

Handle

jonah1005

Vulnerability details

regerralFeePool is vulnerable to MEV searcher

Impact

claimRewardAsMochi in the ReferralFeePoolV0 ignores slippage. This is not a desirable design. There are a lot of MEV searchers in the current network. Swapping assets with no slippage control would get rekted. Please refer to https://github.com/flashbots/pm.

Given the current state of the Ethereum network. Users would likely be sandwiched. I consider this is a high-risk issue.

Proof of Concept

ReferralFeePoolV0.sol#L28-L48

Please refer to https://medium.com/immunefi/mushrooms-finance-theft-of-yield-bug-fix-postmortem-16bd6961388f to see a possible attack pattern.

Tools Used

None

Recommended Mitigation Steps

I recommend adding minReceivedAmount as a parameter.

function claimRewardAsMochi(uint256 _minReceivedAmount) external {
    // original logic here
    require(engine.mochi().balanceOf(address(this)) > _minReceivedAmount, "!min");
    engine.mochi().transfer(
        msg.sender,
        engine.mochi().balanceOf(address(this))
    );
}

Also, the front-end should calculate the min amount with the current price.

Handle

jonah1005

Vulnerability details

One auction at a time would lead to bad debt

Impact

The current DutchAuctionLiquidator only allows one auction for a position at a time. This is not a desirable design. If a position is liquidated due to the price movement of collaterals, the position would likely be liquidated soon.

The protocol may end up with a lot of bad debt if the market went wild. I consider this is a high-risk issue.

Proof of Concept

Please refer to the graph to check the liquidation statistics. th_graph_compound the_graph_cream the_graph_aave

Tools Used

None

Recommended Mitigation Steps

Using dutch auction here is a kind of novelty. I could tell the devs accommodate this idea with many designs to make the protocol safe. However, a new idea is worth more thoughtfulness. The quick fix I could come up with is to allow multiple auctions at a time and allow users to decide the amount to liquidate. However, I feel like this fix isn't safe enough. Recommend the team to check the design and tests it with some simulation based on real market data.

Handle

jonah1005

Vulnerability details

Impact

MochiVaultFactory.sol#L26-L37 There's no permission control in the vaultFactory. Anyone can create a vault. The transaction would be reverted when the government tries to deploy such an asset.

As the protocol checks whether the vault is a valid vault by comparing the contract's address with the computed address, the protocol would recognize the random vault as a valid one.

I consider this is a medium-risk issue.

Proof of Concept

Here's a web3.py script to trigger the bug.

vault_factory.functions.deployVault(usdt.address).transact()
## this tx would be reverted
profile.functions.registerAssetByGov([usdt.address], [3]).transact()

Tools Used

None

Recommended Mitigation Steps

Recommend to add a check.

require(msg.sender == engine, "!engine");