When symbol manager modifies the tradingFee of a certain symbol via ControlFacet.setSymbolTradingFee, the value returned by LibQuote.returnTradingFee is not the original fee

Summary

LibQuote.receiveTradingFee/returnTradingFee internally use the fee returned by LibQuote.getTradingFee where relies on symbol.tradingFee to calculate the fee. If symbol.tradingFee is changed, the new trading fee will be returned to PartyA when a quote created with the old tradingFee is cancelled. The old and new trading fees are obviously different. There will be two problems:

Malicious PartyA will drain the balance of the feeCollector.
PartyA loses the difference between the old and new trading fee.

Vulnerability Detail

Let's first look at the code of LibQuotegetTradingFee:

File: symmio-core\contracts\libraries\LibQuote.sol
122:     function getTradingFee(uint256 quoteId) internal view returns (uint256 fee) {
123:         QuoteStorage.Layout storage quoteLayout = QuoteStorage.layout();
124:         Quote storage quote = quoteLayout.quotes[quoteId];
125:->       Symbol storage symbol = SymbolStorage.layout().symbols[quote.symbolId];
126:         if (quote.orderType == OrderType.LIMIT) {
127:             fee =
128:->               (LibQuote.quoteOpenAmount(quote) * quote.requestedOpenPrice * symbol.tradingFee) /
129:                 1e36;
130:         } else {
131:->           fee = (LibQuote.quoteOpenAmount(quote) * quote.marketPrice * symbol.tradingFee) / 1e36;
132:         }
133:     }

The calculation of fee will change with symbol.tradingFee.

There are two situations when the symbol manger modifies the tradingFee of a certain symbol.

old tradingFee < new tradingFee
old tradingFee > new tradingFee

Let's look at the first situation. For simplicity, I changed formulas [1](https://github.com/sherlock-audit/2023-06-symmetrical/blob/main/symmio-core/contracts/libraries/LibQuote.sol#L128) and 2 to fee = value * symbol.tradingFee, because value is equal for receiveTradingFee and returnTradingFee. Suppose the trading fee of BTC/USDT is changed from 0.1% to 0.2%. The malicious PartyA monitors the memory pool and notices this tx. The attack process is as follows (steps 1-2 are in the same block):

PartyA deposits a large amount of collateral, calls PartyAFacet.sendQuote, and specifies partyBsWhiteList to prevent it from being locked by other PartyB, and the deadline is block.timestamp. Assuming that the value is 1,000,000, then the trading fee PartyA needs to pay is 1,000,000 * 0.1% = 1000.
The symbol manager calls ControlFacet.setSymbolTradingFee.
PartyA calls PartyAFacet.expireQuote, because block.timestamp is already larger than the set deadline. The quote is successfully canceled and the trading fee returned is 1,000,000 * 0.2% = 2000.

The attacker gets a profit of 1000. In this case, all quotes to cancel will also make a profit.

Now look at the second situation. When new tradingFee becomes smaller, the normal quote is cancelled, then the fee returned to PartyA is smaller than the original fee. This caused a trading fee loss to PartyA.

Impact

As mentioned above, when the tradingFee of a certain symbol is changed, the impacts will be:

Protocol suffers trading fee loss. When the balances[feeCollector] is drained close to 0, all quotes to cancel will revert due to [subtraction underflow](https://github.com/sherlock-audit/2023-06-symmetrical/blob/main/symmio-core/contracts/libraries/LibQuote.sol#L139).
When quote is canceled, PartyA gets less trading fee.

Code Snippet

https://github.com/sherlock-audit/2023-06-symmetrical/blob/main/symmio-core/contracts/facets/control/ControlFacet.sol#L171

https://github.com/sherlock-audit/2023-06-symmetrical/blob/main/symmio-core/contracts/libraries/LibQuote.sol#L125-L132

https://github.com/sherlock-audit/2023-06-symmetrical/blob/main/symmio-core/contracts/libraries/LibQuote.sol#L138

https://github.com/sherlock-audit/2023-06-symmetrical/blob/main/symmio-core/contracts/libraries/LibQuote.sol#L145

Tool used

Manual Review

Recommendation

It is recommended to add a parameter fee to the Quote structure. Assign fee in sendQuote. In this way, the issue caused by the difference between the old and new tradingFee can be avoided.

Duplicate of #92

sherlock-audit / 2023-06-symmetrical-judging

nobody2018 - When symbol manager modifies the tradingFee of a certain symbol via ControlFacet.setSymbolTradingFee, the value returned by LibQuote.returnTradingFee is not the original fee #202