Trading.sol: executeLimitOrder function increases open interest by amount that is too large due to not accounting for opening fee

[code423n4]

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/PairsContract.sol#L8
https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/Trading.sol#L480
https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/Position.sol#L99
https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/Trading.sol#L314
https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/Trading.sol#L480
https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/Trading.sol#L163

Vulnerability details

Impact

The PairsContract (https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/PairsContract.sol#L8) keeps track of the open interest for each [asset][tigAsset] pair.

The Trading.executeLimitOrder (https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/Trading.sol#L480) function causes the open interest to be increased.
However the amount by which it is increased is too big because it does not reduce the margin by the amount of fees that is paid upon opening a position.

Also once the position is closed the amount by which the open interest is reduced DOES respect the opening fee.
So the amount by which the open interest is increased is bigger than the amount by which the open interest is reduced when the trade is closed.

Therefore every trade that is opened by executing a limit order causes the open interest to rise by a small amount.

This in turn causes issues in the calculation of the funding rate which relies on the open interst (https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/Position.sol#L99).

There is no immediate loss of funds for a single trade. However in the long run the wrong funding rate calculation can make the Exchange unusable.

So I mark this issue as "Medium".

Proof of Concept

A limit order is created by calling the Trading.initiateLimitOrder function (https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/Trading.sol#L314).

It mints a trade and saves the full margin in it. The opening fees are only later incorporated. After the open interest is increased.

So once the limit order is created, Trading.executeLimitOrder must be called (https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/Trading.sol#L480).

In this function the trade is read into memory:

IPosition.Trade memory trade = position.trades(_id);

Then, without adjusting the trade.margin, the open interest is increased:

if (trade.direction) {
    tradingExtension.modifyLongOi(trade.asset, trade.tigAsset, true, trade.margin*trade.leverage/1e18);
} else {
    tradingExtension.modifyShortOi(trade.asset, trade.tigAsset, true, trade.margin*trade.leverage/1e18);
}

The trade margin is only later adjusted:

position.executeLimitOrder(_id, trade.price, trade.margin - _fee);

You can also easily see that this is a vulnerability by comparing the Trading.executeLimitOrder function to e.g. Trading.initiateMarketOrder (https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/Trading.sol#L163), which increases the open interest by the _positionSize amount:

if (_tradeInfo.direction) {
    tradingExtension.modifyLongOi(_tradeInfo.asset, _tigAsset, true, _positionSize);
} else {
    tradingExtension.modifyShortOi(_tradeInfo.asset, _tigAsset, true, _positionSize);
}

This amount uses the margin adjusted by fees:

uint256 _positionSize = _marginAfterFees * _tradeInfo.leverage / 1e18;

Tools Used

VSCode

Recommended Mitigation Steps

Fix:

                 trade.price -= trade.price * _spread / DIVISION_CONSTANT;
             }
             if (trade.direction) {
-                tradingExtension.modifyLongOi(trade.asset, trade.tigAsset, true, trade.margin*trade.leverage/1e18);
+                tradingExtension.modifyLongOi(trade.asset, trade.tigAsset, true, (trade.margin-_fee)*trade.leverage/1e18);
             } else {
-                tradingExtension.modifyShortOi(trade.asset, trade.tigAsset, true, trade.margin*trade.leverage/1e18);
+                tradingExtension.modifyShortOi(trade.asset, trade.tigAsset, true, (trade.margin-_fee)*trade.leverage/1e18);
             }
             _updateFunding(trade.asset, trade.tigAsset);
             position.executeLimitOrder(_id, trade.price, trade.margin - _fee);

[c4-judge]

GalloDaSballo marked the issue as duplicate of #576

[c4-judge]

GalloDaSballo marked the issue as satisfactory