code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-03-polynomial/blob/main/src/Exchange.sol#L186-L202 https://github.com/code-423n4/2023-03-polynomial/blob/main/src/LiquidityPool.sol#L399-L401 https://github.com/code-423n4/2023-03-polynomial/blob/main/src/KangarooVault.sol#L401-L420 https://github.com/code-423n4/2023-03-polynomial/blob/main/src/KangarooVault.sol#L556-L611 https://github.com/code-423n4/2023-03-polynomial/blob/main/src/KangarooVault.sol#L436-L447 https://github.com/code-423n4/2023-03-polynomial/blob/main/src/LiquidityPool.sol#L404-L406

Vulnerability details

Impact

The following Exchange.getMarkPrice function uses pool.baseAssetPrice()'s returned baseAssetPrice, which is spotPrice returned by perpMarket.assetPrice(), to calculate and return the markPrice. When such spotPrice is 0, this function would return a 0 markPrice.

https://github.com/code-423n4/2023-03-polynomial/blob/main/src/Exchange.sol#L186-L202

    function getMarkPrice() public view override returns (uint256 markPrice, bool isInvalid) {
        (uint256 baseAssetPrice, bool invalid) = pool.baseAssetPrice();
        isInvalid = invalid;

        (int256 fundingRate,) = getFundingRate();
        fundingRate = fundingRate / 1 days;

        int256 currentTimeStamp = int256(block.timestamp);
        int256 fundingLastUpdatedTimestamp = int256(fundingLastUpdated);

        int256 totalFunding = wadMul(fundingRate, (currentTimeStamp - fundingLastUpdatedTimestamp));
        int256 normalizationUpdate = 1e18 - totalFunding;
        uint256 newNormalizationFactor = normalizationFactor.mulWadDown(uint256(normalizationUpdate));

        uint256 squarePrice = baseAssetPrice.mulDivDown(baseAssetPrice, PRICING_CONSTANT);
        markPrice = squarePrice.mulWadDown(newNormalizationFactor);
    }

https://github.com/code-423n4/2023-03-polynomial/blob/main/src/LiquidityPool.sol#L399-L401

    function baseAssetPrice() public view override returns (uint256 spotPrice, bool isInvalid) {
        (spotPrice, isInvalid) = perpMarket.assetPrice();
    }

As shown by the code below, calling the KangarooVault.transferPerpMargin and KangarooVault._openPosition functions would revert if baseAssetPrice returned by LIQUIDITY_POOL.baseAssetPrice() is 0 no matter what the returned isInvalid is. This means that the price returned by perpMarket.assetPrice() should not be trusted and used whenever such price is 0.

https://github.com/code-423n4/2023-03-polynomial/blob/main/src/KangarooVault.sol#L401-L420

    function transferPerpMargin(int256 marginDelta) external requiresAuth nonReentrant {
        if (marginDelta < 0) {
            ...
            (uint256 baseAssetPrice, bool isInvalid) = LIQUIDITY_POOL.baseAssetPrice();
            require(!isInvalid && baseAssetPrice != 0);
            ...
        } else {
            ...
        }
        ...
    }

https://github.com/code-423n4/2023-03-polynomial/blob/main/src/KangarooVault.sol#L556-L611

    function _openPosition(uint256 amt, uint256 minCost) internal {
        ...
        (uint256 baseAssetPrice, bool isInvalid) = LIQUIDITY_POOL.baseAssetPrice();
        require(!isInvalid && baseAssetPrice != 0);
        ...
    }

However, some functions that call the Exchange.getMarkPrice function do not additionally check if the Exchange.getMarkPrice function's returned markPrice is 0, which can lead to unexpected consequences. For example, the following KangarooVault.removeCollateral function executes (uint256 markPrice,) = LIQUIDITY_POOL.getMarkPrice(). When markPrice is 0, which is caused by a 0 spotPrice returned by perpMarket.assetPrice(), such price should be considered as invalid and should not be used; yet, in this case, such 0 markPrice can cause minColl to also be 0, which then makes require(positionData.totalCollateral >= minColl + collateralToRemove) much more likely to be passed. In this situation, calling the KangarooVault.removeCollateral function can remove the specified collateralToRemove collateral from the Power Perp position but this actually should not be allowed because such 0 spotPrice and 0 markPrice should be considered as invalid and should not be used.

https://github.com/code-423n4/2023-03-polynomial/blob/main/src/KangarooVault.sol#L436-L447

    function removeCollateral(uint256 collateralToRemove) external requiresAuth nonReentrant {
        (uint256 markPrice,) = LIQUIDITY_POOL.getMarkPrice();
        uint256 minColl = positionData.shortAmount.mulWadDown(markPrice);
        minColl = minColl.mulWadDown(collRatio);

        require(positionData.totalCollateral >= minColl + collateralToRemove);

        usedFunds -= collateralToRemove;
        positionData.totalCollateral -= collateralToRemove;

        emit RemoveCollateral(positionData.positionId, collateralToRemove);
    }

https://github.com/code-423n4/2023-03-polynomial/blob/main/src/LiquidityPool.sol#L404-L406

    function getMarkPrice() public view override returns (uint256, bool) {
        return exchange.getMarkPrice();
    }

Proof of Concept

The following steps can occur for the described scenario.

The KangarooVault.removeCollateral function is called with collateralToRemove being 100e18.
markPrice returned by LIQUIDITY_POOL.getMarkPrice() is 0 because a 0 spotPrice is returned by perpMarket.assetPrice().
Due to the 0 markPrice, minColl is 0, and positionData.totalCollateral >= minColl + collateralToRemove can be true even if positionData.totalCollateral is also 100e18 at this moment.
Calling the KangarooVault.removeCollateral function does not revert, and 100e18 collateral is remove from the Power Perp position.
However, a 0 spotPrice returned by perpMarket.assetPrice() and a 0 markPrice returned by LIQUIDITY_POOL.getMarkPrice() should be considered as invalid and should not be used. In this case, removing 100e18 collateral from the Power Perp position should not be allowed or succeed but it does.

Tools Used

VSCode

Recommended Mitigation Steps

Functions, such as the KangarooVault.removeCollateral function, that call the Exchange.getMarkPrice function can be updated to additionally check if the Exchange.getMarkPrice function's returned markPrice is 0. If it is 0, calling these functions should revert.

c4-sponsor commented 1 year ago

mubaris marked the issue as sponsor confirmed

c4-judge commented 1 year ago

JustDravee marked the issue as satisfactory