Lines of code

https://github.com/bunkerfinance/bunker-protocol/blob/752126094691e7457d08fc62a6a5006df59bd2fe/contracts/Comptroller.sol#L764-774 https://github.com/bunkerfinance/bunker-protocol/blob/752126094691e7457d08fc62a6a5006df59bd2fe/contracts/Comptroller.sol#L701-L727 https://github.com/bunkerfinance/bunker-protocol/blob/752126094691e7457d08fc62a6a5006df59bd2fe/contracts/CToken.sol#L1082

Vulnerability details

impact

_setNftPriceOracle is an admin-only function which can set a new nftOracle. But there is no restriction in this function. An admin can set any oracle unconditionally. It could be used to manipulate nft prices in liquidateCalculateSeizeNfts().

Proof of Concept

In liquidateBorrowFreshNft(), it calls liquidateCalculateSeizeNfts() to calculate the cNFT tokens to seize given an underlying amount.

https://github.com/bunkerfinance/bunker-protocol/blob/752126094691e7457d08fc62a6a5006df59bd2fe/contracts/CToken.sol#L1082

    function liquidateBorrowNftFresh(address liquidator, address borrower, uint repayAmount, CNftInterface cNftCollateral, uint[] memory seizeIds, uint[] memory seizeAmounts) internal returns (uint, uint) {
        …
        (uint amountSeizeError, uint seizeTokens) = comptroller.liquidateCalculateSeizeNfts(address(this), address(cNftCollateral), actualRepayAmount);
        …
    }

In liquidateCalculateSeizeNfts(), it uses nftOracle.getUnderlyingPrice() in calculation.

https://github.com/bunkerfinance/bunker-protocol/blob/752126094691e7457d08fc62a6a5006df59bd2fe/contracts/Comptroller.sol#L701-L727

    function liquidateCalculateSeizeNfts(address cTokenBorrowed, address cNftCollateral, uint actualRepayAmount) external view returns (uint, uint) {
        require(cNftCollateral == address(nftMarket), "cNFT is from the wrong comptroller");

        /* Read oracle prices for borrowed and collateral markets */
        uint priceBorrowedMantissa = oracle.getUnderlyingPrice(CToken(cTokenBorrowed));
        uint priceCollateralMantissa = nftOracle.getUnderlyingPrice(nftMarket);
        if (priceBorrowedMantissa == 0 || priceCollateralMantissa == 0) {
            return (uint(Error.PRICE_ERROR), 0);
        }

        /*
         * Get the exchange rate and calculate the number of collateral tokens to seize:
         *  seizeTokens = actualRepayAmount * liquidationIncentive * priceBorrowed / priceCollateral
         */
        uint seizeTokens;
        Exp memory numerator;
        Exp memory denominator;
        Exp memory ratio;

        numerator = mul_(Exp({mantissa: liquidationIncentiveMantissa}), Exp({mantissa: priceBorrowedMantissa}));
        denominator = Exp({mantissa: priceCollateralMantissa});
        ratio = div_(numerator, denominator);

        seizeTokens = truncate(mul_(ratio, Exp({mantissa: actualRepayAmount})));

        return (uint(Error.NO_ERROR), seizeTokens);
    }

However, nftOracle can be set by the admin unconditionally. Which means that the price can be manipulated. The admin can then call liquidateBorrowNft to get NFTs with incorrect repayAmount.

    function _setNftPriceOracle(NftPriceOracle newOracle) public returns (uint) {
        // Check caller is admin
        if (msg.sender != admin) {
            return fail(Error.UNAUTHORIZED, FailureInfo.SET_PRICE_ORACLE_OWNER_CHECK);
        }

        // Set comptroller's nft oracle to newOracle
        nftOracle = newOracle;

        return uint(Error.NO_ERROR);
    }

Tools Used

vim

Recommended Mitigation Steps

Add restrictions(e.g. timelock) in _setNftPriceOracle

code-423n4 / 2022-05-bunker-findings