Wrong rate when maker and taker are 0

Summary

When maker and taker positions are zero, the rate function should return minRate but instead is returning maxRate.

Vulnerability Detail

In Product contract, there's a function rate that returns the funding rate based on the provided position:

/**
 * @notice Returns The per-second rate based on the provided `position`
 * @dev Handles 0-maker/taker edge cases
 * @param position_ Position to base utilization on
 * @return The per-second rate
 */
function rate(Position calldata position_) public view returns (Fixed18) {
    UFixed18 utilization = position_.taker.unsafeDiv(position_.maker);
    Fixed18 annualizedRate = utilizationCurve().compute(utilization); // @audit If both maker and taker is zero, utilization will be 1e18 so will result in maxRate
    return annualizedRate.div(Fixed18Lib.from(365 days));
}

The Natspec documentation is explicitly stating that edge cases are handled by this function but as we'll see now, that's not true.

When position_.taker and position_.maker are both zero, utilization will be 1e18 (ONE) as a result of unsafeDiv:

/**
 * @notice Divides unsigned fixed-decimal `a` by `b`
 * @dev Does not revert on divide-by-0, instead returns `ONE` for `0/0` and `MAX` for `n/0`.
 * @param a Unsigned fixed-decimal to divide
 * @param b Unsigned fixed-decimal to divide by
 * @return Resulting divided unsigned fixed-decimal
 */
function unsafeDiv(UFixed18 a, UFixed18 b) internal pure returns (UFixed18) {
    if (isZero(b)) {
        return isZero(a) ? ONE : MAX;
    } else {
        return div(a, b);
    }
}

Utilization being 1e18 will lead to annualizedRate being maxRate instead of minRate. This is calculated in utilizationCurve().compute(utilization):

/**
 * @notice Computes the corresponding rate for a utilization ratio
 * @param utilization The utilization ratio
 * @return The corresponding rate
 */
function compute(JumpRateUtilizationCurve memory self, UFixed18 utilization) internal pure returns (Fixed18) {
    UFixed18 targetUtilization = self.targetUtilization.unpack();
    if (utilization.lt(targetUtilization)) {
        return CurveMath.linearInterpolation(
            UFixed18Lib.ZERO,
            self.minRate.unpack(),
            targetUtilization,
            self.targetRate.unpack(),
            utilization
        );
    }
    if (utilization.lt(UFixed18Lib.ONE)) {
        return CurveMath.linearInterpolation(
            targetUtilization,
            self.targetRate.unpack(),
            UFixed18Lib.ONE,
            self.maxRate.unpack(),
            utilization
        );
    }
    return self.maxRate.unpack();
}

As it can be seen, if utilization is 1e18, the return value will always be maxRate, thus giving a wrong rate.

Impact

This issue can cause confusion or unexpected behaviour on external contracts calling and interacting with this rate function and can cause exploits with protocols integrated with Perennial.

Code Snippet

https://github.com/sherlock-audit/2023-05-perennial/blob/main/perennial-mono/packages/perennial/contracts/product/Product.sol#L484-L494

Tool used

Manual Review

Recommendation

It's recommended to handle this edge case in the rate function by assigning 0 to utilization when position_.taker and position_.maker are both zero.

function rate(Position calldata position_) public view returns (Fixed18) {
    UFixed18 utilization;

    if(position_.maker.isZero() && position_.maker.isZero()) {
        utilization = UFixed18Lib.ZERO;
    } else {
        utilization = position_.taker.unsafeDiv(position_.maker);
    }

    Fixed18 annualizedRate = utilizationCurve().compute(utilization);
    return annualizedRate.div(Fixed18Lib.from(365 days));
}

sherlock-audit / 2023-05-perennial-judging

BLACK-PANDA-REACH - Wrong rate when maker and taker are 0 #219