ether_sky - There may be excess funds in the PnL pool or bad debt due to the funding fee.

[sherlock-admin3]

ether_sky

medium

There may be excess funds in the PnL pool or bad debt due to the funding fee.

Summary

There are two types of makers: OracleMaker and SpotHedgeBaseMaker, where LPs can deposit funds. Traders can then execute their orders against these makers. To incentivize LPs, several mechanisms exist for these makers to profit. One is the borrowing fee, which both makers can benefit from. Another is the funding fee, which specifically benefits OracleMaker. The funding fee incentivizes users to maintain positions with the same direction of OracleMaker. However, due to the funding fee, there may be excess funds in the PnL pool or occurrences of bad debt.

Vulnerability Detail

Typically, in most protocols, the generated yields are totally distributed to users and the protocol itself. In the Perpetual protocol, all borrowing fees from payers are solely distributed to receivers, which are whitelisted by the protocol. However, not all funding fees are distributed, or there may be a lack of funding fees available for distribution. The current funding rate is determined based on the current position of the base pool(OracleMaker).

function getCurrentFundingRate(uint256 marketId) public view returns (int256) {
    uint256 fundingRateAbs = FixedPointMathLib.fullMulDiv(
        fundingConfig.fundingFactor,
        FixedPointMathLib
            .powWad(openNotionalAbs.toInt256(), fundingConfig.fundingExponentFactor.toInt256())
            .toUint256(),
        maxCapacity
    );
}

Holders of positions with the same direction of the position of the OracleMaker receive funding fees, while those with positions in the opposite direction are required to pay funding fees. The amount of funding fees generated per second is calculated as the product of the funding rate and the sum of openNotionals of positions with the opposite direction. Conversely, the amount of funding fees distributed per second is calculated as the product of the funding rate and the sum of openNotionls of positions with the same direction of the position of the OracleMaker.

function getPendingFee(uint256 marketId, address trader) public view returns (int256) {
    int256 fundingRate = getCurrentFundingRate(marketId);
    int256 fundingGrowthLongIndex = _getFundingFeeStorage().fundingGrowthLongIndexMap[marketId] +
        (fundingRate * int256(block.timestamp - _getFundingFeeStorage().lastUpdatedTimestampMap[marketId]));
    int256 openNotional = _getVault().getOpenNotional(marketId, trader);
    int256 fundingFee = 0;
    if (openNotional != 0) {
        fundingFee = _calcFundingFee(
            openNotional,
            fundingGrowthLongIndex - _getFundingFeeStorage().lastFundingGrowthLongIndexMap[marketId][trader]
        );   // @audit, here
    }
    return fundingFee;
}

All orders are settled against makers, meaning for every long position, there should be an equivalent short position. While we might expect the sum of openNotionals of long positions to be equal to the openNotionals of short positions, in reality, they may differ.

Suppose there are two long positions with openNotional values of S and S/2. Then there should be two short positions with openNotianal values of -S and -S/2. If the holder of the first long position cancels his order against the second short position with -S/2, the openNotional of the long position becomes 0, and the second short position becomes a long position. However, we can not be certain that the openNotional of the new long position is exactly S/2.

function add(Position storage self, int256 positionSizeDelta, int256 openNotionalDelta) internal returns (int256) {
    int256 openNotional = self.openNotional;
    int256 positionSize = self.positionSize;

    bool isLong = positionSizeDelta > 0;
    int256 realizedPnl = 0;

    // new or increase position
    if (positionSize == 0 || (positionSize > 0 && isLong) || (positionSize < 0 && !isLong)) {
        // no old pos size = new position
        // direction is same as old pos = increase position
    } else {
        // openNotionalDelta and oldOpenNotional have different signs = reduce, close or reverse position
        // check if it's reduce or close by comparing absolute position size
        // if reduce
        // realizedPnl = oldOpenNotional * closedRatio + openNotionalDelta
        // closedRatio = positionSizeDeltaAbs / positionSizeAbs
        // if close and increase reverse position
        // realizedPnl = oldOpenNotional + openNotionalDelta * closedPositionSize / positionSizeDelta
        uint256 positionSizeDeltaAbs = positionSizeDelta.abs();
        uint256 positionSizeAbs = positionSize.abs();

        if (positionSizeAbs >= positionSizeDeltaAbs) {
            // reduce or close position
            int256 reducedOpenNotional = (openNotional * positionSizeDeltaAbs.toInt256()) /
                positionSizeAbs.toInt256();
            realizedPnl = reducedOpenNotional + openNotionalDelta;
        } else {
            // open reverse position
            realizedPnl =
                openNotional +
                (openNotionalDelta * positionSizeAbs.toInt256()) /
                positionSizeDeltaAbs.toInt256();
        }
    }

    self.positionSize += positionSizeDelta;
    self.openNotional += openNotionalDelta - realizedPnl;

    return realizedPnl;
}

Indeed, the openNotional of the new long position is determined by the current price. Consequently, while the position size of this new long position will be the same with the old second long position with an openNotional value of S/2, the openNotional of the new long position can indeed vary from S/2. As a result, the sum of openNotionals of short positions can differ from the sum of long positions. There are numerous other scenarios where the sums of openNotionals may vary.

I believe that the developers also thought that the funding fees are totally used between it's payers and receivers from the below code.

/// @notice positive -> pay funding fee -> fundingFee should round up
/// negative -> receive funding fee -> -fundingFee should round down
function _calcFundingFee(int256 openNotional, int256 deltaGrowthIndex) internal pure returns (int256) {
    if (openNotional * deltaGrowthIndex > 0) {
        return int256(FixedPointMathLib.fullMulDivUp(openNotional.abs(), deltaGrowthIndex.abs(), WAD));
    } else {
        return (openNotional * deltaGrowthIndex) / WAD.toInt256();
    }
}

They even took rounding into serious consideration to prevent any shortfall of funding fees for distribution.

Impact

Excess funding fees in the PnL pool can arise when the sum of openNotionals of the payers exceeds that of the receivers. Conversely, bad debt may occur in other cases, leading to a situation where users are unable to receive their funding fees due to an insufficient PnL pool. It is worth to note that other yields, such as the borrowing fee, are entirely utilized between it's payers and receivers. Therefore, there are no additional funding sources available to address any shortages of funding fees.

Code Snippet

https://github.com/sherlock-audit/2024-02-perpetual/blob/main/perp-contract-v3/src/fundingFee/FundingFee.sol#L133-L139 https://github.com/sherlock-audit/2024-02-perpetual/blob/main/perp-contract-v3/src/fundingFee/FundingFee.sol#L89-L102 https://github.com/sherlock-audit/2024-02-perpetual/blob/main/perp-contract-v3/src/vault/LibPosition.sol#L45-L48 https://github.com/sherlock-audit/2024-02-perpetual/blob/main/perp-contract-v3/src/fundingFee/FundingFee.sol#L183-L191

Tool used

Manual Review

Recommendation

We can calculate funding fees based on the position size because the sum of the position sizes of long positions will always be equal to the sum of short positions in all cases.

[sherlock-admin4]

2 comment(s) were left on this issue during the judging contest.

santipu_ commented:

Medium

takarez commented:

valid; medium(8)

[vinta]

Confirmed, this is valid. Thanks for finding this bug!

We're still figuring out a fix, or we might probably just disable fundingFee entirely if we cannot solve it before launch.

[etherSky111]

Escalate

I think this is a high risk issue. This has a high likelihood of occurrence, as it can happen easily. The impact is also high due to the absence of available funding sources in case of bad debt. Consequently, users' margins and borrowing fees will be reduced.

And the sponsor has confirmed their intention to disable this feature if a suitable solution is not found. This shows the severity of the impact.

Anyway, this is my first contest in Sherlock and am not familiar with the rules. I hope for a kind review. Thanks in advance.

[sherlock-admin2]

Escalate

I think this is a high risk issue. This has a high likelihood of occurrence, as it can happen easily. The impact is also high due to the absence of available funding sources in case of bad debt. Consequently, users' margins and borrowing fees will be reduced.

And the sponsor has confirmed their intention to disable this feature if a suitable solution is not found. This shows the severity of the impact.

Anyway, this is my first contest in Sherlock and am not familiar with the rules. I hope for a kind review. Thanks in advance.

The escalation could not be created because you are not exceeding the escalation threshold.

You can view the required number of additional valid issues/judging contest payouts in your Profile page, in the Sherlock webapp.

[etherSky111]

Hi @nevillehuang , thanks for your judging.

I need assistance with escalation, as I couldn't create it. What do I need to do for this?

Thanks.

[GTH1235]

Escalate

I think this is a high risk issue. This has a high likelihood of occurrence, as it can happen easily. The impact is also high due to the absence of available funding sources in case of bad debt. Consequently, users' margins and borrowing fees will be reduced.

And the sponsor has confirmed their intention to disable this feature if a suitable solution is not found. This shows the severity of the impact.

[sherlock-admin2]

Escalate

I think this is a high risk issue. This has a high likelihood of occurrence, as it can happen easily. The impact is also high due to the absence of available funding sources in case of bad debt. Consequently, users' margins and borrowing fees will be reduced.

And the sponsor has confirmed their intention to disable this feature if a suitable solution is not found. This shows the severity of the impact.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[IllIllI000]

There may be meaning it won't necessarily happen (i.e. Medium). The funding fee does not cause bad debt, it just temporarily causes there to be fewer funds in the pool than expected. As soon as the funding flips to the other side, it's likely to be remedied without any intervention at all (as is indicated by the fact that the title begins with There may be excess funds). The readme says PnL Pool currently starts empty, so a trader with realized gains may experience temporary illiquidity when he tries to withdraw those gains. This inconvenience will be mitigated in the future with a buffer balance so the pool is expected to have temporary liquidity problems for traders, which are expected to be mitigated. There is no bad debt being created here.

[etherSky111]

Let's change There may be to There should be. In cases where the opposite open notional is larger, there should be bad debt. The PnL pool involves the user's margin and borrowing fees. When users withdraw their funding fees, it is deducted from the PnL pool, which involves the user's margin and borrowing fees. And As soon as the funding flips to the other side,: this is just assumption. If we evaluate things in this manner, the likelihood of most issues will be very low.

[nevillehuang]

@etherSky111 @IllIllI000 How often can this situation of excess funds occur?

[etherSky111]

This situation can easily happen (The likelihood is really high).

There are 2 possible states. 1. The opposite open notional is larger. 2. vice versa

In case 1, there will be bad debts due to there are more receivers than payers. Users' PnL, borrowing fees and funding fees are settled through PnL pool. If there are X bad debts in funding fees, these will be deducted from PnL pool which includes users' PnL and borrowing fees.

It's important to note that there's no guarantee that the market will swiftly transition from the first case to the second. It will disrupt the equilibrium of the market.

Thanks.

[IllIllI000]

There is no bad debt created. Bad debt is when a specific user has losses that are greater than the margin collateral backing the position, which means it will never be paid back and the system as a whole will have a loss forever. In this case, it's a temporary liquidity deficit that will be resolved when the opposite side becomes the larger open notional. As I've pointed out here, according to the readme, it's expected that there are temporary liquidity deficits, so that is not a security risk. The only thing broken here is that the sponsor though that only trader PnL could cause the temporary deficit, but with this issue, the funding fee can cause it too.

[etherSky111]

Are you sure 100%?

when the opposite side becomes the larger open notional.

As I said earlier, this is just an assumption.

[IllIllI000]

There has never been a perpetuals market, where the funding fee stayed only on one side. The whole point of the fee is to incentivize people to open positions on the other side of the market in order to make the futures price equal the spot price, at which point the fee rate will become zero again, and then there will be a random walk of orders coming in, which will randomly choose the next side with the larger open notional.

[etherSky111]

Ensuring that the sum of funding fees aligns with 0 is not guaranteed. And this is not a temporary liquidity deficits. This deficit is based on the fact that the sum of borrowing fees and users' PnL equal 0.

As a newcomer to Sherlock, I'm unfamiliar with the rules, so I'll leave it to the judge to decide, and I'll respect his decision.

Thank you.

[nevillehuang]

Based on discussions, I believe medium severity is appropriate for this issue.

[WangSecurity]

Agree with the Lead Judge, Medium is indeed appropriate here due to requirement of specific state. Hence, planning to reject the escalation and leave the issue as it is.

[Evert0x]

Result: Medium Unique

[sherlock-admin3]

Escalations have been resolved successfully!

Escalation status:

• GTH1235: rejected