Tigris Trade contest - ladboy233's results

Lines of code

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

Vulnerability details

Impact

Lack of reasonable boundary for fee setting

Proof of Concept

the fee structured is listed in the doc

https://docs.tigris.trade/protocol/trading-and-fees#fee-structure

and the liquidation fee is listed in

https://docs.tigris.trade/protocol/trading-and-fees/liquidation

note the doc:

Liquidations are performed by bots that are paid with 0.01% of the position size. The rest remains inside the StableVault to increase its collateralization levels.

However, in the code, there is no such implementation about fee setting in the doc and no such restriction in fee setting:

the owner can set fee whatever they want.

function setFees(bool _open, uint _daoFees, uint _burnFees, uint _referralFees, uint _botFees, uint _percent) external onlyOwner {
	unchecked {
		require(_daoFees >= _botFees+_referralFees*2);
		if (_open) {
			openFees.daoFees = _daoFees;
			openFees.burnFees = _burnFees;
			openFees.referralFees = _referralFees;
			openFees.botFees = _botFees;
		} else {
			closeFees.daoFees = _daoFees;
			closeFees.burnFees = _burnFees;
			closeFees.referralFees = _referralFees;
			closeFees.botFees = _botFees;                
		}
		require(_percent <= DIVISION_CONSTANT);
		vaultFundingPercent = _percent;
	}
}

Same issue for parameter settings which lacks of reasonable boundry is in the code below as well:

/**
 * @dev Sets max payout % compared to margin
 * @param _maxWinPercent payout %
 */
function setMaxWinPercent(
	uint _maxWinPercent
)
	external
	onlyOwner
{
	maxWinPercent = _maxWinPercent;
}

/**
 * @dev Sets executable price range for limit orders
 * @param _range price range in %
 */
function setLimitOrderPriceRange(uint _range) external onlyOwner {
	limitOrderPriceRange = _range;
}

and in block delay:

    function setBlockDelay(
        uint _blockDelay
    )
        external
        onlyOwner
    {
        blockDelay = _blockDelay;
    }

https://docs.tigris.trade/protocol/trading-and-fees#limitations

note the doc:

All trading orders need to confirmed within 10 seconds, otherwise the locked in price will expire.

But different blockchain has different confirmation time, using 10 seconds for all blockchain is too harsh (for example, ethereum has blockchain confirmation of 12 - 15 seconds).

Tools Used

Manual Review

Recommended Mitigation Steps

We recommend the project add reasonable upper and lower bound for parameter setting to conform the document.

Lines of code

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

Vulnerability details

Impact

No check for active Arbitrum Sequencer in Chainlink Oracle

Proof of Concept

If the Arbitrum Sequencer goes down, oracle data will not be kept up to date, and thus could become stale. However, users are able to continue to interact with the protocol directly through the L1 optimistic rollup contract.

You can review Chainlink docs on L2 Sequencer Uptime Feeds for more details on this.

https://docs.chain.link/data-feeds/l2-sequencer-feeds

note, this is not same issue as not checking the timestamp.

If invalid price is used, the trade execution will be greatly affect because trade execution price solely dependent on the centralized price feed.

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"
		);
	}
}

Tools Used

Manual Review

Recommended Mitigation Steps

We recommend the project check the arbitrum sequencer if the contract is deployed in arbitrum sequencer.