Tigris Trade contest - 0x52's results

Lines of code

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

Vulnerability details

Impact

User can make more money than expected potentially causing liquidity issues for the stableVault

Proof of Concept

    (IPosition.Trade memory _trade, uint256 _positionSize, int256 _payout) = tradingExtension._closePosition(_id, _price, _percent);
    position.setAccInterest(_id);
    _updateFunding(_trade.asset, _trade.tigAsset);
    if (_percent < DIVISION_CONSTANT) {
        if ((_trade.margin*_trade.leverage*(DIVISION_CONSTANT-_percent)/DIVISION_CONSTANT)/1e18 < tradingExtension.minPos(_trade.tigAsset)) revert("!size");
        position.reducePosition(_id, _percent);
    } else {
        position.burn(_id);
    }
    uint256 _toMint;
    if (_payout > 0) {
        unchecked {
            _toMint = _handleCloseFees(_trade.asset, uint256(_payout)*_percent/DIVISION_CONSTANT, _trade.tigAsset, _positionSize*_percent/DIVISION_CONSTANT, _trade.trader, _isBot);

            //@audit compares _toMint against margin of entire trade rather than just the portion being withdrawn
            if (maxWinPercent > 0 && _toMint > _trade.margin*maxWinPercent/DIVISION_CONSTANT) {
                _toMint = _trade.margin*maxWinPercent/DIVISION_CONSTANT;
            }
        }
        _handleWithdraw(_trade, _stableVault, _outputToken, _toMint);
    }

maxWinPercent ensures that the user is only able to make up to a certain percentage gain. Trading#_closePosition implements a check L625 to reduce the user's payout if maxWinPercent is exceeded. The problem is that the check will be incorrect if the user is closing only a portion of their position, because it compares the amount to mint against the margin of the ENTIRE position rather than just the portion that is being closed.

This is possible because _trade is stored to memory before the reducePosition call, which modifies the trade storage. This means that the original margin value will be used instead of the modified one.

Example: Assume maxWinPercent = 200e10 (100% gain) and trade.Margin = 100e18. Now the user's trade is highly profitable and the user is set to get a payout of 250e18 (150% gain). The check in L625 should reduce this to 200e18 (200e10 * 100e18 / 100e10). The user can avoid this limit by closing out 80% of their position at a time. This would mean that 80e18 of their margin would be closed which would net them a payout of 200e18 (150% gain). They could then repeat this process again withdrawing 80% of their new position. After 3 iterations they would have successfully avoided the cap on 99.2% of their original position.

Tools Used

Manual Review

Recommended Mitigation Steps

Adjust the comparison to account for the decreased margin:

-   if (maxWinPercent > 0 && _toMint > _trade.margin*maxWinPercent/DIVISION_CONSTANT) {
+   if (maxWinPercent > 0 && _toMint > _trade.margin*maxWinPercent*percent/(DIVISION_CONSTANT*DIVISION_CONSTANT) {

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/TradingExtension.sol#L88-L120

Vulnerability details

Impact

User can abuse how stop losses are priced to open high leverage trades with huge upside and very little downside

Proof of Concept

function limitClose(
    uint _id,
    bool _tp,
    PriceData calldata _priceData,
    bytes calldata _signature
)
    external
{
    _checkDelay(_id, false);
    (uint _limitPrice, address _tigAsset) = tradingExtension._limitClose(_id, _tp, _priceData, _signature);
    _closePosition(_id, DIVISION_CONSTANT, _limitPrice, address(0), _tigAsset, true);
}

function _limitClose(
    uint _id,
    bool _tp,
    PriceData calldata _priceData,
    bytes calldata _signature
) external view returns(uint _limitPrice, address _tigAsset) {
    _checkGas();

    IPosition.Trade memory _trade = position.trades(_id);
    _tigAsset = _trade.tigAsset;
    getVerifiedPrice(_trade.asset, _priceData, _signature, 0);
    uint256 _price = _priceData.price;
    if (_trade.orderType != 0) revert("4"); //IsLimit
    if (_tp) {
        if (_trade.tpPrice == 0) revert("7"); //LimitNotSet
        if (_trade.direction) {
            if (_trade.tpPrice > _price) revert("6"); //LimitNotMet
        } else {
            if (_trade.tpPrice < _price) revert("6"); //LimitNotMet
        }
        _limitPrice = _trade.tpPrice;
    } else {
        if (_trade.slPrice == 0) revert("7"); //LimitNotSet
        if (_trade.direction) {
            if (_trade.slPrice < _price) revert("6"); //LimitNotMet
        } else {
            if (_trade.slPrice > _price) revert("6"); //LimitNotMet
        }
        //@audit stop loss is closed at user specified price NOT market price
        _limitPrice = _trade.slPrice;
    }
}

When closing a position with a stop loss the user is closed at their SL price rather than the current price of the asset. A user could abuse this in directional markets with high leverage to make nearly risk free trades. A user could open a long with a stop loss that in $0.01 below the current price. If the price tanks immediately on the next update then they will be closed out at their entrance price, only out the fees to open and close their position. If the price goes up then they can make a large gain.

Tools Used

Manual Review

Recommended Mitigation Steps

Take profit and stop loss trades should be executed at the current price rather than the price specified by the user:

         if (_trade.tpPrice == 0) revert("7"); //LimitNotSet
        if (_trade.direction) {
            if (_trade.tpPrice > _price) revert("6"); //LimitNotMet
        } else {
            if (_trade.tpPrice < _price) revert("6"); //LimitNotMet
        }
-       _limitPrice = _trade.tpPrice;
+       _limitPrice = _price;
    } else {
        if (_trade.slPrice == 0) revert("7"); //LimitNotSet
        if (_trade.direction) {
            if (_trade.slPrice < _price) revert("6"); //LimitNotMet
        } else {
            if (_trade.slPrice > _price) revert("6"); //LimitNotMet
        }
-       _limitPrice = _trade.slPrice;
+       _limitPrice = _price;

Lines of code

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

Vulnerability details

Impact

User can profit from when there are multiple valid prices for an asset

Proof of Concept

function _checkDelay(uint _id, bool _type) internal {
    unchecked {
        Delay memory _delay = blockDelayPassed[_id];
        if (_delay.actionType == _type) {
            blockDelayPassed[_id].delay = block.number + blockDelay;
        } else {
            if (block.number < _delay.delay) revert("0"); //Wait
            blockDelayPassed[_id].delay = block.number + blockDelay;
            blockDelayPassed[_id].actionType = _type;
        }
    }
}

Trading#_checkDelay is used "to prevent profitable opening and closing in the same tx with two different prices in the 'valid signature pool'". This unfortunately doesn't actually prevent this from happening. To take advantage of this the user would just open a long at the lower valid price and a short at the higher valid price. After the delay has ended the user can close both for a guaranteed profit.

Example: ETH/USD has two valid signed prices. One at 990 and the other at 1010. The user can simultaneously open a 1 ETH short with the 1010 price and a 1 ETH long with the 990 price. Assume that after the block delay the price is 1005. The long can now be closed for a $15 gain an the short can be closed for $5.

Tools Used

Manual Review

Recommended Mitigation Steps

Never allow for there to be more than one valid price for each asset at any given time

Lines of code

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

Vulnerability details

Impact

USDT and other tokens that require zero allowance before approval will be incompatible as a margin asset

Proof of Concept

IERC20(_marginAsset).transferFrom(_trader, address(this), _margin/_marginDecMultiplier);
IERC20(_marginAsset).approve(_stableVault, type(uint).max);
IStableVault(_stableVault).deposit(_marginAsset, _margin/_marginDecMultiplier);

Trading#_handleDeposit is used to pull marginAsset from the trader and deposit into the correct vault. Before each deposit it approves the marginAsset to the stableVault then deposits. This is problematic for assets like USDT, which require the current allowance to be zero. After the first deposit, all following deposits for USDT will revert when trying to make the approve call.

Tools Used

Manual Review

Recommended Mitigation Steps

    IERC20(_marginAsset).transferFrom(_trader, address(this), _margin/_marginDecMultiplier);
-   IERC20(_marginAsset).approve(_stableVault, type(uint).max);
+   IERC20(_marginAsset).approve(_stableVault, _margin/_marginDecMultiplier);
    IStableVault(_stableVault).deposit(_marginAsset, _margin/_marginDecMultiplier);

Lines of code

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

Vulnerability details

Impact

Trade delay will not work correctly on Arbitrum allowing users to exploit multiple valid prices

Proof of Concept

function _checkDelay(uint _id, bool _type) internal {
    unchecked {
        Delay memory _delay = blockDelayPassed[_id];
        //in those situations
        if (_delay.actionType == _type) {
            blockDelayPassed[_id].delay = block.number + blockDelay;
        } else {
            if (block.number < _delay.delay) revert("0"); //Wait
            blockDelayPassed[_id].delay = block.number + blockDelay;
            blockDelayPassed[_id].actionType = _type;
        }
    }
}

_checkDelay enforces a delay of a specific number of block between opening and closing a position. While this structure will work on mainnet, it is problematic for use on Arbitrum. According to Arbitrum Docs block.number returns the most recently synced L1 block number. Once per minute the block number in the Sequencer is synced to the actual L1 block number. This period could be abused to completely bypass this protection. The user would open their position 1 Arbitrum block before the sync happens, the close it the very next block. It would appear that there has been 5 block (60 / 12) since the last transaction but in reality it has only been 1 Arbitrum block. Given that Arbitrum has 2 seconds blocks I would be impossible to block this behavior through parameter changes.

It also presents an issue for Optimism because each transaction is it's own block. No matter what value is used for the block delay, the user can pad enough tiny transactions to allow them to close the trade immediately.

Tools Used

Manual Review

Recommended Mitigation Steps

The delay should be measured using block.timestamp rather than block.number

Lines of code

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

Vulnerability details

Impact

verifyPrice may check against stale data causing valid transactions to revert

Proof of Concept

    if (_chainlinkEnabled && _chainlinkFeed != address(0)) {
        int256 assetChainlinkPriceInt = IPrice(_chainlinkFeed).latestAnswer();
        if (assetChainlinkPriceInt != 0) {
            uint256 assetChainlinkPrice = uint256(assetChainlinkPriceInt) * 10**(18 - IPrice(_chainlinkFeed).decimals());
            require(
                _priceData.price < assetChainlinkPrice+assetChainlinkPrice*2/100 &&
                _priceData.price > assetChainlinkPrice-assetChainlinkPrice*2/100, "!chainlinkPrice"
            );
        }
    }

Chainlink prices are used as a fail-safe in case malicious price data is being produced by a node. When pulling the price it never checks if the price is fresh. This can lead to legitimate transactions reverting if the Chainlink price is stale, causing the whole system to freeze until the oracle is back online.

Tools Used

Manual Review

Recommended Mitigation Steps

Downtime in a futures market can be extremely costly. Since the Chainlink oracle is just a fail-safe it should be bypassed if the data is stale