Polynomial Protocol contest - rbserver's results

Lines of code

https://github.com/code-423n4/2023-03-polynomial/blob/main/src/Exchange.sol#L333-L353 https://github.com/code-423n4/2023-03-polynomial/blob/main/src/ShortCollateral.sol#L121-L144

Vulnerability details

Impact

When calling the following Exchange._liquidate function, uint256 totalCollateralReturned = shortCollateral.liquidate(positionId, debtRepaying, msg.sender) is executed.

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

    function _liquidate(uint256 positionId, uint256 debtRepaying) internal {
        uint256 maxDebtRepayment = shortCollateral.maxLiquidatableDebt(positionId);
        require(maxDebtRepayment > 0);
        if (debtRepaying > maxDebtRepayment) debtRepaying = maxDebtRepayment;

        IShortToken.ShortPosition memory position = shortToken.shortPositions(positionId);

        uint256 totalCollateralReturned = shortCollateral.liquidate(positionId, debtRepaying, msg.sender);

        address user = shortToken.ownerOf(positionId);

        uint256 finalPosition = position.shortAmount - debtRepaying;
        uint256 finalCollateralAmount = position.collateralAmount - totalCollateralReturned;

        shortToken.adjustPosition(positionId, user, position.collateral, finalPosition, finalCollateralAmount);

        pool.liquidate(debtRepaying);
        powerPerp.burn(msg.sender, debtRepaying);
        ...
    }

In the following ShortCollateral.liquidate function, when executing uint256 collateralClaim = debt.mulDivDown(markPrice, collateralPrice), where debt is debtRepaying, collateralClaim can be high if collateralPrice has become much lower comparing to markPrice, such as due to a market sell-off that causes the collateral to be worth much less than before. In this case, totalCollateralReturned can be high as well, which can cause totalCollateralReturned > userCollateral.amount to be true. When such condition is true, totalCollateralReturned = userCollateral.amount is executed, and only userCollateral.amount is transferred to the liquidator after executing ERC20(userCollateral.collateral).safeTransfer(user, totalCollateralReturned).

https://github.com/code-423n4/2023-03-polynomial/blob/main/src/ShortCollateral.sol#L121-L144

    function liquidate(uint256 positionId, uint256 debt, address user)
        external
        override
        onlyExchange
        nonReentrant
        returns (uint256 totalCollateralReturned)
    {
        UserCollateral storage userCollateral = userCollaterals[positionId];

        bytes32 currencyKey = synthetixAdapter.getCurrencyKey(userCollateral.collateral);
        Collateral memory coll = collaterals[currencyKey];

        (uint256 markPrice,) = exchange.getMarkPrice();
        (uint256 collateralPrice,) = synthetixAdapter.getAssetPrice(currencyKey);
        uint256 collateralClaim = debt.mulDivDown(markPrice, collateralPrice);
        uint256 liqBonus = collateralClaim.mulWadDown(coll.liqBonus);
        totalCollateralReturned = liqBonus + collateralClaim;
        if (totalCollateralReturned > userCollateral.amount) totalCollateralReturned = userCollateral.amount;
        userCollateral.amount -= totalCollateralReturned;

        ERC20(userCollateral.collateral).safeTransfer(user, totalCollateralReturned);
        ...
    }

Back in the Exchange._liquidate function, the liquidator burns debtRepaying powerPerp tokens after powerPerp.burn(msg.sender, debtRepaying) is executed. However, in this situation, the liquidator only receives userCollateral.amount collateral tokens that are less than the collateral token amount that should be equivalent to debtRepaying powerPerp tokens but this liquidator still burns debtRepaying powerPerp tokens. As a result, this liquidator loses the extra powerPerp tokens, which are burnt, that are equivalent to the difference between debtRepaying powerPerp tokens' equivalent collateral token amount and userCollateral.amount collateral tokens.

Proof of Concept

The following steps can occur for the described scenario.

1. Alice calls the Exchange._liquidate function with debtRepaying being 1000e18.
2. When the ShortCollateral.liquidate function is called, totalCollateralReturned > userCollateral.amount is true, and userCollateral.amount collateral tokens that are equivalent to 500e18 powerPerp tokens are transferred to Alice.
3. When powerPerp.burn(msg.sender, debtRepaying) is executed in the Exchange._liquidate function, Alice burns 1000e18 powerPerp tokens.
4. Because Alice only receives userCollateral.amount collateral tokens that are equivalent to 500e18 powerPerp tokens, she loses 500e18 powerPerp tokens.

Tools Used

VSCode

Recommended Mitigation Steps

The Exchange._liquidate function can be updated to burn the number of powerPerp tokens that are equivalent to the actual collateral token amount received by the liquidator instead of burning debtRepaying powerPerp tokens.

Lines of code

https://github.com/code-423n4/2023-03-polynomial/blob/main/src/KangarooVault.sol#L19-L21 https://github.com/code-423n4/2023-03-polynomial/blob/main/src/KangarooVault.sol#L183 https://github.com/code-423n4/2023-03-polynomial/blob/main/src/KangarooVault.sol#L243 https://github.com/code-423n4/2023-03-polynomial/blob/main/src/KangarooVault.sol#L215 https://github.com/code-423n4/2023-03-polynomial/blob/main/src/KangarooVault.sol#L269 https://github.com/code-423n4/2023-03-polynomial/blob/main/src/LiquidityPool.sol#L184 https://github.com/code-423n4/2023-03-polynomial/blob/main/src/LiquidityPool.sol#L200-L205 https://github.com/code-423n4/2023-03-polynomial/blob/main/src/LiquidityPool.sol#L219 https://github.com/code-423n4/2023-03-polynomial/blob/main/src/LiquidityPool.sol#L247 https://github.com/code-423n4/2023-03-polynomial/blob/main/src/LiquidityPool.sol#L264-L269 https://github.com/code-423n4/2023-03-polynomial/blob/main/src/LiquidityPool.sol#L284

Vulnerability details

Impact

As shown by the code below, although PauseModifier is imported, the KangarooVault contract does not use the whenNotPaused modifier in any of its functions. More specifically, the KangarooVault.initiateDeposit, KangarooVault.processDepositQueue, KangarooVault.initiateWithdrawal, and KangarooVault.processWithdrawalQueue functions do not use the whenNotPaused modifier.

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

import {PauseModifier} from "./utils/PauseModifier.sol";

contract KangarooVault is Auth, ReentrancyGuard, PauseModifier {

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

    function initiateDeposit(address user, uint256 amount) external nonReentrant {

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

    function processDepositQueue(uint256 idCount) external nonReentrant {

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

    function initiateWithdrawal(address user, uint256 tokens) external nonReentrant {

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

    function processWithdrawalQueue(uint256 idCount) external nonReentrant {

This is unlike the LiquidityPool contract; comparing to the KangarooVault.initiateDeposit, KangarooVault.processDepositQueue, KangarooVault.initiateWithdrawal, and KangarooVault.processWithdrawalQueue functions, the LiquidityPool.deposit, LiquidityPool.queueDeposit, LiquidityPool.processDeposits, LiquidityPool.withdraw, LiquidityPool.queueWithdraw, and LiquidityPool.processWithdraws functions have the similar functionalities but they all use the whenNotPaused modifier. As a result, when an emergency, such as a hack, occurs, the protocol can pause the LiquidityPool.withdraw, LiquidityPool.queueWithdraw, and LiquidityPool.processWithdraws functions to prevent or reduce damages, such as preventing users and the protocol from losing funds, but cannot do that for the KangarooVault.initiateDeposit, KangarooVault.processDepositQueue, KangarooVault.initiateWithdrawal, and KangarooVault.processWithdrawalQueue functions.

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

    function deposit(uint256 amount, address user) external override nonReentrant whenNotPaused("POOL_DEPOSIT") {

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

    function queueDeposit(uint256 amount, address user)
        external
        override
        nonReentrant
        whenNotPaused("POOL_QUEUE_DEPOSIT")
    {

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

    function processDeposits(uint256 count) external override nonReentrant whenNotPaused("POOL_PROCESS_DEPOSITS") {

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

    function withdraw(uint256 tokens, address user) external override nonReentrant whenNotPaused("POOL_WITHDRAW") {

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

    function queueWithdraw(uint256 tokens, address user)
        external
        override
        nonReentrant
        whenNotPaused("POOL_QUEUE_WITHDRAW")
    {

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

    function processWithdraws(uint256 count) external override nonReentrant whenNotPaused("POOL_PROCESS_WITHDRAWS") {

Proof of Concept

The following steps can occur for the described scenario.

1. An emergency, such as a hack, occurs in which further withdrawals can cause users and the protocol to lose funds.
2. The protocol team is able to pause the LiquidityPool.withdraw, LiquidityPool.queueWithdraw, and LiquidityPool.processWithdraws functions.
3. However, the protocol team is unable to pause the KangarooVault.initiateWithdrawal and KangarooVault.processWithdrawalQueue functions.
4. As a result, funds can be lost from the KangarooVault.

Tools Used

VSCode

Recommended Mitigation Steps

The KangarooVault.initiateDeposit, KangarooVault.processDepositQueue, KangarooVault.initiateWithdrawal, and KangarooVault.processWithdrawalQueue functions can be updated to use the whenNotPaused modifier.

Lines of code

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/LiquidityPool.sol#L727-L735 https://github.com/code-423n4/2023-03-polynomial/blob/main/src/LiquidityPool.sol#L764-L771 https://github.com/code-423n4/2023-03-polynomial/blob/main/src/LiquidityPool.sol#L399-L401

Vulnerability details

Impact

The following KangarooVault.transferPerpMargin and KangarooVault._openPosition functions execute require(!isInvalid && baseAssetPrice != 0), where isInvalid and baseAssetPrice are returned from calling LIQUIDITY_POOL.baseAssetPrice(). Because LIQUIDITY_POOL.baseAssetPrice() returns the return values of perpMarket.assetPrice(), the KangarooVault.transferPerpMargin and KangarooVault._openPosition functions would only consider perpMarket.assetPrice() as reliable when both !isInvalid and baseAssetPrice != 0 are true.

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, the following LiquidityPool._getDelta and LiquidityPool._calculateMargin functions execute require(!isInvalid || spotPrice > 0) and would consider perpMarket.assetPrice() as reliable when either !isInvalid or spotPrice > 0 is true. When perpMarket.assetPrice() returns a positive spotPrice and a true isInvalid, such spotPrice should be considered as invalid and untrusted; trades, such as these for opening and closing long and short positions using the LiquidityPool, that depend on the LiquidityPool._getDelta and LiquidityPool._calculateMargin functions' return values, which then rely on such invalid spotPrice, should not be allowed. However, because !isInvalid || spotPrice > 0 is true in this case, calling the LiquidityPool._getDelta and LiquidityPool._calculateMargin functions will not revert, and such trades that should not be allowed can still be made.

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

    function _getDelta() internal view returns (uint256 delta) {
        (uint256 spotPrice, bool isInvalid) = baseAssetPrice();
        uint256 pricingConstant = exchange.PRICING_CONSTANT();

        require(!isInvalid || spotPrice > 0);

        delta = spotPrice.mulDivDown(2e18, pricingConstant);
        delta = delta.mulWadDown(exchange.normalizationFactor());
    }

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

    function _calculateMargin(int256 size) internal view returns (uint256 margin) {
        (uint256 spotPrice, bool isInvalid) = baseAssetPrice();

        require(!isInvalid || spotPrice > 0);

        uint256 absSize = size.abs();
        margin = absSize.mulDivDown(spotPrice, futuresLeverage);
    }

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();
    }

Proof of Concept

The following steps can occur for the described scenario.

1. Calling the perpMarket.assetPrice function returns a positive spotPrice and a true isInvalid at this moment.
2. Calling the LiquidityPool._getDelta and LiquidityPool._calculateMargin functions would not revert because require(!isInvalid || spotPrice > 0) would be passed.
3. Trades, such as for closing a long position through calling the LiquidityPool.closeLong function, can go through even though the used spotPrice is invalid and untrusted.
4. In this case, such trades should not be allowed but are still made.

Tools Used

VSCode

Recommended Mitigation Steps

The LiquidityPool._getDelta and LiquidityPool._calculateMargin functions can be updated to execute require(!isInvalid && spotPrice > 0) instead of require(!isInvalid || spotPrice > 0).

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.

1. The KangarooVault.removeCollateral function is called with collateralToRemove being 100e18.
2. markPrice returned by LIQUIDITY_POOL.getMarkPrice() is 0 because a 0 spotPrice is returned by perpMarket.assetPrice().
3. 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.
4. Calling the KangarooVault.removeCollateral function does not revert, and 100e18 collateral is remove from the Power Perp position.
5. 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.

Lines of code

https://github.com/code-423n4/2023-03-polynomial/blob/main/src/KangarooVault.sol#L782-L797 https://github.com/code-423n4/2023-03-polynomial/blob/main/src/LiquidityPool.sol#L591-L609 https://github.com/code-423n4/2023-03-polynomial/blob/main/src/LiquidityPool.sol#L774-L778 https://github.com/code-423n4/2023-03-polynomial/blob/main/src/KangarooVault.sol#L341-L362 https://github.com/code-423n4/2023-03-polynomial/blob/main/src/LiquidityPool.sol#L340-L365

Vulnerability details

Impact

The following KangarooVault._resetTrade, LiquidityPool.rebalanceMargin, and LiquidityPool._getTotalMargin functions call the external perp market contract's remainingMargin function to get the remaining margin for the KangarooVault or LiquidityPool. Yet, none of these functions use isInvalid that can be returned by such remainingMargin function. When the returned remaining margin is invalid, such value should not be trusted and used. Using such invalid remaining margin can have negative effects.

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

    function _resetTrade() internal {
        positionData.positionId = 0;
        (uint256 totalMargin,) = PERP_MARKET.remainingMargin(address(this));
        PERP_MARKET.transferMargin(-int256(totalMargin));
        usedFunds -= totalMargin;
        ...
    }

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

    function rebalanceMargin(int256 marginDelta) external requiresAuth nonReentrant {
        int256 currentPosition = _getTotalPerpPosition();
        uint256 marginRequired = _calculateMargin(currentPosition);
        (uint256 currentMargin,) = perpMarket.remainingMargin(address(this));

        int256 additionalMargin = marginDelta;

        if (currentMargin >= marginRequired) {
            marginDelta -= int256(currentMargin - marginRequired);
        } else {
            marginDelta += int256(marginRequired - currentMargin);
        }
        ...
    }

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

    function _getTotalMargin() internal view returns (uint256) {
        (uint256 margin,) = perpMarket.remainingMargin(address(this));

        return margin;
    }

For example, we can compare the KangarooVault.getTokenPrice and LiquidityPool.getTokenPrice functions. The following KangarooVault.getTokenPrice function checks the isInvalid returned by PERP_MARKET.remainingMargin(address(this)) so calling it will revert if PERP_MARKET.remainingMargin(address(this))'s returned totalMargin is invalid. In contrast, the LiquidityPool.getTokenPrice function does not do this.

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

    function getTokenPrice() public view returns (uint256) {
        ...
        (totalMargin, isInvalid) = PERP_MARKET.remainingMargin(address(this));
        require(!isInvalid);
        ...
    }

In the following LiquidityPool.getTokenPrice function, when _getTotalMargin()'s returned totalMargin, which is also margin returned by perpMarket.remainingMargin(address(this)) in the LiquidityPool._getTotalMargin function, is invalid, such totalMargin is still used to increase totalValue, which then affects the liquidity token's price. The LiquidityPool.getTokenPrice function is called in functions like LiquidityPool.deposit and LiquidityPool.withdraw. Thus, calling such functions would not revert when such invalid totalMargin is used. As a result, the depositing and withdrawal actions that should not be allowed because of such invalid totalMargin can still be allowed unexpectedly.

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

    function getTokenPrice() public view override returns (uint256) {
        if (totalFunds == 0) {
            return 1e18;
        }

        uint256 totalSupply = liquidityToken.totalSupply() + totalQueuedWithdrawals;
        int256 skew = _getSkew();

        if (skew == 0) {
            return totalFunds.divWadDown(totalSupply);
        }

        (uint256 markPrice, bool isInvalid) = getMarkPrice();
        require(!isInvalid);

        uint256 totalValue = totalFunds;

        uint256 amountOwed = markPrice.mulWadDown(powerPerp.totalSupply());
        uint256 amountToCollect = markPrice.mulWadDown(shortToken.totalShorts());
        uint256 totalMargin = _getTotalMargin();

        totalValue += totalMargin + amountToCollect;
        totalValue -= uint256((int256(amountOwed) + usedFunds));

        return totalValue.divWadDown(totalSupply);
    }

Proof of Concept

The following steps can occur for the described scenario.

1. The LiquidityPool.deposit function is called to deposit some sUSD tokens.
2. When the LiquidityPool.getTokenPrice function is called, _getTotalMargin()'s returned totalMargin, which is margin returned by perpMarket.remainingMargin(address(this)), is invalid. However, such invalid totalMargin does not cause calling the LiquidityPool.getTokenPrice function to revert.
3. The deposit action succeeds while the used liquidity token's price that depends on the invalid totalMargin is inaccurate. Such deposit action should not be allowed but it is.

Tools Used

VSCode

Recommended Mitigation Steps

The KangarooVault._resetTrade, LiquidityPool.rebalanceMargin, and LiquidityPool._getTotalMargin functions can be updated to check isInvalid that is returned by the external perp market contract's remainingMargin function. If such isInvalid is true, calling these functions should revert.

Lines of code

https://github.com/code-423n4/2023-03-polynomial/blob/main/src/Exchange.sol#L333-L353 https://github.com/code-423n4/2023-03-polynomial/blob/main/src/ShortCollateral.sol#L121-L144 https://github.com/code-423n4/2023-03-polynomial/blob/main/src/SynthetixAdapter.sol#L26-L28

Vulnerability details

Impact

When calling the following Exchange._liquidate function, uint256 totalCollateralReturned = shortCollateral.liquidate(positionId, debtRepaying, msg.sender) is executed.

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

    function _liquidate(uint256 positionId, uint256 debtRepaying) internal {
        uint256 maxDebtRepayment = shortCollateral.maxLiquidatableDebt(positionId);
        require(maxDebtRepayment > 0);
        if (debtRepaying > maxDebtRepayment) debtRepaying = maxDebtRepayment;

        IShortToken.ShortPosition memory position = shortToken.shortPositions(positionId);

        uint256 totalCollateralReturned = shortCollateral.liquidate(positionId, debtRepaying, msg.sender);
        ...
    }

Then, in the following ShortCollateral.liquidate function, (uint256 collateralPrice,) = synthetixAdapter.getAssetPrice(currencyKey) is executed. collateralPrice returned by the SynthetixAdapter.getAssetPrice function is used; however, the SynthetixAdapter.getAssetPrice function also returns isInvalid but it is not used or checked. This means that it is possible that collateralPrice is invalid; in this case, such invalid collateralPrice can be used to determine an inaccurate totalCollateralReturned collateral token amount to be transferred to the liquidator. As a result, the liquidator can receive less or more collateral than she or he should receive while such liquidation action based on such invalid collateralPrice should actually not be allowed.

https://github.com/code-423n4/2023-03-polynomial/blob/main/src/ShortCollateral.sol#L121-L144

    function liquidate(uint256 positionId, uint256 debt, address user)
        external
        override
        onlyExchange
        nonReentrant
        returns (uint256 totalCollateralReturned)
    {
        UserCollateral storage userCollateral = userCollaterals[positionId];

        bytes32 currencyKey = synthetixAdapter.getCurrencyKey(userCollateral.collateral);
        Collateral memory coll = collaterals[currencyKey];

        (uint256 markPrice,) = exchange.getMarkPrice();
        (uint256 collateralPrice,) = synthetixAdapter.getAssetPrice(currencyKey);
        uint256 collateralClaim = debt.mulDivDown(markPrice, collateralPrice);
        uint256 liqBonus = collateralClaim.mulWadDown(coll.liqBonus);
        totalCollateralReturned = liqBonus + collateralClaim;
        if (totalCollateralReturned > userCollateral.amount) totalCollateralReturned = userCollateral.amount;
        userCollateral.amount -= totalCollateralReturned;

        ERC20(userCollateral.collateral).safeTransfer(user, totalCollateralReturned);
        ...
    }

https://github.com/code-423n4/2023-03-polynomial/blob/main/src/SynthetixAdapter.sol#L26-L28

    function getAssetPrice(bytes32 key) public view override returns (uint256, bool) {
        return exchangeRates.rateAndInvalid(key);
    }

Proof of Concept

The following steps can occur for the described scenario.

1. The Exchange._liquidate function is called by the liquidator.
2. When calling the ShortCollateral.liquidate function, the used collateralPrice that is returned by synthetixAdapter.getAssetPrice(currencyKey) is invalid.
3. Using such invalid collateralPrice, the calculated totalCollateralReturned is inaccurate and less than the collateral amount that the liquidator is entitled to.
4. The liquidator receives less collateral than she or he should receive because such liquidation action based on such invalid collateralPrice, which should not be allowed, is allowed.

Tools Used

VSCode

Recommended Mitigation Steps

The ShortCollateral.liquidate function can be updated to use and check isInvalid returned by synthetixAdapter.getAssetPrice(currencyKey). If such isInvalid is true, calling this function should revert.

Lines of code

https://github.com/code-423n4/2023-03-polynomial/blob/main/src/ShortCollateral.sol#L251-L268 https://github.com/code-423n4/2023-03-polynomial/blob/main/src/Exchange.sol#L233-L285 https://github.com/code-423n4/2023-03-polynomial/blob/main/src/ShortCollateral.sol#L153-L171

Vulnerability details

Impact

The ShortCollateral contract does not have a function to disapprove a collateral that was previously approved. Moreover, the following ShortCollateral.approveCollateral function always executes collateral.isApproved = true so it cannot be used to disapprove a collateral.

https://github.com/code-423n4/2023-03-polynomial/blob/main/src/ShortCollateral.sol#L251-L268

    function approveCollateral(bytes32 key, uint256 collateralRatio, uint256 liqRatio, uint256 liqBonus)
        external
        requiresAuth
    {
        Collateral storage collateral = collaterals[key];
        address synth = synthetixAdapter.getSynth(key);
        require(synth != address(0x0));
        require(liqRatio < collateralRatio);

        collateral.currencyKey = key;
        collateral.synth = synth;
        collateral.isApproved = true;
        ...
    }

When a collateral that was previously approved becomes hacked or untrusted, this collateral should not be used in the protocol anymore. However, since there is no way to disapprove such collateral, users can still interact with the protocol through using such collateral. For example, users can still call the following Exchange._openTrade function to open a new short position by providing the collateral, which was previously approved, that should no longer be accepted; when this function executes uint256 minCollateral = shortCollateral.getMinCollateral(totalShortAmount, params.collateral), require(coll.isApproved) will be passed in the ShortCollateral.getMinCollateral function. As a result, users can open positions and make trades using a collateral that should be disapproved and no longer accepted, and users and the protocol can lose funds because such collateral can be worthless.

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

    function _openTrade(TradeParams memory params) internal returns (uint256, uint256) {
        if (params.isLong) {
            ...
        } else {
            uint256 holdings = powerPerp.balanceOf(msg.sender);
            require(holdings == 0, "Long position must be closed before opening");

            IShortToken.ShortPosition memory shortPosition;

            uint256 totalShortAmount = params.amount;
            uint256 totalCollateralAmount = params.collateralAmount;

            ...

            uint256 minCollateral = shortCollateral.getMinCollateral(totalShortAmount, params.collateral);
            require(totalCollateralAmount >= minCollateral, "Not enough collateral");

            ERC20(params.collateral).safeTransferFrom(msg.sender, address(this), params.collateralAmount);

            params.positionId = shortToken.adjustPosition(
                params.positionId, msg.sender, params.collateral, totalShortAmount, totalCollateralAmount
            );

            ERC20(params.collateral).safeApprove(address(shortCollateral), params.collateralAmount);
            shortCollateral.collectCollateral(params.collateral, params.positionId, params.collateralAmount);

            uint256 totalCost = pool.openShort(params.amount, msg.sender, params.referralCode);
            uint256 minCost = params.minCost;
            require(totalCost >= minCost);

            ...
        }
    }

https://github.com/code-423n4/2023-03-polynomial/blob/main/src/ShortCollateral.sol#L153-L171

    function getMinCollateral(uint256 shortAmount, address collateral)
        public
        view
        override
        returns (uint256 collateralAmt)
    {
        bytes32 currencyKey = synthetixAdapter.getCurrencyKey(collateral);
        require(currencyKey != "");
        Collateral memory coll = collaterals[currencyKey];
        require(coll.isApproved);
        (uint256 markPrice, bool isInvalid) = exchange.getMarkPrice();
        require(!isInvalid);
        uint256 collateralPrice;
        (collateralPrice, isInvalid) = synthetixAdapter.getAssetPrice(currencyKey);
        require(!isInvalid);

        collateralAmt = markPrice.mulDivDown(shortAmount, collateralPrice);
        collateralAmt = collateralAmt.mulWadDown(coll.collateralRatio);
    }

Proof of Concept

The following steps can occur for the described scenario.

1. The collateral that was previously approved is hacked and becomes untrusted.
2. The protocol cannot disapprove such collateral since no function serves this purpose.
3. Users can still call the Exchange._openTrade function to open positions with this untrusted collateral.
4. The protocol ends up owning the collateral that can be worthless.

Tools Used

VSCode

Recommended Mitigation Steps

The ShortCollateral contract can be updated to add a function, which would be only callable by trusted admin, for disapproving a collateral that was previously approved.

QA REPORT

[01] LACK OF LIMITS FOR SETTING FEES

When calling the following LiquidityPool.setFees function, there are no limits for setting depositFee and withdrawalFee. If these fees are set to 1e18, calling the LiquidityPool.deposit function can cause all of the amount to become the deposit fees and zero liquidity tokens to be minted to the user, and calling the LiquidityPool.withdraw function can cause all of the susdToReturn to become the withdrawal fees and zero SUSD tokens to be transferred to the user. If these fees are set to be more than 1e18, calling the LiquidityPool.deposit function can revert because amount - fees underflows, and calling the LiquidityPool.withdraw function can also revert because susdToReturn - fees underflows.

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

    function setFees(uint256 _depositFee, uint256 _withdrawalFee) external requiresAuth {
        emit UpdateFees(depositFee, _depositFee, withdrawalFee, _withdrawalFee);
        depositFee = _depositFee;
        withdrawalFee = _withdrawalFee;
    }

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

    function deposit(uint256 amount, address user) external override nonReentrant whenNotPaused("POOL_DEPOSIT") {
        uint256 tokenPrice = getTokenPrice();
        uint256 fees = amount.mulWadDown(depositFee);
        uint256 amountForTokens = amount - fees;
        uint256 tokensToMint = amountForTokens.divWadDown(tokenPrice);
        liquidityToken.mint(user, tokensToMint);
        totalFunds += amountForTokens;
        SUSD.safeTransferFrom(msg.sender, feeReceipient, fees);
        SUSD.safeTransferFrom(msg.sender, address(this), amountForTokens);
        ...
    }

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

    function withdraw(uint256 tokens, address user) external override nonReentrant whenNotPaused("POOL_WITHDRAW") {
        ...
        uint256 tokenPrice = getTokenPrice();
        uint256 susdToReturn = tokens.mulWadDown(tokenPrice);
        uint256 fees = susdToReturn.mulWadDown(withdrawalFee);
        SUSD.safeTransfer(feeReceipient, fees);
        SUSD.transfer(user, susdToReturn - fees);
        totalFunds -= susdToReturn;
        liquidityToken.burn(msg.sender, tokens);
        ...
    }

Moreover, the LiquidityPool.setDevFee function has no limit for setting devFee, and similar issues can occur. For example, if devFee is set to more than 1e18, calling the LiquidityPool.openLong function will revert because externalFee is more than feesCollected and executing feesCollected - externalFee reverts.

As a mitigation, to prevent the LiquidityPool.deposit, LiquidityPool.withdraw, and LiquidityPool.openLong functions from behaving unexpectedly, the LiquidityPool.setFees and LiquidityPool.setDevFee functions can be updated to only allow the corresponding fees to be set to values that cannot exceed certain limits, which are reasonable values that are less than 1e18.

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

    function setDevFee(uint256 _devFee) external requiresAuth {
        emit UpdateDevFee(devFee, _devFee);
        devFee = _devFee;
    }

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

    function openLong(uint256 amount, address user, bytes32 referralCode)
        external
        override
        onlyExchange
        nonReentrant
        returns (uint256 totalCost)
    {
        ...
        uint256 tradeCost = amount.mulWadDown(markPrice);
        uint256 fees = orderFee(int256(amount));
        totalCost = tradeCost + fees;

        SUSD.safeTransferFrom(user, address(this), totalCost);

        uint256 hedgingFees = _hedge(int256(amount), false);
        uint256 feesCollected = fees - hedgingFees;
        uint256 externalFee = feesCollected.mulWadDown(devFee);

        SUSD.safeTransfer(feeReceipient, externalFee);

        usedFunds -= int256(tradeCost);
        totalFunds += feesCollected - externalFee;
        ...
    }

[02] VaultToken.setVault FUNCTION IS CALLABLE BY ANYONE, AND DEV TEAM'S VaultToken.setVault TRANSACTION CAN BE FRONTRUN BY MALICIOUS ACTOR

The following VaultToken.setVault function is callable by anyone, and the dev team's VaultToken.setVault transaction can be frontrun by a malicious actor, which can cause vault to be set to an address that is not the deployed KangarooVault contract. When this occurs, calling the KangarooVault contract's functions that further call the VaultToken.mint or VaultToken.burn function would revert due to the onlyVault modifier. Hence, such functionalities of the KangarooVault contract are DOS'ed.

https://github.com/code-423n4/2023-03-polynomial/blob/main/src/VaultToken.sol#L35-L40

    function setVault(address _vault) external {
        if (vault != address(0x0)) {
            revert();
        }
        vault = _vault;
    }

https://github.com/code-423n4/2023-03-polynomial/blob/main/src/VaultToken.sol#L27-L33

    function mint(address _user, uint256 _amt) external onlyVault {
        _mint(_user, _amt);
    }

    function burn(address _user, uint256 _amt) external onlyVault {
        _burn(_user, _amt);
    }

https://github.com/code-423n4/2023-03-polynomial/blob/main/src/VaultToken.sol#L18-L23

    modifier onlyVault() {
        if (msg.sender != vault) {
            revert OnlyVault(address(this), msg.sender, address(vault));
        }
        _;
    }

If users are not promptly notified about this situation, they can interact with the deployed KangarooVault contract, which can cause users' transactions to revert or lose funds unexpectedly. For example, after a user calls the KangarooVault._openPosition function to open a position, another user can call the KangarooVault.initiateDeposit function to queue a deposit request, which transfers SUSD tokens from such user to the deployed KangarooVault contract. Later, calling the KangarooVault.processDepositQueue function will revert because executing VAULT_TOKEN.mint(current.user, tokensToMint) reverts. As a result, such user loses the transferred SUSD tokens, which are locked in the deployed KangarooVault contract.

As a mitigation, the VaultToken.setVault function can be updated to be only callable by trusted admin.

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

    function _openPosition(uint256 amt, uint256 minCost) internal {
        ...
        if (positionData.positionId == 0) {
            positionData.positionId = positionId;
        }
        ...
    }

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

    function initiateDeposit(address user, uint256 amount) external nonReentrant {
        ...
        // Instant processing
        if (positionData.positionId == 0) {
            ...
        } else {
            // Queueing the deposit request
            QueuedDeposit storage newDeposit = depositQueue[nextQueuedDepositId];

            newDeposit.id = nextQueuedDepositId++;
            newDeposit.user = user;
            newDeposit.depositedAmount = amount;
            newDeposit.requestedTime = block.timestamp;

            totalQueuedDeposits += amount;
            emit InitiateDeposit(newDeposit.id, msg.sender, user, amount);
        }

        SUSD.safeTransferFrom(msg.sender, address(this), amount);
    }

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

    function processDepositQueue(uint256 idCount) external nonReentrant {
        uint256 tokenPrice = getTokenPrice();

        for (uint256 i = 0; i < idCount; i++) {
            QueuedDeposit storage current = depositQueue[queuedDepositHead];

            if (current.requestedTime == 0 || block.timestamp < current.requestedTime + minDepositDelay) {
                return;
            }

            uint256 tokensToMint = current.depositedAmount.divWadDown(tokenPrice);

            current.mintedTokens = tokensToMint;
            totalQueuedDeposits -= current.depositedAmount;
            totalFunds += current.depositedAmount;
            VAULT_TOKEN.mint(current.user, tokensToMint);
            ...
        }
    }

[03] ALLOWING ShortCollateral.refresh FUNCTION TO BE CALLABLE BY ANYONE CAN BE DANGEROUS

The following ShortCollateral.refresh function is callable by anyone. If the external synthetix contract is hacked or malfunctions, calling synthetix.synths(key) can return an untrusted address. When this happens, anyone can call ShortCollateral.refresh function to set susd.synth to such untrusted address. As a result, all functionalities that rely on susd.synth will become unreliable and insecure.

As a mitigation, the ShortCollateral.refresh function can be updated to be only callable by trusted admin.

https://github.com/code-423n4/2023-03-polynomial/blob/main/src/ShortCollateral.sol#L64-L70

    function refresh() public {
        ...
        Collateral storage susd = collaterals["sUSD"];
        susd.synth = synthetixAdapter.getSynth("sUSD");
    }

https://github.com/code-423n4/2023-03-polynomial/blob/main/src/SynthetixAdapter.sol#L18-L20

    function getSynth(bytes32 key) public view override returns (address synth) {
        synth = synthetix.synths(key);
    }

[04] SOME FUNCTIONS DO NOT FOLLOW CHECKS-EFFECTS-INTERACTIONS PATTERN

Functions like LiquidityPool.withdraw and ShortCollateral.collectCollateral below transfer the corresponding tokens before updating the relevant states, which do not follow the checks-effects-interactions pattern. In contrast, functions like LiquidityPool.deposit and ShortCollateral.sendCollateral below transfer the corresponding tokens after updating the relevant states. To reduce the potential attack surface and increase the level of security, please consider updating the functions that do not follow the checks-effects-interactions pattern to follow such pattern.

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

    function withdraw(uint256 tokens, address user) external override nonReentrant whenNotPaused("POOL_WITHDRAW") {
        ...
        SUSD.safeTransfer(feeReceipient, fees);
        SUSD.transfer(user, susdToReturn - fees);
        totalFunds -= susdToReturn;
        liquidityToken.burn(msg.sender, tokens);
        ...
    }

https://github.com/code-423n4/2023-03-polynomial/blob/main/src/ShortCollateral.sol#L85-L101

    function collectCollateral(address collateral, uint256 positionId, uint256 amount)
        external
        onlyExchange
        nonReentrant
    {
        ERC20(collateral).safeTransferFrom(address(exchange), address(this), amount);

        UserCollateral storage userCollateral = userCollaterals[positionId];

        if (userCollateral.collateral == address(0x0)) {
            userCollateral.collateral = collateral;
        }

        userCollateral.amount += amount;
        ...
    }

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

    function deposit(uint256 amount, address user) external override nonReentrant whenNotPaused("POOL_DEPOSIT") {
        uint256 tokenPrice = getTokenPrice();
        uint256 fees = amount.mulWadDown(depositFee);
        uint256 amountForTokens = amount - fees;
        uint256 tokensToMint = amountForTokens.divWadDown(tokenPrice);
        liquidityToken.mint(user, tokensToMint);
        totalFunds += amountForTokens;
        SUSD.safeTransferFrom(msg.sender, feeReceipient, fees);
        SUSD.safeTransferFrom(msg.sender, address(this), amountForTokens);

        emit Deposit(user, amount, fees, tokensToMint);
    }

https://github.com/code-423n4/2023-03-polynomial/blob/main/src/ShortCollateral.sol#L106-L116

    function sendCollateral(uint256 positionId, uint256 amount) external override onlyExchange nonReentrant {
        UserCollateral storage userCollateral = userCollaterals[positionId];

        userCollateral.amount -= amount;

        address user = shortToken.ownerOf(positionId);

        ERC20(userCollateral.collateral).safeTransfer(user, amount);

        emit SendCollateral(positionId, userCollateral.collateral, amount);
    }

[05] MISSING address(0) CHECKS FOR CRITICAL ADDRESS INPUTS

( Please note that the following instances are not found in https://gist.github.com/Picodes/c9a016e3df62a3f634ba5d47b49bd1c0#nc-1-missing-checks-for-address0-when-assigning-values-to-address-state-variables. )

To prevent unintended behaviors, critical address inputs should be checked against address(0). address(0) checks are missing for the address input variables in the following constructor and init function. Please consider checking them.

https://github.com/code-423n4/2023-03-polynomial/blob/main/src/SystemManager.sol#L47-L82

    constructor(
        address _addressResolver,
        address _futureMarketManager,
        address _susd,
        bytes32 _baseAsset,
        bytes32 _perpMarketName
    ) Auth(msg.sender, Authority(address(0x0))) {
        addressResolver = IAddressResolver(_addressResolver);
        futuresMarketManager = IFuturesMarketManager(_futureMarketManager);
        ...
        SUSD = ERC20(_susd);
        ...
    }

    function init(
        address _pool,
        address _powerPerp,
        address _exchange,
        address _liquidityToken,
        address _shortToken,
        address _synthetixAdapter,
        address _shortCollateral
    ) public {
        ...
        pool = ILiquidityPool(_pool);
        powerPerp = IPowerPerp(_powerPerp);
        exchange = IExchange(_exchange);
        liquidityToken = ILiquidityToken(_liquidityToken);
        shortToken = IShortToken(_shortToken);
        synthetixAdapter = ISynthetixAdapter(_synthetixAdapter);
        shortCollateral = IShortCollateral(_shortCollateral);
        ...
    }

[06] nonReentrant MODIFIER CAN BE PLACED AND EXECUTED BEFORE OTHER MODIFIERS IN FUNCTIONS

As a best practice, the nonReentrant modifier could be placed and executed before other modifiers in functions to prevent reentrancies through other modifiers and make code more efficient. To follow the best practice, please consider placing the nonReentrant modifier before the requiresAuth modifier in the following functions.

src\KangarooVault.sol
  376: function openPosition(uint256 amt, uint256 minCost) external requiresAuth nonReentrant {    
  383: function closePosition(uint256 amt, uint256 maxCost) external requiresAuth nonReentrant {    
  389: function clearPendingOpenOrders(uint256 maxCost) external requiresAuth nonReentrant {    
  395: function clearPendingCloseOrders(uint256 minCost) external requiresAuth nonReentrant {    
  401: function transferPerpMargin(int256 marginDelta) external requiresAuth nonReentrant {    
  424: function addCollateral(uint256 additionalCollateral) external requiresAuth nonReentrant {    
  436: function removeCollateral(uint256 collateralToRemove) external requiresAuth nonReentrant {    
  450: function executePerpOrders(bytes[] calldata priceUpdateData) external payable requiresAuth nonReentrant {

[07] MISSING REASON STRING IN revert STATEMENT

( Please note that the following instance is not found in https://gist.github.com/Picodes/c9a016e3df62a3f634ba5d47b49bd1c0#nc-2--requirerevertstatements-should-have-descriptive-reason-strings. )

When the reason string is missing in the revert statement, it is unclear about why certain condition reverts. Please add a descriptive reason string for the following revert statement.

https://github.com/code-423n4/2023-03-polynomial/blob/main/src/VaultToken.sol#L35-L40

    function setVault(address _vault) external {
        if (vault != address(0x0)) {
            revert();
        }
        vault = _vault;
    }

[08] REDUNDANT return KEYWORDS IN ShortToken.transferFrom and ShortToken.safeTransferFrom FUNCTIONS

The following ShortToken.transferFrom and ShortToken.safeTransferFrom functions do not have returns but have return statements. Moreover, Solmate's corresponding ERC721.transferFrom and ERC721.safeTransferFrom functions do not return anything. Thus, these ShortToken.transferFrom and ShortToken.safeTransferFrom functions' return keywords are redundant. To improve the code quality, please consider removing the return keywords from these functions.

https://github.com/code-423n4/2023-03-polynomial/blob/main/src/ShortToken.sol#L92-L105

    function transferFrom(address _from, address _to, uint256 _id) public override {
        require(powerPerp.balanceOf(_to) == 0, "Receiver has long positions");
        return ERC721.transferFrom(_from, _to, _id);
    }

    function safeTransferFrom(address _from, address _to, uint256 _id) public override {
        require(powerPerp.balanceOf(_to) == 0, "Receiver has long positions");
        return ERC721.safeTransferFrom(_from, _to, _id);
    }

    function safeTransferFrom(address _from, address _to, uint256 _id, bytes calldata data) public override {
        require(powerPerp.balanceOf(_to) == 0, "Receiver has long positions");
        return ERC721.safeTransferFrom(_from, _to, _id, data);
    }

[09] CONSTANTS CAN BE USED INSTEAD OF MAGIC NUMBERS

To improve readability and maintainability, a constant can be used instead of the magic number. Please consider replacing the magic numbers, such as 1e18, used in the following code with constants.

src\Exchange.sol
  191: fundingRate = fundingRate / 1 days; 
  197: int256 normalizationUpdate = 1e18 - totalFunding;   

src\ShortCollateral.sol
  235: if (safetyRatio > 1e18) return maxDebt; 
  237: maxDebt = position.shortAmount / 2; 

[10] HARDCODED STRING THAT IS REPEATEDLY USED CAN BE REPLACED WITH A CONSTANT

sUSD is repeatedly used in the ShortCollateral contract. For better maintainability, please consider replacing it with a constant.

https://github.com/code-423n4/2023-03-polynomial/blob/main/src/ShortCollateral.sol#L51-L70

    constructor(uint256 susdRatio, uint256 susdLiqRatio, uint256 susdLiqBonus, ISystemManager _systemManager)
        Auth(msg.sender, Authority(address(0x0)))
    {
        ...
        Collateral storage susd = collaterals["sUSD"];
        susd.currencyKey = "sUSD";
        ...
    }

    function refresh() public {
        ...
        Collateral storage susd = collaterals["sUSD"];
        susd.synth = synthetixAdapter.getSynth("sUSD");
    }

[11] ShortToken.adjustPosition FUNCTION DOES NOT NEED TO UPDATE totalShorts and position.shortAmount IN CERTAIN CASE

When calling the following ShortToken.adjustPosition function, if positionId == 0 is false and shortAmount equals position.shortAmount, totalShorts and position.shortAmount will be unchanged. Hence, to increase the code's efficiency, this function can be updated to not update totalShorts and position.shortAmount in this case.

https://github.com/code-423n4/2023-03-polynomial/blob/main/src/ShortToken.sol#L48-L90

    function adjustPosition(
        uint256 positionId,
        address trader,
        address collateral,
        uint256 shortAmount,
        uint256 collateralAmount
    ) external onlyExchange returns (uint256) {
        if (positionId == 0) {
            ...
        } else {
            require(trader == ownerOf(positionId));

            ShortPosition storage position = shortPositions[positionId];

            if (shortAmount >= position.shortAmount) {
                totalShorts += shortAmount - position.shortAmount;
            } else {
                totalShorts -= position.shortAmount - shortAmount;
            }

            position.collateralAmount = collateralAmount;
            position.shortAmount = shortAmount;

            if (position.shortAmount == 0) {
                _burn(positionId);
            }
        }
        ...
    }

[12] LiquidityPool.withdraw FUNCTION CALLS BOTH SUSD.safeTransfer AND SUSD.transfer

The LiquidityPool.withdraw function calls SUSD.safeTransfer(feeReceipient, fees) and SUSD.transfer(user, susdToReturn - fees). For consistency and a higher level of security, the LiquidityPool.withdraw function can be updated to call SUSD.safeTransfer(user, susdToReturn - fees) instead of SUSD.transfer(user, susdToReturn - fees).

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

    function withdraw(uint256 tokens, address user) external override nonReentrant whenNotPaused("POOL_WITHDRAW") {
        ...
        SUSD.safeTransfer(feeReceipient, fees);
        SUSD.transfer(user, susdToReturn - fees);
        ...
    }

[13] LiquidityPool.orderFee FUNCTION CAN CALL getMarkPrice() INSTEAD OF exchange.getMarkPrice()

The following LiquidityPool.orderFee function calls exchange.getMarkPrice() while all other functions in the same contract that need to call exchange.getMarkPrice(), such as the LiquidityPool.getTokenPrice function below, call getMarkPrice() instead. To make code more consistent and better, please consider updating the LiquidityPool.orderFee function to call getMarkPrice() instead of exchange.getMarkPrice().

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

    function orderFee(int256 sizeDelta) public view override returns (uint256) {
        ...
        (uint256 markPrice,) = exchange.getMarkPrice();
        ...
    }

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

    function getTokenPrice() public view override returns (uint256) {
        ...
        (uint256 markPrice, bool isInvalid) = getMarkPrice();
        ...
    }

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();
    }

[14] IMMUTABLES CAN BE NAMED USING SAME CONVENTION

As shown below, some immutables are named using capital letters and underscores while some are not. For a better code quality, please consider naming these immutables using the same naming convention.

src\Exchange.sol
  34: uint256 public immutable override PRICING_CONSTANT;

src\KangarooVault.sol
  60: bytes32 public immutable name;
  63: bytes32 public immutable UNDERLYING_SYNTH_KEY;
  66: ERC20 public immutable SUSD;
  69: IVaultToken public immutable VAULT_TOKEN;
  72: IExchange public immutable EXCHANGE;
  75: ILiquidityPool public immutable LIQUIDITY_POOL;
  78: IPerpsV2Market public immutable PERP_MARKET;

src\LiquidityPool.sol
  56: bytes32 public immutable baseAsset;
  65: ERC20 public immutable SUSD;

src\LiquidityToken.sol
  8: bytes32 public immutable marketKey;
  10: ISystemManager public immutable systemManager;

src\PowerPerp.sol
  9: bytes32 public immutable marketKey;
  10: ISystemManager public immutable systemManager;

src\ShortCollateral.sol
  46: ISystemManager public immutable systemManager;

src\ShortToken.sol
  9: bytes32 public immutable marketKey;
  11: ISystemManager public immutable systemManager;

src\SystemManager.sol
  21: bytes32 public immutable baseAsset;
  24: ERC20 public immutable SUSD;
  27: bytes32 public immutable PERP_MARKET_CONTRACT;

[15] UNUSED IMPORT

The IFuturesMarket interface is not used in the SystemManager contract. Please consider removing the corresponding import statement for better readability and maintainability.

https://github.com/code-423n4/2023-03-polynomial/blob/main/src/SystemManager.sol#L14-L19

import {IFuturesMarket} from "./interfaces/synthetix/IFuturesMarket.sol";
...
contract SystemManager is ISystemManager, Auth {

[16] FLOATING PRAGMAS

It is a best practice to lock pragmas instead of using floating pragmas to ensure that contracts are tested and deployed with the intended compiler version. Accidentally deploying contracts with different compiler versions can lead to unexpected risks and undiscovered bugs. Please consider locking pragmas for the following files.

src\Exchange.sol
  2: pragma solidity ^0.8.9; 

src\KangarooVault.sol
  2: pragma solidity ^0.8.9; 

src\LiquidityPool.sol
  2: pragma solidity ^0.8.9; 

src\LiquidityToken.sol
  2: pragma solidity ^0.8.9; 

src\PowerPerp.sol
  2: pragma solidity ^0.8.9; 

src\ShortCollateral.sol
  2: pragma solidity ^0.8.9; 

src\ShortToken.sol
  2: pragma solidity ^0.8.9; 

src\SynthetixAdapter.sol
  2: pragma solidity ^0.8.9; 

src\SystemManager.sol
  2: pragma solidity ^0.8.9; 

src\libraries\SignedMath.sol
  2: pragma solidity ^0.8.9; 

src\utils\PauseModifier.sol
  3: pragma solidity ^0.8.9; 

[17] SOLIDITY VERSION 0.8.19 CAN BE USED

Using the more updated version of Solidity can enhance security. As described in https://github.com/ethereum/solidity/releases, Version 0.8.19 is the latest version of Solidity, which "contains a fix for a long-standing bug that can result in code that is only used in creation code to also be included in runtime bytecode". To be more secured and more future-proofed, please consider using Version 0.8.19 for all contracts, including the VaultToken contract that uses Version 0.8.9 currently.

https://github.com/code-423n4/2023-03-polynomial/blob/main/src/VaultToken.sol#L2

pragma solidity 0.8.9;

[18] ORDERS OF LAYOUT DO NOT FOLLOW OFFICIAL STYLE GUIDE

https://docs.soliditylang.org/en/v0.8.19/style-guide.html#order-of-layout suggests that the following order should be used in a contract:

1. Type declarations
2. State variables
3. Events
4. Errors
5. Modifiers
6. Functions

Events or error are placed after functions in the following contracts. To follow the official style guide, please consider placing these events or error before all functions in these contracts.

src\ShortCollateral.sol
  16: contract ShortCollateral is IShortCollateral, Auth, ReentrancyGuard {    

src\ShortToken.sol
  8: contract ShortToken is ERC721 {  

src\SystemManager.sol
  19: contract SystemManager is ISystemManager, Auth {  

src\VaultToken.sol
  6: contract VaultToken is ERC20 {  

[19] INCOMPLETE NATSPEC COMMENTS

NatSpec comments provide rich code documentation. The following functions miss the @param and/or @return comments. Please consider completing the NatSpec comments for these functions.

src\Exchange.sol
  87: function openTrade(TradeParams memory tradeParams)  
  155: function getIndexPrice() public view override returns (uint256 indexPrice, bool isInvalid) {    
  186: function getMarkPrice() public view override returns (uint256 markPrice, bool isInvalid) {  
  233: function _openTrade(TradeParams memory params) internal returns (uint256, uint256) {    

src\LiquidityPool.sol
  379: function orderFee(int256 sizeDelta) public view override returns (uint256) {    
  399: function baseAssetPrice() public view override returns (uint256 spotPrice, bool isInvalid) {    
  409: function getSkew() external view override returns (int256) {    
  430: function openLong(uint256 amount, address user, bytes32 referralCode)   
  720: function _getSkew() internal view returns (int256 skew) {   
  727: function _getDelta() internal view returns (uint256 delta) {   
  798: function _hedge(int256 size, bool isLiquidation) internal returns (uint256 hedgingFees) {   

src\ShortCollateral.sol
  121: function liquidate(uint256 positionId, uint256 debt, address user)  
  153: function getMinCollateral(uint256 shortAmount, address collateral)  
  176: function getLiquidationBonus(address collateral, uint256 collateralAmount)  
  192: function canLiquidate(uint256 positionId) public view override returns (bool) { 
  215: function maxLiquidatableDebt(uint256 positionId) public view override returns (uint256 maxDebt) {

[20] MISSING NATSPEC COMMENTS

NatSpec comments provide rich code documentation. The following functions miss NatSpec comments. Please consider adding NatSpec comments for these functions.

src\LiquidityToken.sol
  19: function refresh() public { 
  28: function mint(address _user, uint256 _amt) public onlyPool { 
  32: function burn(address _user, uint256 _amt) public onlyPool { 

src\PowerPerp.sol
  22: function refresh() public { 
  32: function mint(address _user, uint256 _amt) public onlyExchange { 
  36: function burn(address _user, uint256 _amt) public onlyExchange { 
  40: function transfer(address _to, uint256 _amount) public override returns (bool) { 
  45: function transferFrom(address _from, address _to, uint256 _amount) public override returns (bool) { 

src\ShortToken.sol
  33: function refresh() public {
  44: function tokenURI(uint256 tokenId) public view override returns (string memory) { 
  48: function adjustPosition(    
  92: function transferFrom(address _from, address _to, uint256 _id) public override { 
  97: function safeTransferFrom(address _from, address _to, uint256 _id) public override { 
  102: function safeTransferFrom(address _from, address _to, uint256 _id, bytes calldata data) public override { 

src\SynthetixAdapter.sol
  18: function getSynth(bytes32 key) public view override returns (address synth) {   
  22: function getCurrencyKey(address synth) public view override returns (bytes32 key) {   
  26: function getAssetPrice(bytes32 key) public view override returns (uint256, bool) {   

src\SystemManager.sol
  62: function init(  
  84: function refreshSynthetixAddresses() public {  
  89: function setStatusFunction(bytes32 key, bool status) public requiresAuth {

src\VaultToken.sol
  27: function mint(address _user, uint256 _amt) external onlyVault { 
  31: function burn(address _user, uint256 _amt) external onlyVault { 
  35: function setVault(address _vault) external {    

src\libraries\SignedMath.sol
  5: function signedAbs(int256 x) internal pure returns (int256) {   
  9: function abs(int256 x) internal pure returns (uint256) {   
  13: function max(int256 x, int256 y) internal pure returns (int256) {   
  17: function min(int256 x, int256 y) internal pure returns (int256) {