Division by Zero in _computeSpread Function Leads to Potential Runtime Errors and Incorrect Collateral Calculations

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-06-panoptic/blob/main/contracts/CollateralTracker.sol#L1516

Vulnerability details

Impact

The _computeSpread function in the CollateralTracker contract contains a calculation that may result in a division by zero error if either notional or notionalP is zero. This issue can lead to runtime errors, causing the contract to terminate unexpectedly. Such errors can disrupt the collateral tracking process, potentially leading to incorrect collateral calculations and vulnerabilities in the contract.

Proof of Concept

Line of Code:

spreadRequirement = (notional < notionalP)
    ? Math.unsafeDivRoundingUp((notionalP - notional) * contracts, notional)
    : Math.unsafeDivRoundingUp((notional - notionalP) * contracts, notionalP);

If either notional or notionalP is zero, the division in the Math.unsafeDivRoundingUp function will result in a division by zero error, causing the contract execution to fail. This can be demonstrated by setting notional or notionalP to zero and observing the contract's behavior during execution.

Tools Used

Manual

Recommended Mitigation Steps

Check for zero values before performing the division.

function _computeSpread(
    TokenId tokenId,
    uint128 positionSize,
    uint256 index,
    uint256 partnerIndex,
    uint128 poolUtilization
) internal view returns (uint256 spreadRequirement) {
    // Compute the total amount of funds moved for the position's current leg
    LeftRightUnsigned amountsMoved = PanopticMath.getAmountsMoved(tokenId, positionSize, index);

    // Compute the total amount of funds moved for the position's partner leg
    LeftRightUnsigned amountsMovedPartner = PanopticMath.getAmountsMoved(
        tokenId,
        positionSize,
        partnerIndex
    );

    // Amount moved is right slot if tokenType=0, left slot otherwise
    uint128 movedRight = amountsMoved.rightSlot();
    uint128 movedLeft = amountsMoved.leftSlot();

    // Amounts moved for partner
    uint128 movedPartnerRight = amountsMovedPartner.rightSlot();
    uint128 movedPartnerLeft = amountsMovedPartner.leftSlot();

    uint256 tokenType = tokenId.tokenType(index);

    // Compute the max loss of the spread
    uint256 notional;
    uint256 notionalP;
    uint128 contracts;

    if (tokenType == 1) {
        notional = movedRight;
        notionalP = movedPartnerRight;
        contracts = movedLeft;
    } else {
        notional = movedLeft;
        notionalP = movedPartnerLeft;
        contracts = movedRight;
    }

    // Check for zero values to prevent division by zero
    if (notional == 0 || notionalP == 0) {
        spreadRequirement = 0;
    } else {
        spreadRequirement = (notional < notionalP)
            ? Math.unsafeDivRoundingUp((notionalP - notional) * contracts, notional)
            : Math.unsafeDivRoundingUp((notional - notionalP) * contracts, notionalP);
    }
}

Assessed type

Math

[Picodes]

How could they end up being 0 ?

[c4-judge]

Picodes marked the issue as unsatisfactory: Insufficient proof

[anthonyshiks]

Hi @Picodes,

These values can end up being zero if positionSize is zero for a particular leg.

In the getAmountsMoved function, positionSize is used to calculate amount0 and amount1:

if (tokenId.asset(legIndex) == 0) {
    amount0 = positionSize * uint128(tokenId.optionRatio(legIndex));
    amount1 = Math.getAmount1ForLiquidity(Math.getLiquidityForAmount0(tickLower, tickUpper, amount0)).toUint128();
} else {
    amount1 = positionSize * uint128(tokenId.optionRatio(legIndex));
    amount0 = Math.getAmount0ForLiquidity(Math.getLiquidityForAmount1(tickLower, tickUpper, amount1)).toUint128();
}

If positionSize is zero, amount0 and amount1 will also be zero, regardless of the value of tokenId.asset(legIndex). This means that if positionSize is zero for a particular leg, the corresponding movedRight, movedLeft, movedPartnerRight, or movedPartnerLeft values in the _computeSpread function will be zero, potentially leading to notional or notionalP being zero.

[Picodes]

In your original report, you can't just say "if this variable is zero it reverts". You need to properly show with a credible scenario that this variable can be 0.

[anthonyshiks]

@Picodes Yes, noted. I have provided further information in comment to confirm this report.

[Picodes]

My point is that I can't correct the judgment based on these comments. PJQA is made to clarify the original report and correct mistakes, not to add new elements

[anthonyshiks]

@Picodes Possibility of zero value not mitigated has been confirmed in many prior reports that don't necessarily show how the zero value will happen.. even so PJQA is meant to offer a chance for wardens to justify validity of submissions unless I'm mistaken. I can provide links to the reports. Kindly review this again as the original report is valid.

[anthonyshiks]

I can also provide links to reports that judgement of report has changed based on comments. Let me know thanks.

[anthonyshiks]

How could they end up being 0 ?

I was also answering your question given the basis of invalidating report was insufficient proof..and I added proof to clarify the original report which includes where the problem code is.

[Picodes]

• your additional comment isn't proving anything as it's not a full PoC. It's not up to me to build the scenario to get a positionSize of 0
• the rules of PJQA are very clear on the fact that you're suppose to comment further after the judges answer only to bring additional facts or mistakes, and that this is not a debate

Thanks for your understanding

[Picodes]

Regarding other reports, I honestly don't know, but every context is different so let's refer to C4 rules only