Fraxlend (Frax Finance) contest - cccz's results

Lines of code

https://github.com/code-423n4/2022-08-frax/blob/c4189a3a98b38c8c962c5ea72f1a322fbc2ae45f/src/contracts/FraxlendPairCore.sol#L929-L933

Vulnerability details

Impact

In the liquidateClean function of the FraxlendPairCore contract, when the debt is fully liquidated, the liquidator receives more collateral than when it is partially liquidated, due to the fact that cleanLiquidationFee is larger than dirtyLiquidationFee. In the liquidate function used for partial liquidation, cleanLiquidationFee is used to calculate the collateral obtained by the liquidator, which means that the contract discourages the user from calling the liquidateClean function for full liquidation, since the user receives the same percentage of liquidation fees for partial and full liquidation. Since the liquidator has no incentive to perform a full liquidation, this may increase the bad debt in the contract

Proof of Concept

https://github.com/code-423n4/2022-08-frax/blob/c4189a3a98b38c8c962c5ea72f1a322fbc2ae45f/src/contracts/FraxlendPairCore.sol#L929-L933 https://github.com/code-423n4/2022-08-frax/blob/c4189a3a98b38c8c962c5ea72f1a322fbc2ae45f/src/contracts/FraxlendPairCore.sol#L193-L194

Tools Used

None

Recommended Mitigation Steps

Use dirtyLiquidationFee in the liquidate function to calculate the liquidation fee

    function liquidate(uint256 _shares, address _borrower)
        external
        whenNotPaused
        nonReentrant
        approvedLender(msg.sender)
        returns (uint256 _collateralForLiquidator)
    {
        _addInterest();

        // Get best available exchange rate
        uint256 _exchangeRate = _updateExchangeRate();
        if (_isSolvent(_borrower, _exchangeRate)) {
            revert BorrowerSolvent();
        }

        VaultAccount memory _totalBorrow = totalBorrow;

        // Determine how much of the borrow and collateral will be repaid
        _collateralForLiquidator =
            (((_totalBorrow.toAmount(_shares, false) * _exchangeRate) / EXCHANGE_PRECISION) *
-                (LIQ_PRECISION + cleanLiquidationFee)) /
+                (LIQ_PRECISION + dirtyLiquidationFee)) /
            LIQ_PRECISION;

Lines of code

https://github.com/code-423n4/2022-08-frax/blob/c4189a3a98b38c8c962c5ea72f1a322fbc2ae45f/src/contracts/FraxlendPairCore.sol#L194-L195

Vulnerability details

Impact

According to the comments, dirtyLiquidationFee should be 90% of cleanLiquidationFee, since LIQ_PRECISION = 1e5, dirtyLiquidationFee here is actually 9% of cleanLiquidationFee.

            cleanLiquidationFee = _liquidationFee;
            dirtyLiquidationFee = (_liquidationFee * 9000) / LIQ_PRECISION; // 90% of clean fee

In the liquidateClean function, the dirtyLiquidationFee is used to decide the amount of collateral that the user should get when they are not fully liquidated, and a decrease in the dirtyLiquidationFee will reduce the amount of collateral that the liquidator gets, thus making the liquidator suffer a loss.

            uint256 _optimisticCollateralForLiquidator = (_liquidationAmountInCollateralUnits *
                (LIQ_PRECISION + cleanLiquidationFee)) / LIQ_PRECISION;

            // Because interest accrues every block, _liquidationAmountInCollateralUnits from a few lines up is an ever increasing value
            // This means that leftoverCollateral can occasionally go negative by a few hundred wei (cleanLiqFee premium covers this for liquidator)
            _leftoverCollateral = (_userCollateralBalance.toInt256() - _optimisticCollateralForLiquidator.toInt256());

            // If cleanLiquidation fee results in no leftover collateral, give liquidator all the collateral
            // This will only be true when there liquidator is cleaning out the position
            _collateralForLiquidator = _leftoverCollateral <= 0
                ? _userCollateralBalance
                : (_liquidationAmountInCollateralUnits * (LIQ_PRECISION + dirtyLiquidationFee)) / LIQ_PRECISION;

Proof of Concept

https://github.com/code-423n4/2022-08-frax/blob/c4189a3a98b38c8c962c5ea72f1a322fbc2ae45f/src/contracts/FraxlendPairCore.sol#L194-L195 https://github.com/code-423n4/2022-08-frax/blob/c4189a3a98b38c8c962c5ea72f1a322fbc2ae45f/src/contracts/FraxlendPairCore.sol#L979-L990

Tools Used

None

Recommended Mitigation Steps

Change 9000 to 90000

-            dirtyLiquidationFee = (_liquidationFee * 9000) / LIQ_PRECISION; // 90% of clean fee
+            dirtyLiquidationFee = (_liquidationFee * 90000) / LIQ_PRECISION; // 90% of clean fee

[Low-01] Must approve 0 first

Impact

Some tokens (like USDT) do not work when changing the allowance from an existing non-zero allowance value. They must first be approved by zero and then the actual allowance must be approved.

Proof of Concept

https://github.com/code-423n4/2022-08-frax/blob/c4189a3a98b38c8c962c5ea72f1a322fbc2ae45f/src/contracts/FraxlendPairCore.sol#L1103-L1104 https://github.com/code-423n4/2022-08-frax/blob/c4189a3a98b38c8c962c5ea72f1a322fbc2ae45f/src/contracts/FraxlendPairCore.sol#L1184-L1185

Tools Used

None

Recommended Mitigation Steps

Set the allowance to zero immediately before each of the existing approve() calls.

[Low-02] Chainlink's latestRoundData might return stale or incorrect results

Impact

On FraxlendPairCore.sol, we are using latestRoundData, but there is no check if the return value indicates stale data.

        if (oracleMultiply != address(0)) {
            (, int256 _answer, , , ) = AggregatorV3Interface(oracleMultiply).latestRoundData();
            if (_answer <= 0) {
                revert OracleLTEZero(oracleMultiply);
            }
            _price = _price * uint256(_answer);
        }

        if (oracleDivide != address(0)) {
            (, int256 _answer, , , ) = AggregatorV3Interface(oracleDivide).latestRoundData();
            if (_answer <= 0) {
                revert OracleLTEZero(oracleDivide);
            }
            _price = _price / uint256(_answer);
        }

This could lead to stale prices according to the Chainlink documentation:

https://docs.chain.link/docs/historical-price-data/#historical-rounds https://docs.chain.link/docs/faq/#how-can-i-check-if-the-answer-to-a-round-is-being-carried-over-from-a-previous-round

Proof of Concept

https://github.com/code-423n4/2022-08-frax/blob/c4189a3a98b38c8c962c5ea72f1a322fbc2ae45f/src/contracts/FraxlendPairCore.sol#L523-L537

Tools Used

None

Recommended Mitigation Steps

        if (oracleMultiply != address(0)) {
            (uint80 roundID, int256 _answer, , uint256 timestamp, uint80 answeredInRound) = AggregatorV3Interface(oracleMultiply).latestRoundData();
            require(answeredInRound >= roundID, "Stale price");
		    require(timestamp != 0,"Round not complete");
            if (_answer <= 0) {
                revert OracleLTEZero(oracleMultiply);
            }
            _price = _price * uint256(_answer);
        }

        if (oracleDivide != address(0)) {
            (uint80 roundID, int256 _answer, , uint256 timestamp, uint80 answeredInRound) = AggregatorV3Interface(oracleDivide).latestRoundData();
            require(answeredInRound >= roundID, "Stale price");
		    require(timestamp != 0,"Round not complete");
            if (_answer <= 0) {
                revert OracleLTEZero(oracleDivide);
            }
            _price = _price / uint256(_answer);
        }