search

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

4 stars 0 forks source link

`protocolFee` may cause DoS #228

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityExchange.sol#L793 https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityExchange.sol#L899 https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityExchange.sol#L1146

Vulnerability details

Impact

Some ERC20 tokens will fail when transferring 0 amount. Transactions sent by users may be reverted if protocolFee is set to 0.

Proof of Concept

If protocolFeeBps is set to 0, protocolFee will be 0:

https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityExchange.sol#L873

    uint256 protocolFee = (protocolFeeBps * execPrice) / 10000;

These lines will transfer 0 amount:

https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityExchange.sol#L793

      IERC20(buy.execParams[1]).safeTransferFrom(buy.signer, address(this), protocolFee);

https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityExchange.sol#L899

      IERC20(buy.execParams[1]).safeTransferFrom(buy.signer, address(this), protocolFee);

https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityExchange.sol#L1146

      IERC20(currency).safeTransferFrom(buyer, address(this), protocolFee);

Tools Used

None

Recommended Mitigation Steps

Consider setting a lower bound in setProtocolFee, and also PROTOCOL_FEE_BPS should have a cap (max: 10,000).

nneverlander commented 2 years ago

Thanks

HardlyDifficult commented 2 years ago

Since the admin could correct the fee it set in error, lowering risk and merging with the warden's QA report #232