Polynomial Protocol contest - KIntern_NA's results

Lines of code

https://github.com/code-423n4/2023-03-polynomial/blob/main/src/LiquidityPool.sol#L561 https://github.com/code-423n4/2023-03-polynomial/blob/main/src/Exchange.sol#L347-L350

Vulnerability details

Impact

Hedging will not work as expected, and LiquidityPool will lose funds without expectation.

Proof of concept

1. When a short position is liquidated in contract Exchange, function _liquidate will be triggered. It will burn the power perp tokens and reduce the short position amount accordingly.

function _liquidate(uint256 positionId, uint256 debtRepaying) internal {
    ...
    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); 
    ...

2. As you can see, it will decrease the size of short position by debtRepaying, and burn debtRepaying power perp tokens. Because of the same amount, the skew of LiquidityPool will not change.
3. Howerver, pool.liquidate will be called, and LiquidityPool will be hedged with debtRepaying amount.

function liquidate(uint256 amount) external override onlyExchange nonReentrant {
    (uint256 markPrice, bool isInvalid) = getMarkPrice();
    require(!isInvalid);

    uint256 hedgingFees = _hedge(int256(amount), true);
    usedFunds += int256(hedgingFees);

    emit Liquidate(markPrice, amount);
}

4. Therefore, LiquidityPool will be hedged more than it needs, and the position of LiquidityPool in the Perp Market will be incorrect (compared with what it should be for hedging).

Tool used

Manual Review

Recommended Mitigation Steps

Should not hedge the LiquidityPool during liquidation.

Lines of code

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

Vulnerability details

Impact

KangarooVault can't get back its collateral from Exchange contract, then it loses a lot funds.

Proof of concept

Function KangarooVault.removeCollateral doesn't call Exchange.removeCollateral():

///url = 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);
}

Testing: Place the following testing function into test/KangarooVault.t.sol, and run forge test --match-test testRemoveCollateralLost

function testRemoveCollateralLost() public {
    uint256 amt = 1e18;
    uint256 collDelta = 1000e18;

    kangaroo.openPosition(amt, 0);
    skip(100);
    kangaroo.executePerpOrders(emptyData);
    kangaroo.clearPendingOpenOrders(0);

    uint256 initialBalance = susd.balanceOf(address(kangaroo));

    kangaroo.addCollateral(collDelta);
    uint256 balance1 = susd.balanceOf(address(kangaroo));

    kangaroo.removeCollateral(collDelta);
    uint256 balance2 = susd.balanceOf(address(kangaroo));

    //kangaroo vault lost collateral tokens
    assertEq(balance2, balance1); 
    assertEq(balance2, initialBalance - collDelta);
}

Tool used

Foundry

Recommended Mitigation Steps

Call Exchange.removeCollateral() in function KangarooVault.removeCollateral to regain the collateral tokens.

function testRemoveCollateralLost() public {
    uint256 amt = 1e18;
    uint256 collDelta = 1000e18;

    kangaroo.openPosition(amt, 0);
    skip(100);
    kangaroo.executePerpOrders(emptyData);
    kangaroo.clearPendingOpenOrders(0);

    uint256 initialBalance = susd.balanceOf(address(kangaroo));

    kangaroo.addCollateral(collDelta);
    uint256 balance1 = susd.balanceOf(address(kangaroo));

    kangaroo.removeCollateral(collDelta);
    uint256 balance2 = susd.balanceOf(address(kangaroo));

    //kangaroo vault lost collateral tokens
    assertEq(balance2, balance1); 
    assertEq(balance2, initialBalance - collDelta);
}

Lines of code

https://github.com/code-423n4/2023-03-polynomial/blob/main/src/LiquidityPool.sol#L452 https://github.com/code-423n4/2023-03-polynomial/blob/main/src/LiquidityPool.sol#L484 https://github.com/code-423n4/2023-03-polynomial/blob/main/src/LiquidityPool.sol#L516 https://github.com/code-423n4/2023-03-polynomial/blob/main/src/LiquidityPool.sol#L549 https://github.com/code-423n4/2023-03-polynomial/blob/main/src/LiquidityPool.sol#L562

Vulnerability details

Impact

usedFunds of LiquidityPool is calculated incorrectly

Proof of concept

usedFunds tracks the funds utilitized from the pool. Let's see how usedFunds is accumulated when opening a long position:

function openLong(uint256 amount, address user, bytes32 referralCode)
    external
    override
    onlyExchange
    nonReentrant
    returns (uint256 totalCost)
{
    (uint256 markPrice, bool isInvalid) = getMarkPrice();
    require(!isInvalid);

    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;

    emit RegisterTrade(referralCode, feesCollected, externalFee);
    emit OpenLong(markPrice, amount, fees);
}

usedFunds is still updated in function _hedge:

function _hedge(int256 size, bool isLiquidation) internal returns (uint256 hedgingFees) {
    bool isInvalid;

    uint256 delta = _getDelta();
    int256 hedgingSize = wadMul(size, int256(delta));

    (hedgingFees, isInvalid) = perpMarket.orderFee(hedgingSize, IPerpsV2MarketBaseTypes.OrderType.Delayed);
    require(!isInvalid);

    uint256 marginRequired = _calculateMargin(hedgingSize) + hedgingFees;
    usedFunds += int256(marginRequired);
    require(usedFunds <= 0 || totalFunds >= uint256(usedFunds));

    perpMarket.transferMargin(int256(marginRequired));

    emit TransferMargin(marginRequired);

    _placeDelayedOrder(hedgingSize, isLiquidation);
}

1. marginRequired in function _hedge includes the hedgingFees. Then usedFunds is updated correctly in function _hedge, that it increases the amount of SUSD tokens which is sent to PerpMarket.
2. In function openLong, the pool claims totalCost of SUSD tokens from user. And it only transfer externalFee outside of contract. Then usedFunds should be updated in function openLong as the following:

usedFunds -= int256(totalCost) - externalFee;

Similarly, all functions closeLong, openShort, closeShort and liquidate update usedFunds incorrectly. It shoule be updated as the following:

• Function closeLong:

usedFunds += int256(totalCost) + externalFee;

• Function openShort:

usedFunds += int256(totalCost) + externalFee;

• Function closeShort:

usedFunds -= int256(totalCost) - externalFee;

• Function liquidate: remove usedFunds += int256(hedgingFees);

Tool used

Manual Review

Recommended Mitigation Steps

Should follow the above mitigation to fix the calculation of usedFunds

Lines of code

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

Vulnerability details

Impact

Function hedgePositions is incorrect, leads to the hedging will not work as expected, and LiquidityPool can lose funds without expectation.

Proof of concept

Let's see function hedgePositions in LiquidtyPool contract:

function hedgePositions() external override requiresAuth nonReentrant {
    int256 currentPosition = _getTotalPerpPosition();
    int256 skew = _getSkew();
    uint256 delta = _getDelta();
    int256 requiredPosition = wadMul(skew, int256(delta));

    int256 newPosition = requiredPosition - currentPosition;
    int256 marginDelta = int256(_calculateMargin(newPosition));

    if (requiredPosition.abs() < currentPosition.abs()) {
        marginDelta = -marginDelta;
    }

    usedFunds += marginDelta;

    perpMarket.transferMargin(marginDelta);
    _placeDelayedOrder(newPosition, false);

    emit HedgePositions(currentPosition, requiredPosition, marginDelta);
}

currentPosition is the sum of: the current position size of Liquidity Pool in Synthetix and the delta size of the current delayed order which was submitted into Synthetix perp market.

///url = https://github.com/code-423n4/2023-03-polynomial/blob/main/src/LiquidityPool.sol#L738-L743
function _getTotalPerpPosition() internal view returns (int256 positionSize) {
    IPerpsV2MarketBaseTypes.Position memory position = perpMarket.positions(address(this));
    IPerpsV2MarketBaseTypes.DelayedOrder memory delayedOrder = perpMarket.delayedOrders(address(this));

    positionSize = position.size + delayedOrder.sizeDelta;
}

However, currentPosition missed the variable queuedPerpSize, is the total amount of pending size delta (waiting to be submitted). Then _placeDelayedOrder will be called with the wrong newPosition, leads to the position size of pool can get a large deviation. The hedging will not be safe anymore.

Scenerio:

• _getTotalPerpPosition = 0, requiredPosition = 1000, queuedPerpSize = 1000
• newPosition is calculated incorrectly to be 1000 (since it missed queuedPerpSize)
• It calls _placeDelayedOrder(1000, false), then queuedPerpSize increase to be 2000
• After executing all delayed orders, position size of LiquidityPool = 2000 (incorrect hedging)
• newPosition should be -1000 in this case

Tool used

Manual Review

Recommended Mitigation Steps

currentPosition should be _getTotalPerpPosition() + queuedPerpSize in function hedgePositions

Lines of code

https://github.com/code-423n4/2023-03-polynomial/blob/main/src/Exchange.sol#L409-L424 https://github.com/code-423n4/2023-03-polynomial/blob/main/src/Exchange.sol#L190-L198

Vulnerability details

Impact

normalizationFactor in Exchange contract is calculated incorrectly, which makes it unable to change. Then hedging of the liquidity pool will be incorrect, and protocol might lose a lot.

Proof of concept

The fundingRate of Exchange contract has a bound of maxFunding. And maxFunding is initialized as 1e16.

///url = https://github.com/code-423n4/2023-03-polynomial/blob/main/src/Exchange.sol#L178-L181
int256 maxFunding = int256(maxFundingRate);

// Apply bounds to funding rate
fundingRate = normalizedSkew.min(maxFunding).max(-maxFunding);

///url = https://github.com/code-423n4/2023-03-polynomial/blob/main/src/Exchange.sol#L62
uint256 public maxFundingRate = 1e16;

Let's see the normalizationFactor calculation in function _updateFundingRate:

///url = https://github.com/code-423n4/2023-03-polynomial/blob/main/src/Exchange.sol#L409-L424
function _updateFundingRate() internal {
    (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;

    normalizationFactor = normalizationFactor.mulWadDown(uint256(normalizationUpdate));

    emit UpdateFundingRate(fundingLastUpdated, normalizationFactor);

    fundingLastUpdated = block.timestamp;
}

Let's call the initialFundingRate is the result of getFundingRate(). You can see that:

totalFunding = wadMul(initialFundingRate / 1 days, (currentTimeStamp - fundingLastUpdatedTimestamp))

Since initialFundingRate is very small compared to 1e18, the results of this wadMul function always be 0 if passed time is smaller than 1 day. Because the multiplication will be divided by 1e18 in wadMul. It makes the totalFunding to be 0 in most cases (passed time < 1 days), then normalizationFactor never changes. The hedging of the liquidity pool will be incorrect after that.

Testing: Place this testing function into test/Exchange.Simple.t.sol, and run forge test --match-test testNormalizationFactorNeverChanges

function testNormalizationFactorNeverChanges() public {
    uint256 currentTime = block.timestamp;
    exchange.setMaxFundingRate(2e16);
    uint256 initialNormalizationFactor = exchange.normalizationFactor();

    openLong(1e18, 1000e18, user_1);
    vm.warp(currentTime + 3000);

    (int256 fundingRate_,) = exchange.getFundingRate();
    assertEq(initialNormalizationFactor, exchange.normalizationFactor());
}

Tool used

Foundry

Recommended Mitigation Steps

Should replace the division fundingRate / 1 days by wadDiv as the following:

function _updateFundingRate() internal {
    (int256 fundingRate,) = getFundingRate();
    fundingRate = wadDiv(fundingRate, 1 days);
    ...

function testNormalizationFactorNeverChanges() public {
    // Init
    exchange.setMaxFundingRate(2e16);
    uint256 initialNormalizationFactor = exchange.normalizationFactor();
    (int256 initialFundingRate,) = exchange.getFundingRate();

    // Time skip 10 days and opening trade to call _updateFundingRate (fundingRate increases)
    skip(10 days);
    openLong(1e18, 1000e18, user_1);
    (int256 fundingRate2,) = exchange.getFundingRate();
    assertLt(initialFundingRate, fundingRate2);

    // Time skip 300 days and opening trade to call _updateFundingRate (fundingRate increases)
    skip(300 days);
    openLong(1e18, 1000e18, user_1);
    (int256 finalFundingRate,) = exchange.getFundingRate();
    assertLt(fundingRate2, finalFundingRate);

    // Comparing normalization factor to its initial value
    assertEq(initialNormalizationFactor, exchange.normalizationFactor());
}

function _updateFundingRate() internal {
    (int256 fundingRate,) = getFundingRate();
    fundingRate = wadDiv(fundingRate, 1 days);
    ...

Lines of code

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

Vulnerability details

Impact

The invalid remainingMargin can be used in contract KangarooVault, leads to the functions can be incorrect.

Proof of concept

There is no check if the PERP_MARKET.remainingMargin() is valid valid in function _resetTrade():

function _resetTrade() internal {
        positionData.positionId = 0;
        (uint256 totalMargin,) = PERP_MARKET.remainingMargin(address(this));

Therefore, the invalid PERP_MARKET.remainingMargin() can be used.

Tool used

Manual Review

Recommended Mitigation Steps

Should add the checks if PERP_MARKET.remainingMargin() is valid.

Lines of code

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

Vulnerability details

Impact

KangarooVault contract can't close the trades and users can't withdraw from it, then users and KangarooVaults will lose a lot of funds.

Proof of concept

1. In contract KangarooVault, usedFunds is the uint256 variable which tracks the funds utilitized from the vault. And totalFunds is the uin256 variable which tracks the funds claimed by vault from profits and users' depositing.
2. Contract KangarooVault has no check if totalFunds >= usedFunds when usedFunds is increased (transfer from vault) or totalFunds is decreased (transfer to vault).
3. If someone transfers funds directly into the vault, usedFunds can be greater than totalFunds because the vault can transfer out more than totalFunds.
4. When usedFunds > totalFunds, KangarooVault can not close its trades because it will revert on underflow in function _resetTrade:

function _resetTrade() internal {
        ...
        totalFunds -= usedFunds;
        ...
    }

5. When usedFunds > totalFunds, user can't not withdraw by function processWithdrawalQueue because it will revert on underflow.

function processWithdrawalQueue(uint256 idCount) external nonReentrant {
    for (uint256 i = 0; i < idCount; i++) {
        ...

        uint256 availableFunds = totalFunds - usedFunds;

        ...

Scenerio:

• A deposit 1000 SUSD, then totalFunds = 1000 SUSD, usedFunds = 0
• B transfer directly 1000 SUSD to the KangarooVault, that doesn't change totalFunds and usedFunds
• KangarooVault opens a short position and uses 2000 SUSD to increase the margin, then usedFunds = 2000
• After that, usedFunds > totalFunds (2000 > 1000) then KangarooVault can't close its position

Tool used

Manual Review

Recommended Mitigation Steps

Should add the checks if totalFunds >= usedFunds when increasing usedfunds or decreasing totalFunds in contract KangarooVault.sol

Lines of code

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

Vulnerability details

Impact

The Liquidity Pool can lack the margin of PerpMarket if liquidationBufferRatio of contract PerpsV2MarketSettings from Synthetix is greater than 1e18. Then the delay orders of the pool can not be executed and the position of the pool might be liquidated.

Proof of concept

• Function _calculateMargin returns the margin amount needed to transfer with the specific size of PerpMarket.

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

• When LiquidityPool executes a delayed order of Synthetix, function _updatePositionMargin from PerpsV2Market of Synthetix will be called. It will check the margin of the new position in the perps market:

uint liqPremium = _liquidationPremium(position.size, price);
uint liqMargin = _liquidationMargin(position.size, price).add(liqPremium);
_revertIfError(
    (margin < _minInitialMargin()) ||
        (margin <= liqMargin) ||
        (_maxLeverage(_marketKey()) < _abs(_currentLeverage(position, price, margin))),
    Status.InsufficientMargin
);

• Function _liquidationMargin returns the maximum of margin that will be liquidated with the position size of perps market. Then the new margin must to be greater than _liquidationMargin.

 function _liquidationMargin(int positionSize, uint price) internal view returns (uint lMargin) {
    uint liquidationBuffer = _abs(positionSize).multiplyDecimal(price).multiplyDecimal(_liquidationBufferRatio());
    return liquidationBuffer.add(_liquidationFee(positionSize, price));
}

• To calculate the _liquidationMargin, PerpsV2Martket use the variable _liquidationBufferRatio as the scale of _abs(positionSize).multiplyDecimal(price) (value of the position). This variable has getter and setter functions in the contract PerpsV2MarketSettings. You can find this contract at https://optimistic.etherscan.io/address/0x09793Aad1518B8d8CC72FDd356479E3CBa7B4Ad1#code.
• _liquidationBufferRatio is 1e16 (1%) now but can be changed in the future, and can become market-specific (I asked Synthetix team and they said it will be changed in a couple of weeks, but I didn't know how it will be changed).
• Since function _calculateMargin in contract LiquidityPool doesn't consider this minimum required margin (to not be liquidated), LiquidityPool can lack the margin of perps market in the future.
• Scenario:

1. futuresLeverage of LiquidityPool is applied to be 5.Then function _calculateMargin returns 1/5 (20%) amount of position value (position value = size * spotPrice)
2. _liquidationBufferRatio is set to be 3e17 (30%) in PerpsV2MarketSettings contract. Then it requires a margin >= 30% of the position value when updating a position.
3. LiquidityPool's margin is not enough for its position in PerpsMarket. Then the delay orders of the pool can't be executed and the position of the pool in PerpsMarket can be liquidated

Tool used

Manual Review

Recommended Mitigation Steps

Should calculate _liquidationMargin from PerpsMarket using the current _liquidationBufferRatio from PerpsV2MarketSettings contract, to set the minimum margin in function _calculateMargin

Lines of code

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

Vulnerability details

Impact

The attacker can post-running attack to keep the LiquidityPool's can't submit the orders of perpetual for hedging. It leads to every trade of the pool will not be hedged anymore.

Proof of concept

1. LiquidityPool calls function _hedge every trades, and it triggers function _placeDelayedOrder.

function _placeDelayedOrder(int256 sizeDelta, bool isLiquidation) internal {
    IPerpsV2MarketBaseTypes.DelayedOrder memory order = perpMarket.delayedOrders(address(this));

    (,,,,, IPerpsV2MarketBaseTypes.Status status) =
        perpMarket.postTradeDetails(sizeDelta, 0, IPerpsV2MarketBaseTypes.OrderType.Delayed, address(this));

    int256 oldSize = order.sizeDelta;
    if (oldSize != 0 || isLiquidation || uint8(status) != 0) {
        queuedPerpSize += sizeDelta;
        return;
    }
    perpMarket.submitOffchainDelayedOrderWithTracking(sizeDelta, perpPriceImpactDelta, synthetixTrackingCode);

    emit SubmitDelayedOrder(sizeDelta);
}

2. In this function, if (oldSize != 0 || isLiquidation || uint8(status) != 0), the pool will accumulate the variable queuedPerpSize and return. Else, the pool will submit a delay order of sizeDelta (the current size delta of the trade) to the Synthetix perpetual market.
3. The current delay order of Pool will be executed by function executePerpOrders . After that, the order size of the pool will return to 0 and the pool can submit the other delayed order.
4. However, when the pool can submit a new order, function _placeDelayedOrder just submit the order with sizeDelta of the current trade. And the order of queuedPerpSize can only submitted in function placeQueuedOrder, but it require the current order size of pool is 0:

function placeQueuedOrder() external requiresAuth nonReentrant {
    IPerpsV2MarketBaseTypes.DelayedOrder memory order = perpMarket.delayedOrders(address(this));

    require(order.sizeDelta == 0);

5. Therefore, after the executePerpOrders call from the authority, attacker can post-run opening/closing a position, to trigger function _placeDelayedOrder, and make the pool submit the order of the current sizeDelta. Then the order size of the pool will be different from 0, and the pool can't submit other delay orders, until the next executePerpOrders call. And all the sizes of trades after that will be accumulated into queuedPerpSize.
6. Attacker can repeat post-running function executePerpOrders with a small trade, to keep the pool can't submit the necessary order (with queuedPerpSize) for hedging. Then every trade will not be hedged.

Tool used

Manual Review

Recommended Mitigation Steps

Function _placeDelayedOrder should submit the order of queuedPerpSize + sizeDelta instead of sizeDelta. You can take a look at the following example:

function _placeDelayedOrder(int256 sizeDelta, bool isLiquidation) internal {
    IPerpsV2MarketBaseTypes.DelayedOrder memory order = perpMarket.delayedOrders(address(this));

    (,,,,, IPerpsV2MarketBaseTypes.Status status) =
        perpMarket.postTradeDetails(queuedPerpSize + sizeDelta, 0, IPerpsV2MarketBaseTypes.OrderType.Delayed, address(this));

    int256 oldSize = order.sizeDelta;
    if (oldSize != 0 || isLiquidation || uint8(status) != 0) {
        queuedPerpSize += sizeDelta;
        return;
    }
    perpMarket.submitOffchainDelayedOrderWithTracking(queuedPerpSize + sizeDelta, perpPriceImpactDelta, synthetixTrackingCode);

    emit SubmitDelayedOrder(sizeDelta);
}

Lines of code

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

Vulnerability details

Impact

Contract LiquidityPool will transfer margin of size delta for every trade, and this margin is always > 0. Then attacker can repeat open and close a position in 1 transaction, to make the pool transfer all its SUSD tokens to Synthetix perpetual market as margin, even the perpetual size of LiquidityPool will not change after that. Then many actions of users which need SUSD of the pool such as withdrawing liquidity tokens, opening short positions, closing long positions... can't be executed until the pool is rebalanced by the authority.

Proof of concept

Let's take a look at function _hedge in contract LiquidityPool:

/// url = https://github.com/code-423n4/2023-03-polynomial/blob/main/src/LiquidityPool.sol#L807-L811
function _hedge(int256 size, bool isLiquidation) internal returns (uint256 hedgingFees) {
    ...
    uint256 marginRequired = _calculateMargin(hedgingSize) + hedgingFees;
    usedFunds += int256(marginRequired);
    require(usedFunds <= 0 || totalFunds >= uint256(usedFunds));

    perpMarket.transferMargin(int256(marginRequired));
    ...

In this function, marginRequired is always > 0, since it is the required margin for the independant hedgingSize. Even the size of pool's perpetual position increases or decreases (sometimes it doesn't need to transfer more positive margin), it always transfer this amount of SUSD as margin to Synthetix perpetual market. Then attacker can make the pool transfer all its SUSD tokens as margin by repeating open and close a position, although it will not change the pool's perpetual size after that. It leads to users' actions can be broken because of the lack of SUSD in LiquidityPool, until the pool is rebalanced by the authority. Furthermore, the attacker can front-run to break the actions of important specific users.

Tool used

Manual Review

Recommended Mitigation Steps

Calculate marginRequired (can be positive or negative) from the new perpetual size and the remaining margin. I advise you to use the similar calculation from the function rebalanceMargin.

Lines of code

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

Vulnerability details

Impact

A collateral token of the protocol might collapse in the future, or the depeg of stable coin might happen (example the USDC depeg > 10% on March 11th 2023). However, shortCollateral contract doesn't have a way to unapprove this collateral token.

Proof of concept

Function approveCollateral is used to approve a new collateral token. The variable collateral.isApproved is setted to be true. But there is no way to set collateral.isApproved to be false in ShortCollateral contract. It means all aproved collateral can't be unapproved.

Tool used

Manual Review

Recommended Mitigation Steps

Should have a way to unapprove collateral tokens in ShortCollateral contract