Trading.sol: addToPosition function does allow trader to increase position size without paying fees

[code423n4]

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/Trading.sol#L255-L305
https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/Trading.sol#L278
https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/Trading.sol#L293-L294

Vulnerability details

Impact

When opening a position, an opening fee has to be paid.
You can see this in the Trading.initiateMarketOrder function (https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/Trading.sol#L163-L210).

It deposits the whole _tradeInfo.margin (https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/Trading.sol#L180) but the trade is only created with the _marginAfterFees (https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/Trading.sol#L185), i.e. the margin minus the opening fees.

This is how paying the opening fee should work.

There is an issue in the Trading.addToPosition function (https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/Trading.sol#L255-L305). Instead of depositing the whole _addMargin which is the margin that is added to the position, the _addMargin - _fee (https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/Trading.sol#L278) is deposited. This is the same amount that is then added to the position (https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/Trading.sol#L293-L294).

In effect this means that no opening fees are deducted from the margin that is deposited.

Therefore increasing a position is free for the trader (except gas fees).

A trader can exploit this by opening a small position first (which must be as big as the minimum position size).

The trader can then increase his position to the desired size using the Trading.addToPosition function. So the trader only needs to pay the opening fee for the minimum position size.

Proof of Concept

1. The trader opens a position with the Trading.initiateMarketOrder function. The position is big enough to meet the minimum position size requirement (https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/TradingExtension.sol#L218)
2. The trader calls the Trading.addToPosition function to increase his position to the desired size.
   The amount he needs to deposit into the StableVault is _addMargin - fee:

_handleDeposit(
    _trade.tigAsset,
    _marginAsset,
    _addMargin - _fee,
    _stableVault,
    _permitData,
    _trader
);

The _newMargin is calculated as _trade.margin + _addMargin - fee:

_addMargin -= _fee;
uint _newMargin = _trade.margin + _addMargin;

The margin of the trade is then changed to be the _newMargin:

position.addToPosition(
    _trade.id,
    _newMargin,
    _newPrice
);

Code from Position.addToPosition:

function addToPosition(uint256 _id, uint256 _newMargin, uint256 _newPrice) external onlyMinter {
    _trades[_id].margin = _newMargin;
    _trades[_id].price = _newPrice;
    initId[_id] = accInterestPerOi[_trades[_id].asset][_trades[_id].tigAsset][_trades[_id].direction]*int256(_newMargin*_trades[_id].leverage/1e18)/1e18;
}

In the end the trader deposited _addMargin - fee and the position was also increased by _addMargin - fee. So the trader did not pay fees for increasing his position.

Tools Used

VSCode

Recommended Mitigation Steps

The solution is very simple. The trader must deposit the whole _addMargin into the vault.

         _handleDeposit(
             _trade.tigAsset,
             _marginAsset,
-            _addMargin - _fee,
+            _addMargin,
             _stableVault,
             _permitData,
             _trader
         };

[c4-judge]

GalloDaSballo marked the issue as duplicate of #659

[c4-judge]

GalloDaSballo changed the severity to 3 (High Risk)

[c4-judge]

GalloDaSballo marked the issue as satisfactory