ParaSpace contest - Kong's results

Lines of code

https://github.com/code-423n4/2022-11-paraspace/blob/main/paraspace-core/contracts/misc/NFTFloorOracle.sol#L167-L172

Vulnerability details

Impact

In the NFTFloorOracle contract, the removeFeeder function is used to remove the Feeder role in the contract, and the addFeeders function is used to add the Feeder role in the contract. Only the DEFAULT_ADMIN_ROLE role can perform the addFeeders operation, but the removeFeeder function can be called by any user. This will lead to the risk of malicious removal of the Feeder role in the contract.

Proof of Concept

    function removeFeeder(address _feeder)
        external
        onlyWhenFeederExisted(_feeder)
    {
        _removeFeeder(_feeder);
    }

https://github.com/code-423n4/2022-11-paraspace/blob/main/paraspace-core/contracts/misc/NFTFloorOracle.sol#L167-L172

Tools Used

N/A

Recommended Mitigation Steps

It is recommended to add permission control to the removeFeeder function.

Lines of code

https://github.com/code-423n4/2022-11-paraspace/blob/main/paraspace-core/contracts/misc/ParaSpaceFallbackOracle.sol#L43-L61

Vulnerability details

Impact

In the ParaSpaceFallbackOracle contract, the getAssetPrice function is used to obtain the price of the specified token. When the token is a non-ERC721 token, it will obtain the reserve amount of the pool through the getReserves function of the Pair contract, and calculate the price through the getAmountOut interface. This is an extremely easy-to-manipulate price acquisition method. As long as malicious users use a large amount of funds to perform swap operations in the Pair, they can manipulate the price calculation results. And malicious users can use flash loans to reduce manipulation costs. Therefore, it is extremely dangerous to use this method to obtain prices. The ParaSpaceFallbackOracle::getAssetPrice function is called by the getAssetPrice function of the ParaSpaceOracle contract. When assetsSources[asset] is 0, the ParaSpaceFallbackOracle::getAssetPrice call can be triggered. And ParaSpaceOracle::getAssetPrice is used in the validateBorrow, calculateUserAccountData, _calculateERC20LiquidationParameters operations of the protocol. These are the core functions to ensure the stable operation of the protocol. Once manipulated, it will cause losses to users' assets.

Proof of Concept

    function getAssetPrice(address asset) public view returns (uint256) {
       ...
        address pairAddress = IUniswapV2Factory(UNISWAP_FACTORY).getPair(
            WETH,
            asset
        );
        require(pairAddress != address(0x00), "pair not found");
        IUniswapV2Pair pair = IUniswapV2Pair(pairAddress);
        (uint256 left, uint256 right, ) = pair.getReserves();
        (uint256 tokenReserves, uint256 ethReserves) = (asset < WETH)
            ? (left, right)
            : (right, left);
        uint8 decimals = ERC20(asset).decimals();
        //returns price in 18 decimals
        return
            IUniswapV2Router01(UNISWAP_ROUTER).getAmountOut(
                10**decimals,
                tokenReserves,
                ethReserves
            );
    }

https://github.com/code-423n4/2022-11-paraspace/blob/main/paraspace-core/contracts/misc/ParaSpaceFallbackOracle.sol#L43-L61

    function getAssetPrice(address asset)
        public
        view
        override
        returns (uint256)
    {
        ...
        if (price == 0 && address(_fallbackOracle) != address(0)) {
            price = _fallbackOracle.getAssetPrice(asset);
        }
        ...
    }

https://github.com/code-423n4/2022-11-paraspace/blob/main/paraspace-core/contracts/misc/ParaSpaceOracle.sol#L131

Tools Used

N/A

Recommended Mitigation Steps

If the protocol needs to obtain prices from Uniswap v2 Pairs, a safe implementation is to use TWAP oracles. It uses a time-weighted approach to deal with short-term price manipulation. The following is the implementation reference of the TWAP oracle: https://github.com/Uniswap/v2-periphery/blob/master/contracts/examples/ExampleOracleSimple.sol