ParaSpace contest - minhquanym's results

Lines of code

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

Vulnerability details

Impact

In NFTFloorPrice, there are multiple components to allow easy adding/removing keepers

• address[] public feeders - List of all keepers' address
• mapping(address => FeederPosition{index, register}) private feederPositionMap

When adding new keeper, new keeper address is pushed to feeders and its index is stored in feederPositionMap. Its index is used to find position of keeper in the list to easily delete.

function _addFeeder(address _feeder)
    internal
    onlyWhenFeederNotExisted(_feeder)
{
    feeders.push(_feeder);
    feederPositionMap[_feeder].index = uint8(feeders.length - 1);
    feederPositionMap[_feeder].registered = true;
    _setupRole(UPDATER_ROLE, _feeder);
    emit FeederAdded(_feeder);
}

When removing a keeper, its will use feederPositionMap to find its position, swap it with the last element in the feeders list, and pop the last element.

function _removeFeeder(address _feeder)
    internal
    onlyWhenFeederExisted(_feeder)
{
    uint8 feederIndex = feederPositionMap[_feeder].index;
    if (feederIndex >= 0 && feeders[feederIndex] == _feeder) {
        feeders[feederIndex] = feeders[feeders.length - 1]; 
        // @audit did not update index for last feeder.
        feeders.pop();
    }
    delete feederPositionMap[_feeder];
    revokeRole(UPDATER_ROLE, _feeder);
    emit FeederRemoved(_feeder);
}

However, function _removeFeeder() did not update the index for the last feeder - the one replaced position of removed keeper.

The result is even last keeper had been moved to different position in the list, it still kept the same index.

Proof of Concept

There is 2 cases after admin _removeFeeder() and feeders' index is in wrong state:

1. Admin add new keeper, new keeper index will be the same as the wrong-index keeper. Now removing keeper A can result in keeper B removed.
2. Admin remove the wrong-index keeper by using _removeFeeder(), it will point to wrong index and result in TX reverted.

So the more admin trying to fix this onchain, the more the keepers be indexed wrongly.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider updating correct index for the last keeper in _removeKeeper() function.

Lines of code

https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/misc/UniswapV3OracleWrapper.sol#L176

Vulnerability details

Impact

In Paraspace protocol, any Uniswap V3 position that are consist of ERC20 tokens that Paraspace support can be used as collateral to borrow funds from Paraspace pool. The value of the Uniswap V3 position will be sum of value of ERC20 tokens in it.

function getTokenPrice(uint256 tokenId) public view returns (uint256) {
    UinswapV3PositionData memory positionData = getOnchainPositionData(
        tokenId
    );

    PairOracleData memory oracleData = _getOracleData(positionData);

    (uint256 liquidityAmount0, uint256 liquidityAmount1) = LiquidityAmounts
        .getAmountsForLiquidity(
            oracleData.sqrtPriceX96,
            TickMath.getSqrtRatioAtTick(positionData.tickLower),
            TickMath.getSqrtRatioAtTick(positionData.tickUpper),
            positionData.liquidity
        );

    (
        uint256 feeAmount0,
        uint256 feeAmount1
    ) = getLpFeeAmountFromPositionData(positionData);

    return // @audit can be easily manipulated with low TVL pool
        (((liquidityAmount0 + feeAmount0) * oracleData.token0Price) /
            10**oracleData.token0Decimal) +
        (((liquidityAmount1 + feeAmount1) * oracleData.token1Price) /
            10**oracleData.token1Decimal);
}

However, Uniswap V3 can have multiple pools for the same pairs of ERC20 tokens with different fee params. A fews has most the liquidity, while other pools have extremely little TVL or even not created yet. Attackers can abuse it, create low TVL pool where liquidity in this pool mostly (or fully) belong to attacker’s position, deposit this position as collateral and borrow token in Paraspace pool, swap to make the original position reduce the original value and cause Paraspace pool in loss.

Proof of Concept

Consider the scenario where WETH and DAI are supported as collateral in Paraspace protocol.

1. Alice (attacker) create a new WETH/DAI pool in Uniswap V3 and add liquidity with the following amount 1e18 wei WETH - 1e6 wei DAI = 1 WETH - 1e-12 DAI ~= 1 ETH Let's just assume Alice position has price range from [MIN_TICK, MAX_TICK] so the math can be approximately like Uniswap V2 - constant product. Note that this pool only has liquidity from Alice.
2. Alice deposit this position into Paraspace, value of this position is approximately 1 WETH and Alice borrow maximum possible amount of USDC.
3. Alice make swap in her WETH/DAI pool in Uniswap V3 to make the position become 1e6 wei WETH - 1e18 wei DAI = 1e-12 WETH - 1 DAI ~= 1 DAI

Please note that the math I've done above is approximation based on Uniswap V2 formula x * y = k because Alice provided liquidity from MIN_TICK to MAX_TICK. For more information about Uniswap V3 formula, please check their whitepaper here: https://uniswap.org/whitepaper-v3.pdf

Tools Used

Manual Review

Recommended Mitigation Steps

Consider adding whitelist, only allowing pool with enough TVL to be collateral in Paraspace protocol.

Lines of code

https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/misc/NFTFloorOracle.sol#L284 https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/misc/NFTFloorOracle.sol#L312

Vulnerability details

Impact

In NFTFloorPrice, it has a set of feeders and a set of assets. Feeders will set price for assets. Of coursers, with the growing NFT space, number of assets collection could be much more than 256 and because Paraspace uses median price from keepers so number of keepers must be even larger.

However, in both functions _addAsset() and _addFeeder(), it casted the index of newly added asset and feeder to uint8. If there is more than 256 feeders for example, its index will be overflow and return incorrect value.

function _addAsset(address _asset)
    internal
    onlyWhenAssetNotExisted(_asset)
{
    assetFeederMap[_asset].registered = true;
    assets.push(_asset);
    assetFeederMap[_asset].index = uint8(assets.length - 1); // @audit unsafe cast
    emit AssetAdded(_asset);
}

function _addFeeder(address _feeder)
    internal
    onlyWhenFeederNotExisted(_feeder)
{
    feeders.push(_feeder);
    feederPositionMap[_feeder].index = uint8(feeders.length - 1); // @audit unsafe cast
    feederPositionMap[_feeder].registered = true;
    _setupRole(UPDATER_ROLE, _feeder);
    emit FeederAdded(_feeder);
}

Proof of Concept

Consider the scenario

1. NFTPriceOracle already had 256 feeders so last feeder index now is 255
2. Admin add new feeder, its index will be overflow and become 0.
3. Now if admin remove last feeder, it will actually remove the first feeder, because first feeder is actually the feeder with index 0.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider using SafeCast library from OpenZeppelin instead https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/math/SafeCast.sol