IllIllI - Position fees are still assessed even if the ability to decrease positions is disabled

[sherlock-admin]

IllIllI

medium

Position fees are still assessed even if the ability to decrease positions is disabled

Summary

Position fees (borrowing and funding) are still assessed even if the ability to decrease positions is disabled

Vulnerability Detail

Config keepers have the ability to disable order placement and order execution, and them doing so does not pause the state of position fees.

Impact

Users will be assessed position fees even if they wished to close their positions, and can be liquidated through no fault of their own.

Code Snippet

Order creation (to close a position) may be disabled:

// File: gmx-synthetics/contracts/exchange/OrderHandler.sol : OrderHandler.createOrder()   #1

43:           FeatureUtils.validateFeature(dataStore, Keys.createOrderFeatureDisabledKey(address(this), uint256(params.orderType)));

https://github.com/sherlock-audit/2023-02-gmx/blob/main/gmx-synthetics/contracts/exchange/OrderHandler.sol#L43

As may execution of orders prior to the disabling of creation:

// File: gmx-synthetics/contracts/exchange/OrderHandler.sol : OrderHandler._executeOrder()   #2

207:           FeatureUtils.validateFeature(params.contracts.dataStore, Keys.executeOrderFeatureDisabledKey(address(this), uint256(params.order.orderType())));

https://github.com/sherlock-audit/2023-02-gmx/blob/main/gmx-synthetics/contracts/exchange/OrderHandler.sol#L197-L210

But position fees are tracked based on time and do not account for pauses:

// File: gmx-synthetics/contracts/market/MarketUtils.sol : MarketUtils.updatedAt   #3

1759        function getSecondsSinceCumulativeBorrowingFactorUpdated(DataStore dataStore, address market, bool isLong) internal view returns (uint256) {
1760            uint256 updatedAt = getCumulativeBorrowingFactorUpdatedAt(dataStore, market, isLong);
1761            if (updatedAt == 0) { return 0; }
1762 @>         return block.timestamp - updatedAt;
1763:       }

https://github.com/sherlock-audit/2023-02-gmx/blob/main/gmx-synthetics/contracts/market/MarketUtils.sol#L1759-L1763

// File: gmx-synthetics/contracts/market/MarketUtils.sol : MarketUtils.getSecondsSinceFundingUpdated()   #4

1658        function getSecondsSinceFundingUpdated(DataStore dataStore, address market) internal view returns (uint256) {
1659            uint256 updatedAt = dataStore.getUint(Keys.fundingUpdatedAtKey(market));
1660            if (updatedAt == 0) { return 0; }
1661 @>         return block.timestamp - updatedAt;
1662:       }

https://github.com/sherlock-audit/2023-02-gmx/blob/main/gmx-synthetics/contracts/market/MarketUtils.sol#L1658-L1662

Tool used

Manual Review

Recommendation

Track and account for disabling, and adjust position fees based on whether things were paused or not.

[xvi10]

this is a valid concern, but we do not think this logic should be added

orders should only be disabled if there is an emergency issue, we do not think the additional complexity is worth adding for this case