BLACK-PANDA-REACH - Loss of Precision when calculating `positionFee`

[sherlock-admin]

BLACK-PANDA-REACH

medium

Loss of Precision when calculating positionFee

Summary

When opening or closing a position, a positionFee is taken from the user's collateral but the fee is sometimes rounded down to zero if called with enough low values.

Vulnerability Detail

In Product contract, functions openTakeFor, _closeTake, openMakeFor and _closeMake make some calculations to get the positionFee that the user must pay for opening or closing positions.

The calculations are equal in 4 functions, we'll take openTakeFor as an example:

UFixed18 positionFee = amount.mul(latestOracleVersion.price.abs()).mul(takerFee());

This uses the mul function from UFixed18 library:

function mul(UFixed18 a, UFixed18 b) internal pure returns (UFixed18) {
    return UFixed18.wrap(UFixed18.unwrap(a) * UFixed18.unwrap(b) / BASE);
}

So the calculation of positionFee can be translated to the following expression:

positionFee = amount * latestOracleVersion.price / 1e18 * takerFee / 1e18

As you can see, there's some division performed before the multiplication and that can carry precision loss.

In products that latestOracleVersion.price is a small number, when calling the function with an amount being low enough the user can avoid paying the position fee.

Imagine the following scenario:

1. We have a product with a latestOracleVersion.price of 1e10 and a takerFee of 15e14 (0.0015 in ETH).
2. A user wants to open a taker position of 1e18 but doesn't want to pay the fees.
3. Instead of calling openTake with an amount of 1e18 it calls openTake 100000000 times with a value of 1e10.
4. Because of the precision loss, the position fee calculated will be zero so the user won't pay any fees.

positionFee = amount * latestOracleVersion.price / 1e18 * takerFee / 1e18
positionFee = 1e10 * 1e10 / 1e18 * 15e14 / 1e18
positionFee = 0

Impact

Everyone can call the openTakeFor, _closeTake, openMakeFor and _closeMake functions and avoid paying any position fees. Later, when settling a new oracle version, position fees are going to be distributed to makers so it can cause protocol insolvency.

Code Snippet

Snippet in openTakeFor function: https://github.com/sherlock-audit/2023-05-perennial/blob/main/perennial-mono/packages/perennial/contracts/product/Product.sol#L223

Snippet in _closeTake function: https://github.com/sherlock-audit/2023-05-perennial/blob/main/perennial-mono/packages/perennial/contracts/product/Product.sol#L264

Snippet in openMakeFor function: https://github.com/sherlock-audit/2023-05-perennial/blob/main/perennial-mono/packages/perennial/contracts/product/Product.sol#L305

Snippet in _closeMake function: https://github.com/sherlock-audit/2023-05-perennial/blob/main/perennial-mono/packages/perennial/contracts/product/Product.sol#L347

Tool used

Manual Review

Recommendation

After the position fee calculation, it's recommended to check that if the set fee is not zero, the calculated fee must not be zero.

Example solution with openTakeFor:

function openTakeFor(address account, UFixed18 amount)
    public
    nonReentrant
    notPaused
    notClosed
    onlyAccountOrMultiInvoker(account)
    settleForAccount(account)
    maxUtilizationInvariant
    positionInvariant(account)
    liquidationInvariant(account)
    maintenanceInvariant(account)
{
    // {...}

    UFixed18 positionFee = amount.mul(latestOracleVersion.price.abs()).mul(takerFee());
+   if(takerFee() != 0 && positionFee == 0) revert(); // @audit Sanity check to avoid precision loss

    // {...}
}