Mochi contest - cmichel's results

Handle

cmichel

Vulnerability details

The MochiProfileV0 defines liquidation and collateral factors for different asset types. For the AssetClass.Sigma type, the liquidation factor is less than the collateral factor:

function liquidationFactor(address _asset)
    public
    view
    override
    returns (float memory)
{
    AssetClass class = assetClass(_asset);
    if (class == AssetClass.Sigma) { // } else if (class == AssetClass.Sigma) {
        return float({numerator: 40, denominator: 100});
    }
}

function maxCollateralFactor(address _asset)
    public
    view
    override
    returns (float memory)
{
    AssetClass class = assetClass(_asset);
    if (class == AssetClass.Sigma) {
        return float({numerator: 45, denominator: 100});
    }
}

This means that one can take a loan of up to 45% of their collateral but then immediately gets liquidated as the liquidation factor is only 40%. There should always be a buffer between these such that taking the max loan does not immediately lead to liquidations:

A safety buffer is maintained between max CF and LF to protect users against liquidations due to normal volatility. Docs

Recommended Mitigation Steps

The max collateral factor for the Sigma type should be higher than its liquidation factor.

Handle

cmichel

Vulnerability details

The contracts are missing slippage checks which can lead to being vulnerable to sandwich attacks.

A common attack in DeFi is the sandwich attack. Upon observing a trade of asset X for asset Y, an attacker frontruns the victim trade by also buying asset Y, lets the victim execute the trade, and then backruns (executes after) the victim by trading back the amount gained in the first trade. Intuitively, one uses the knowledge that someone’s going to buy an asset, and that this trade will increase its price, to make a profit. The attacker’s plan is to buy this asset cheap, let the victim buy at an increased price, and then sell the received amount again at a higher price afterwards.

See FeePoolV0._buyMochi:

uniswapRouter.swapExactTokensForTokens(
    mochiShare,
    1, // @audit min return set to zero
    path,
    address(this),
    type(uint256).max
);

Also ReferralFeePoolV0.claimRewardAsMochi:

uniswapRouter.swapExactTokensForTokens(
    reward[msg.sender],
    1, // @audit min return set to zero
    path,
    address(this),
    type(uint256).max
);

Same with MochiTreasuryV0._buyCRV.

Impact

Trades can happen at a bad price and lead to receiving fewer tokens than at a fair market price. The attacker's profit is the protocol's loss.

Recommended Mitigation Steps

Add minimum return amount checks. Accept a function parameter that can be chosen by the transaction sender, then check that the actually received amount is above this parameter.

Alternatively, check if it's feasible to send these transactions directly to a miner (flashbots) such that they are not visible in the public mempool.

Handle

cmichel

Vulnerability details

The ERC20.transfer() and ERC20.transferFrom() functions return a boolean value indicating success. This parameter should checked for success.

See VestedRewardPool.claim which performs ERC20 transfers without checking for the return value.

Impact

As the trusted mochi token is used which supposedly reverts on failed transfers, not checking the return value does not lead to any security issues. We still recommend checking it to abide by the EIP20 standard.

Recommended Mitigation Steps

Consider using require(mochi.transfer(msg.sender, vesting[msg.sender].claimable), "transfer failed") instead.

Handle

cmichel

Vulnerability details

There is no check in ChainlinkAdapterEth.getPrice if the return values indicate stale data. This could lead to stale prices according to the Chainlink documentation:

• under current notifications: "if answeredInRound < roundId could indicate stale data."
• under historical price data: "A timestamp with zero value means the round is not complete and should not be used."

Impact

Stale prices that do not reflect the current market price anymore could be used which would influence the liquidations.

Recommendation

Add the recommended checks:

(
    uint80 roundID,
    int256 price,
    ,
    uint256 timeStamp,
    uint80 answeredInRound
) = feed[_asset].latestRoundData();
require(
    timeStamp != 0,
    “ChainlinkOracle::getLatestAnswer: round is not complete”
);
require(
    answeredInRound >= roundID,
    “ChainlinkOracle::getLatestAnswer: stale data”
);
require(price != 0, "Chainlink Malfunction”);

Handle

cmichel

Vulnerability details

The total debt in MochiVault.accrueDebt increases by the current debt times the debt index growth. This is correct but the total debt is then reduced again by the calling user's discounted debt, meaning, the total debt depends on which specific user performs the debt accrual.

This should not be the case.

POC

Assume we have a total debt of 2000, two users A and B, where A has a debt of 1000, and B has a debt of 100. The (previous) debtIndex = 1.0 and accruing it now would increase it to 1.1.

There's a difference if user A or B first does the accrual.

User A accrues first

User A calls accrueDebt: increased = 2000 * 1.1/1.0 - 2000 = 200. Thus debts is first set to 2200. The user's increasedDebt = 1000 * 1.1 / 1.0 - 1000 = 100 and assume a discount of 10%, thus discountedDebt = 100 * 10% = 10. Then debts = 2200 - 10 = 2190.

The next accrual will work with a total debt of 2190.

User B accruess first

User B calls accrueDebt: increased = 2000 * 1.1/1.0 - 2000 = 200. Thus debts is first set to 2200. The user's increasedDebt = 100 * 1.1 / 1.0 - 100 = 10 and assume a discount of 10%, thus discountedDebt = 10 * 10% = 1. Then debts = 2200 - 1 = 2199.

The next accrual will work with a total debt of 2199, leading to more debt overall.

Impact

The total debt of a system depends on who performs the accruals which should ideally not be the case. The discrepancy compounds and can grow quite large if a whale always does the accrual compared to someone with almost no debt or no discount.

Recommended Mitigation Steps

Don't use the discounts or track the weighted average discount across all users that is subtracted from the increased total debt each time, i.e., reduce it by the discount of all users (instead of current caller only) when accruing to correctly track the debt.

Handle

cmichel

Vulnerability details

Governance can change the engine.nft address which is used by vaults to represent collateralized debt positions (CDP). When minting a vault using MochiVault.mint the address returned ID will be used and overwrite the state of an existing debt position and set its status to Idle.

Impact

Changing the NFT address will allow overwriting existing CDPs.

Recommended Mitigation Steps

Disallow setting a new NFT address. or ensure that the new NFT's IDs start at the old NFT's IDs.

Handle

cmichel

Vulnerability details

The UniswapV2LPAdapter/SushiswapV2LPAdapter.update function retrieves the underlying from the LP token pair (_asset) but then calls router.update(_asset, _proof) which is the LP token itself again. This will end up with the router calling this function again recursively.

Impact

This function fails as there's an infinite recursion and eventually runs out of gas.

Recommendation

The idea was most likely to update the underlying price which is used in _getPrice as uint256 eAvg = cssr.getExchangeRatio(_underlying, weth);.

Call router.update(underlying, _proof) instead. Note that the _proof does not necessarily update the underlying <> WETH pair, it could be any underlying <> keyAsset pair.

Handle

cmichel

Vulnerability details

The UniswapV2TokenAdapter.supports function calls its aboveLiquidity function which returns the UniswapV2 liquidity if the pair exists. If this is below minimumLiquidity, the supports function will return false.

However, it could be that the Sushiswap pair has lots of liquidity and could be used.

try uniswapCSSR.getLiquidity(_asset, _pairedWith) returns (
            uint256 liq
        ) {
    float memory price = cssrRouter.getPrice(_pairedWith);
    // @audit this returns early. if it's false it should check sushiswap first
    return convertToValue(liq, price) >= minimumLiquidity;
} catch {
    try sushiCSSR.getLiquidity(_asset, _pairedWith) returns (
        uint256 liq
    ) {
        float memory price = cssrRouter.getPrice(_pairedWith);
        return convertToValue(liq, price) >= minimumLiquidity;
    } catch {
        return false;
    }
}

Impact

Suppose the UniswapV2TokenAdapter wants to be used as an adapter for a Sushiswap pool. An attacker creates a UniswapV2 pool for the same pair and does not provide liquidity. The Router.setPriceSource calls UniswapV2TokenAdapter.supports and returns false as the Uniswap liquidity is too low, without checking the Sushiswap liquidity.

Recommendation

In aboveLiquidity, if the UniswapV2 liquidity is less than the minimum liquidity, instead of returning, compare the Sushiswap liquidity against this threshold.