Skip to content

Latest commit

 

History

History
63 lines (50 loc) · 4.6 KB

078.md

File metadata and controls

63 lines (50 loc) · 4.6 KB

Orbiting Sangria Porpoise

Medium

Checks not implemented for lower & upper bounds of closeFactorMantissa

Description

NumaComptroller.sol specifies that:

@-->    // closeFactorMantissa must be strictly greater than this value
        uint internal constant closeFactorMinMantissa = 0.05e18; // 0.05

@-->    // closeFactorMantissa must not exceed this value
        uint internal constant closeFactorMaxMantissa = 0.9e18; // 0.9

However this check is never implemented inside _setCloseFactor:

    function _setCloseFactor(
        uint newCloseFactorMantissa
    ) external returns (uint) {
        // Check caller is admin
        require(msg.sender == admin, "only admin can set close factor");

        uint oldCloseFactorMantissa = closeFactorMantissa;
        closeFactorMantissa = newCloseFactorMantissa;
        emit NewCloseFactor(oldCloseFactorMantissa, closeFactorMantissa);

        return uint(Error.NO_ERROR);
    }

Weaponizing the Admin's Error

Someone could put forward an argument that this is just a cosmetic bound decided somewhat arbitrarily and even if the admin sets closeFactorMantissa to say 1e18 or 0.9999e18, there's no harm. In fact the current tests use closeFactorMantissa = 1e18!

However that's not true. The following flow shows how an attack path emerges in such a scenario:

  1. Bob has 15 ether of debt which has gone underwater. Let's assume admin has correctly set closeFactorMantissa = 0.9e18.
  2. Alice tries to maliciously liquidate Bob partially for amount of 14.99999 ether so that a dust amount of leftover debt remains. This is allowed by the logic here and here. As long as current debt value > minBorrowAmountAllowPartialLiquidation, partial liquidations are allowed. By doing so with many shortfall debts, she can flood the system with dust debts too unprofitable for anyone to liquidate, specially on a chain like Ethereum with high gas costs.
  3. Alice's attempt is thwarted by the checks related to maxClose and closeFactorMantissa. She can't repay more than 90% of the debt in the worst case:
  4. Attack not possible because closeFactorMantissa = 0.9e18. But if it were 1e18, she could have repaid 14.99999 ether and the attack works.
  5. With closeFactorMantissa = 0.9e18, the smallest leftover debt that can remain after an attacker's attempt is around 1 ether. This is because the protocol implements the minBorrowAmountAllowPartialLiquidation limit of 10 ether and partial repayment below this limit is not possible. So even if an attacker tries the attack when borrowBalance = 10 ether + 1, they will be able to repay only around 9 ether and not more.

Impact

Admin can inadvertently set it to values outside bounds and dilute/negate the protection mechanisms in place against avoidance of dust leftover debts. This issue is contingent on an admin error hence setting severity as medium.

Mitigation

    function _setCloseFactor(
        uint newCloseFactorMantissa
    ) external returns (uint) {
        // Check caller is admin
        require(msg.sender == admin, "only admin can set close factor");
+       require(newCloseFactorMantissa > closeFactorMinMantissa && newCloseFactorMantissa <= closeFactorMaxMantissa, "newCloseFactorMantissa outside limits");

        uint oldCloseFactorMantissa = closeFactorMantissa;
        closeFactorMantissa = newCloseFactorMantissa;
        emit NewCloseFactor(oldCloseFactorMantissa, closeFactorMantissa);

        return uint(Error.NO_ERROR);
    }