search

code-423n4 / 2022-06-infinity-findings

4 stars 0 forks source link

Consistently check account balance before and after transfers for Fee-On-Transfer discrepancies #352

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L239 https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L339 https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L729 https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L745 https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L793 https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L1144 https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L1146

Vulnerability details

As arbitrary ERC20 tokens can be passed, the amount here should be calculated every time to take into consideration a possible fee-on-transfer or deflation. Also, it's a good practice for the future of the solution.

Affected code:

contracts/core/InfinityExchange.sol:
   239:         IERC20(makerOrder.execParams[1]).safeTransferFrom(makerOrder.signer, address(this), protocolFee); //@audit med FOT
   729:     IERC20(buy.execParams[1]).safeTransferFrom(buy.signer, sell.signer, remainingAmount); //@audit fot
   745:       IERC20(buy.execParams[1]).safeTransferFrom(buy.signer, address(this), protocolFee); //@audit fot
   793:       IERC20(buy.execParams[1]).safeTransferFrom(buy.signer, address(this), protocolFee); //@audit fot
  1144:       IERC20(currency).safeTransferFrom(buyer, seller, remainingAmount); //@audit fot
  1146:       IERC20(currency).safeTransferFrom(buyer, address(this), protocolFee); //@audit fot

Recommended Mitigation Steps

Use the balance before and after the transfer to calculate the received amount instead of assuming that it would be equal to the amount passed as a parameter.

As a template:

        uint256 balanceBefore = ERC20(token).balanceOf(address(this));
        ERC20(token).safeTransferFrom(msg.sender, address(this), amount);
        uint256 realReceivedAmount = ERC20(token).balanceOf(address(this)) - balanceBefore;
nneverlander commented 2 years ago

Arbitrary ERC20 tokens can't be used. They have to be pre approved by the contract.

Low

HardlyDifficult commented 2 years ago

Since tokens require approval, and this report does not make clear where things would break down due to a fee on transfer token - closing as invalid.