InfinityExchange owner can steal user's tokens via front-running

[code423n4]

Lines of code

https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L1266-L1269 https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L725-L726 https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L775-L776 https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L819-L820 https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L873-L874 https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L1135-L1136

Vulnerability details

Impact

Contract InfinityExchange.sol charges protocol fee through PROTOCOL_FEE_BPS. The issue is that owner of the contract is able to change protocol fee at any time without any restriction which puts him in a very privileged position and allows him to steal user's funds via front-running attack by increasing protocol fee to 100%.

Exploit Scenario:

1. Buyers and sellers orders are being matched through matchOneToOneOrders
2. Owner notices valuable transaction in the mempool and launches front-running attack by updating PROTOCOL_FEE_BPS to 100% through setProtocolFee.
3. Transaction setProtocolFee is being included before matchOneToOneOrders
4. The protocol fee is now 100%
5. Based on calculations in _execMatchOneToOneOrders the remainingAmount sent to seller is 0 and protocol takes all the funds in form of protocol fee.

   uint256 protocolFee = (protocolFeeBps * execPrice) / 10000;
   uint256 remainingAmount = execPrice - protocolFee;

This issue is also relevant in case of owner not being a malicious actor. User accepts some kind of protocol fee for example 2.5%. Then owner just changes the fee to 5%. User didn't expect that change and effectively lost funds.

Proof of Concept

• Update of fee - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L1266-L1269
• Protocol fee applied in _execMatchToOneOrders - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L725-L726
• Protocol fee applied in _execMatchOneMakerSellToManyMakerBuys - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L775-L776
• Protocol fee applied in _execMatchOneMakerBuyToManyMakerSells - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L819-L820
• Protocol fee applied in _execMatchOrders - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L873-L874
• Protocol fee applied in _transferFees - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L1135-L1136

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to add additional parameter to sell - accepted protocol fee (similar to a slippage on a dex) and revert transaction if the fee is higher than the one accepted by user. In addition it is recommended to add validation to setProtocolFee so passed _protocolFeeBps cannot exceed hardcoded value e.g. 5%. The final improvement is adding timelock for updating fee through setProtocolFee.

[nneverlander]

Fixed in https://github.com/infinitydotxyz/exchange-contracts-v2/commit/793ee814d86030477470c81c4f6fda353967a42a

Assessment medium since this assumes mal intent on admin.

[HardlyDifficult]

Dupe https://github.com/code-423n4/2022-06-infinity-findings/issues/259