Missing totalFunds update in LiquidityPool's OpenShort(), causing LiquidityPool token holder to lose a portion of their token value

[code423n4]

Lines of code

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

Vulnerability details

The function openShort() in LiquidityPool.sol is missing an update to totalFunds, to increase LiquidityPool funds by the collected net fees.

Impact

As a result of the missing increment to totalFunds, the availableFunds in the LiquidityPool will be lower. This will impact the token price, causing a lower token price on openShort() trades. This will result in LiquidityPool token holders to lose part of their token value.

Detailed Explanation

The function openShort() is supposed to increase the totalFunds by (feesCollected - externalFee) as the trading fees is paid by the trader, via a deduction of the tradeCost.

    totalCost = tradeCost - fees;

    SUSD.safeTransfer(user, totalCost);

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

Proof of Concept

Add the following imports and test case to test/LiquidityPool.Trades.t.sol

import {wadMul} from "solmate/utils/SignedWadMath.sol";
import {IPerpsV2Market} from "../src/interfaces/synthetix/IPerpsV2Market.sol";

function testLiquidityPoolOpenShort() public {
    uint256 amount = 1e18;

    (uint256 markPrice, bool isInvalid) = pool.getMarkPrice();
    uint256 tradeCost = amount.mulWadDown(markPrice);
    uint256 fees = pool.orderFee(int256(amount)); 
    uint256 delta = pool.getDelta();
    int256 hedgingSize = wadMul(int256(amount), int256(delta));
    IPerpsV2Market perp = pool.perpMarket();
    (uint256 hedgingFees, ) = perp.orderFee(hedgingSize, IPerpsV2MarketBaseTypes.OrderType.Delayed);
    uint256 feesCollected = fees - hedgingFees;
    uint256 externalFee = feesCollected.mulWadDown(pool.devFee());

    uint256 totalFundsBefore = pool.totalFunds();
    int256 usedFundsBefore = pool.usedFunds();

    // Open a Short trade
    openShort(amount, amount * 1000, user_1);

    // Calculated expected totalFunds and usedFunds
    uint256 expectedTotalFunds = totalFundsBefore + feesCollected - externalFee;
    uint256 marginRequired = tradeCost + hedgingFees;
    int256 expectedUsedFunds = usedFundsBefore + int256(tradeCost) - int256(hedgingFees) + int256(marginRequired);

    // This is incorrect as LiquidityPool's totalFunds is supposed to increase by net fee (feesCollected - externalFee)
    assertLt(pool.totalFunds(), expectedTotalFunds);

    // LiquidityPool's UsedFunds is also wrong and is higher than expected as it included hedgingFees.
    assertGt(pool.usedFunds(), expectedUsedFunds);

    uint256 poolAvailableFunds = pool.totalFunds() - uint256(pool.usedFunds());
    uint256 expectedAvailableFunds = expectedTotalFunds - uint256(expectedUsedFunds);

    // LiquidityPool's available funds is wrong and is less than expected, as totalFunds is not increased correctly
    assertLt(poolAvailableFunds, expectedAvailableFunds);
    assertEq(poolAvailableFunds, expectedAvailableFunds - hedgingFees);

    // LiquidityPool's available funds is wrong and is also less than SUSD balance 
    assertLt(poolAvailableFunds, susd.balanceOf(address(pool)));

    // LiquidityPool's available fund is expected to be the same as Pool's SUSD balance
    assertEq(expectedAvailableFunds, susd.balanceOf(address(pool)));

    // LiquidityPool Token price is less than expected.
    assertLt(pool.getTokenPrice(), getExpectedTokenPrice(expectedTotalFunds, expectedUsedFunds, perp));

}

function getExpectedTokenPrice(uint256 expectedTotalFunds, int256 expectedUsedFunds, IPerpsV2Market perp) public returns (uint256 expectedTokenPrice) {
    (uint256 markPrice,) = pool.getMarkPrice();
    uint256 totalValue = expectedTotalFunds;
    uint256 totalSupply = lqToken.totalSupply() + pool.totalQueuedWithdrawals();
    uint256 amountOwed = markPrice.mulWadDown(powerPerp.totalSupply());
    uint256 amountToCollect = markPrice.mulWadDown(shortToken.totalShorts());
    //uint256 totalMargin = _getTotalMargin();

    (uint256 totalMargin,) = perp.remainingMargin(address(pool));

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

    expectedTokenPrice = totalValue.divWadDown(totalSupply);
}

Recommended Mitigation Steps

Add the following to update totalFunds with the net fee collection.

    totalFunds += feesCollected - externalFee; 

[c4-judge]

JustDravee marked the issue as primary issue

[c4-judge]

JustDravee marked the issue as satisfactory

[rivalq]

fee part is included in usedFunds, At any time pool's total funds is not just included in totalFunds, but some part of it is in usedFunds, note that usedFunds can be negative too.

[c4-sponsor]

rivalq marked the issue as sponsor disputed

[c4-sponsor]

rivalq requested judge review

[JustDravee]

fee part is included in usedFunds, At any time pool's total funds is not just included in totalFunds, but some part of it is in usedFunds, note that usedFunds can be negative too.

As this issue was raised by several wardens, I'm willing to give this the benefit of the doubt and would like to ask the sponsor @rivalq to view it a second time. Perhaps looking through duplicated issues? They are mostly low quality and badly explained but might be on to something. https://github.com/code-423n4/2023-03-polynomial-findings/issues/46 is the duplicate with the most arguments

I'd like to use this issue to bring attention to another issue, https://github.com/code-423n4/2023-03-polynomial-findings/issues/117, as it actually says the opposite (that openShort is done right but closeLong has an extra update that shouldn't exist). So, this one, which I repeat isn't a duplicate but actually an opposite finding, might be right.

[c4-sponsor]

mubaris marked the issue as sponsor confirmed

[mubaris]

I'm confirming this from our side, although I think it's a Medium risk similar to #117

[c4-judge]

JustDravee marked the issue as selected for report

[JustDravee]

As this is still considered a loss of funds for users, I'll keep it as high