Tigris Trade contest - koxuan's results

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/Trading.sol#L708-L718 https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/Trading.sol#L786-L792 https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/Trading.sol#L954-L967

Vulnerability details

Impact

If user is referred, referral fees will be sent to the referral and a discount will be deducted on the trading fees of user. However, the amount for the discount is the same as the referral fees in the calculation, causing user to pay lesser fees than they should if they are referred.

Proof of Concept

In handleOpenFees, you can see that if referrer is present, it will mint referralFees for the referrer. The next step is to deduct the referral fees and discount from user fees that they will pay, instead of deducting discount which is 10% of trade fees separately, referralFees is being used instead as the amount for the discount for referred user.

According to https://docs.tigris.trade/protocol/trading-and-fees and https://docs.tigris.trade/protocol/referrals, The discount should be 10% of 0.1% trading fees for commodities and crypto, which is significantly lesser than 0.05% that referrer will get according to the docs. User will pay lesser fees, dao will get lesser fees and therefore GovNFT holders will lose out on fees.

        if (_referrer != address(0)) {
            unchecked {
                IStable(_tigAsset).mintFor(
                    _referrer,
                    _positionSize
                    * _fees.referralFees // get referral fee%
                    / DIVISION_CONSTANT // divide by 100%
                );
            }
            _fees.daoFees = _fees.daoFees - _fees.referralFees*2;
        }

Tools Used

Manual Review

Recommended Mitigation Steps

Few places need to be rewritten to calculate the discount standalone instead of using referral fees as the discount

https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/Trading.sol#L708-L718 https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/Trading.sol#L788-L792 https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/Trading.sol#L954-L967

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/Trading.sol#L574 https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/TradingExtension.sol#L98 https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/TradingExtension.sol#L172-L180 https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/utils/TradingLibrary.sol#L113

Vulnerability details

Impact

latestAnswer in ChainLink Api is deprecated according to chainlink docs https://docs.chain.link/data-feeds/price-feeds/ .This might return stale data or returns 0 in the event of an error. If it returns stale data, verifyPrice might fail due to difference in price or succeed even though the fresh price from chainlink is not within the tolerance of the other oracle price. If it returns 0, no checks are done and the other oracle gets to dictate the price.

Proof of Concept

limitClose in Trading.sol is the function that is affected by this. Order of calling is limitClose -> _limitClose -> getVerifiedPrice -> verifyPrice.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider using the latestRoundData to get price

       (uint80 roundID, int256 assetChainlinkPriceInt, , uint256 timestamp, uint80 answeredInRound) = IPrice(_chainlinkFeed).latestRoundData();

        require(assetChainlinkPriceInt > 0, "Price have to be more than 0");
        require(answeredInRound >= roundID, "Stale price");
        require(timestamp > 0, "Round not completed");