Tigris Trade contest - Jeiwan's results

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/Trading.sol#L278

Vulnerability details

Impact

The protocol doesn't receive fees when margin is added via the addToPosition function. An equal amount of tigUSD is minted even though it's not backed by margin.

Proof of Concept

The protocol takes fees from margin provided by users: for example, when opening a market order:

1. fees are calculated on the total order size (margin * leverage): Trading.sol#L178;
2. full margin is transferred from the trader (fees are not subtracted): Trading.sol#L180;
3. accounted margin excludes fess: Trading.sol#L183-L194 (see _marginAfterFees).

Thus, traders provide full margin when opening orders and fees are subtracted from accounted margin. If trader closes an order, they'll receive margin - fees.

The protocol also allows traders to add margin to existing positions at a different price, via the addToPosition function (Trading.sol#L255). This function, however, subtracts fees from margin before transferring margin from traders:

_handleDeposit(
    _trade.tigAsset,
    _marginAsset,
    _addMargin - _fee, // @audit trader won't pay fees
    _stableVault,
    _permitData,
    _trader
);

As a result, the protocol doesn't receive fees when margin is added via addToPosition.

Tools Used

Manual review

Recommended Mitigation Steps

Consider not subtracting fees from _addMargin in addToPosition when calling _handleDeposit.

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/Trading.sol#L513-L517

Vulnerability details

Impact

The open interest trackers in the PairsContract will accumulate non-removable residual due to fees not being excluded from open interest values. The residual will grow over time.

Proof of Concept

Open interest is the total leveraged value deposited by traders to the protocol. It excludes fees since fees are subtracted from traders' positions and sent to a treasury. For example, when opening a market order, open interest is changed by the position size, (margin - fees) * leverage (Trading.sol#L198-L200):

uint256 _marginAfterFees = _tradeInfo.margin - _handleOpenFees(_tradeInfo.asset, _tradeInfo.margin*_tradeInfo.leverage/1e18, _trader, _tigAsset, false);
uint256 _positionSize = _marginAfterFees * _tradeInfo.leverage / 1e18;
...
if (_tradeInfo.direction) {
    tradingExtension.modifyLongOi(_tradeInfo.asset, _tigAsset, true, _positionSize);
} else {
    tradingExtension.modifyShortOi(_tradeInfo.asset, _tigAsset, true, _positionSize);
}

For limit orders, however, accounting is different. Fees are not applied when a limit order is created because limit orders are not active until they're executed (Trading.sol#L330-L346):

// @audit-info full margin is deposited
_handleDeposit(_tigAsset, _tradeInfo.marginAsset, _tradeInfo.margin, _tradeInfo.stableVault, _permitData, _trader);
...
// @audit-info full margin is accounted
position.mint(
    IPosition.MintTrade(
        _trader,
        _tradeInfo.margin,
        _tradeInfo.leverage,
        _tradeInfo.asset,
        _tradeInfo.direction,
        _price,
        _tradeInfo.tpPrice,
        _tradeInfo.slPrice,
        _orderType,
        _tigAsset
    )
);

However, when a limit order is executed, the change in open interest doesn't account for fees (Trading.sol#L513-L517):

// @audit-info fees are calculated on the full position size, this is correct
uint _fee = _handleOpenFees(trade.asset, trade.margin*trade.leverage/1e18, trade.trader, trade.tigAsset, true);
...
if (trade.direction) {
    // @audit wrong: fees are not subtracted from the position size
    tradingExtension.modifyLongOi(trade.asset, trade.tigAsset, true, trade.margin*trade.leverage/1e18);
} else {
    // @audit wrong: fees are not subtracted from the position size
    tradingExtension.modifyShortOi(trade.asset, trade.tigAsset, true, trade.margin*trade.leverage/1e18);
}
...
// @audit-info accounted margin excludes fees, correct
position.executeLimitOrder(_id, trade.price, trade.margin - _fee);

As a result, open interest will be increased by an amount that is bigger than the actual value deposited to the protocol. However, when positions are closed or liquidated, open interest is reduced by values that include fees (TradingExtension.sol#L70-L74, Trading.sol#L546-L550). This will create a residual in open interest that will accumulate over time and may, in the worst case, cause a denial of service when new positions are opened: the modifyLongOi and modifyShortOi revert when maximal open interest is reached (PairsContract.sol#L157, PairsContract.sol#L177).

Tools Used

Manual review

Recommended Mitigation Steps

Consider excluding fees from the position size before updating open interest in the executeLimitOrder function.

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/utils/TradingLibrary.sol#L113

Vulnerability details

Impact

The deprecated interface doesn't provide additional data that can make integration with Chainlink more reliable. This can potentially lead the protocol to be more sensitive to disruptions or non-standard behavior of Chainlink oracles.

Proof of Concept

The verifyPrice function of the TradingLibrary library uses a deprecated Chainlink interface (TradingLibrary.sol#L113):

int256 assetChainlinkPriceInt = IPrice(_chainlinkFeed).latestAnswer();

As per the Chainlink documentation, the latestAnswer function is deprecated.

This interface doesn't allow to check the timestamp of when the latest price was reported and ensure that the price is not stale. Also, the Chainlink data feed aggregator provides minimal and maximal price values per each supported token to improve validation of prices returned by oracles. Using the old interface may result in the protocol not detecting stale or wrong prices reported by Chainlink feeds.

Tools Used

Manual review

Recommended Mitigation Steps

Consider using the latestRoundData function instead of latestAnswer and consider following the Monitoring Data Feeds recommendations by Chainlink.