sherlock-audit / 2023-05-perennial-judging

12 stars 9 forks source link

ast3ros - Maker can close position without checking utilization rate. #210

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

ast3ros

medium

Maker can close position without checking utilization rate.

Summary

A maker can close his position and increase the utilization rate above the allowed level.

Vulnerability Detail

When the product owner creates a product market, he sets the utilization buffer to limit the utilization rate to a certain level. This is necessary to prevent potential issues that could arise when utilization gets too high, such as liquidity crunches or funding rate spikes (if using JumpRateUtilizationCurve).

    function updateUtilizationBuffer(UFixed18 newUtilizationBuffer) external onlyProductOwner settleProduct {
        if (newUtilizationBuffer.gt(UFixed18Lib.ONE)) revert ParamProviderInvalidParamValue();
        _utilizationBuffer.store(newUtilizationBuffer);
        emit UtilizationBufferUpdated(newUtilizationBuffer, _productVersion());
    }

https://github.com/sherlock-audit/2023-05-perennial/blob/main/perennial-mono/packages/perennial/contracts/product/UParamProvider.sol#L249-L253

The utilization level check is implemented in the maxUtilizationInvariant modifier

    modifier maxUtilizationInvariant() {
        _;

        if (closed()) return;

        Position memory next = positionAtVersion(latestVersion()).next(_position.pre);
        UFixed18 utilization = next.taker.unsafeDiv(next.maker);
        if (utilization.gt(UFixed18Lib.ONE.sub(utilizationBuffer())))
            revert ProductInsufficientLiquidityError(utilization);
    }

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

However, when a maker closes his position, the utilization level check is not applied. It means that the maker can close his position and drive the utilization rate higher than the safe level (1 - utilizationBuffer). This will lead to operational issues when takers cannot open positions anymore or face a jump in funding rate.

Also, when calculating the max redeem amount to withdraw from the BalancedVault, the utilization level with the buffer is not considered. It can lead to a redemption that raises the utilization rate to 1.

    UFixed18 longAvailable = longGlobalPosition.maker.sub(longGlobalPosition.taker.min(longGlobalPosition.maker)); 
    UFixed18 shortAvailable = shortGlobalPosition.maker.sub(shortGlobalPosition.taker.min(shortGlobalPosition.maker));

https://github.com/sherlock-audit/2023-05-perennial/blob/main/perennial-mono/packages/perennial-vaults/contracts/balanced/BalancedVault.sol#L669-L670

Impact

The utilization rate can exceed the buffer level set by the product owner. The product market will face liquidity and funding issues.

Code Snippet

https://github.com/sherlock-audit/2023-05-perennial/blob/main/perennial-mono/packages/perennial/contracts/product/UParamProvider.sol#L249-L253 https://github.com/sherlock-audit/2023-05-perennial/blob/main/perennial-mono/packages/perennial/contracts/product/Product.sol#L547-L556 https://github.com/sherlock-audit/2023-05-perennial/blob/main/perennial-mono/packages/perennial-vaults/contracts/balanced/BalancedVault.sol#L669-L670

Tool used

Manual Review

Recommendation

Add maxUtilizationInvariant to Product.closeMakeFor function and consider the utilizationBuffer when calculating the max redeem amount in BalancedVault._maxRedeemAtEpoch function.

Duplicate of #75