Divisions lose precision

Description

In getExpectedAmount method, oracleAmount is scaled from collateral's to fund's asset decimals. Similar scaling is done for minRatioAmount - the _minRatio value should be provided in funds asset's decimals precision, according to tests.

function getExpectedAmount(uint256 swapAmount_) external view override returns (uint256 returnAmount_) {
    address collateralAsset = IMapleLoanLike(_loan).collateralAsset();
    address fundsAsset      = IMapleLoanLike(_loan).fundsAsset();

    uint256 oracleAmount =
        swapAmount_
            * IMapleGlobalsLike(_getGlobals()).getLatestPrice(collateralAsset)  // Convert from `fromAsset` value.
            * 10 ** IERC20Like(fundsAsset).decimals()                           // Convert to `toAsset` decimal precision.
            * (10_000 - _allowedSlippage)                                       // Multiply by allowed slippage basis points
            / IMapleGlobalsLike(_getGlobals()).getLatestPrice(fundsAsset)       // Convert to `toAsset` value.
            / 10 ** IERC20Like(collateralAsset).decimals()                      // Convert from `fromAsset` decimal precision.
            / 10_000;                                                           // Divide basis points for slippage

    uint256 minRatioAmount = swapAmount_ * _minRatio / 10 ** IERC20Like(collateralAsset).decimals();

    return oracleAmount > minRatioAmount ? oracleAmount : minRatioAmount;
}

If the IERC20Like(collateralAsset).decimals() is larger than IERC20Like(fundsAsset).decimals() and swapAmount_ is small enough, the numerators will be smaller than denominators in both equations, and the value returned from the getExpectedAmount will be 0.

For an example, consider a case with USDC as funds asset and YAMv2 as collateral:

function test() external view returns (uint256) {
    uint256 swapAmount_ = 7 * 10 ** 16;
    uint8 fundsAssetDecimals = 6;  // e.g. USDC
    uint8 collateralAssetDecimals = 24;  // e.g. YAMv2
    uint256 _minRatio = 13 * 10 ** fundsAssetDecimals;  // ~13$

    uint256 collateralAssetPrice = 13 * 10**8;
    uint256 fundsAssetPrice = 1 * 10**8;
    uint256 _allowedSlippage = 0;

    uint256 oracleAmount =
       swapAmount_
           * collateralAssetPrice                                              // Convert from `fromAsset` value.
           * 10 ** fundsAssetDecimals                                         // Convert to `toAsset` decimal precision.
           * (10_000 - _allowedSlippage)                                       // Multiply by allowed slippage basis points
           / fundsAssetPrice                                                   // Convert to `toAsset` value.
           / 10 ** collateralAssetDecimals                                       // Convert from `fromAsset` decimal precision.
           / 10_000;                                                           // Divide basis points for slippage

    uint256 minRatioAmount = swapAmount_ * _minRatio / 10 ** collateralAssetDecimals;

    return oracleAmount > minRatioAmount ? oracleAmount : minRatioAmount;  // will be zero  
}

This bug may allow an attacker to drain Liquidator from collateral for "free" - especially if the additional gas cost is lower than the expected funds assets value.

Exploit Scenario

A loan with USDC as the funds asset and some token with 18 decimals as collateral asset is defaulted. An attacker repeatedly calls liquidatePortion method (e.g. using reentrancy from the msg.sender.call invocation) from the Liquidator contract with small swapAmount values. He repeatedly receives small amounts of collateral token, and don't have to send back any funding tokens, because getExpectedAmount method keeps returning zero - due to the arithmetic precision bug.

Recommendations

In liquidatePortion method, reject calls with small swapAmounts, or check if values returned from getExpectedAmount don't equal to zero. Make sure that small amounts of collateral won't be locked in the contract.

Alternatively, in getExpectedAmount check if value returned will be 0, and return 1 instead - forcing users to overpay for small liquidations.

We just spent some time modeling this and found that in order for there to be a zero value returned, there has to be an expectedAmount of less than 0.000001 USDC. Now that we have added a reentrancy guard in the liquidatePortion function, we believe it should be fine to leave the function as is.

During our discussion though we did discover something else that we would like to discuss with your team. Right now we use a custom oracle that wraps Chainlink (e.g. https://etherscan.io/address/0xF808ec05c1760DE4794813d08d2Bf1E16e7ECD0B#code)

It can be seen here that there is a manual override function that was put in place in case of an oracle outage: https://github.com/maple-labs/maple-core/blob/4577df4ac7e9ffd6a23fe6550c1d6ef98c5185ea/contracts/oracles/ChainlinkOracle.sol#L83

This was to protect against an oracle outage in the LoansV1 code, where we used the price to determine collateral amounts to post to meet a given collateral ratio and to determine the expected amount to be returned from the atomic UniswapV2 liquidation.

Now we are concerned that this actually opens up a new attack vector, where an attacker could "wrench attack" the owners of the security multisig keys and set the oracle price to zero manually in the oracle wrapper and drain all of the funds.

Two questions:

Should we add a pause function in the liquidator?
Should we no longer use the chainlink oracle wrappers and instead do a zero price check on the normal chainlink oracles in the getExpectedAmount function?

maple-labs / trail-of-bits-audit