Fee-on-transfer check can be avoided

[code423n4]

Handle

harleythedog

Vulnerability details

Impact

In Exchange.sol, there is a check in addLiquidity to see if the base token is a fee-on-transfer token (and it reverts if it is, since this is not supported and would break a lot of the internal accounting):

bool isExchangeEmpty = IERC20(baseToken).balanceOf(address(this)) == 0;
...
if (isExchangeEmpty) {
    require(
        IERC20(baseToken).balanceOf(address(this)) ==
            tokenQtys.baseTokenQty,
         "Exchange: FEE_ON_TRANSFER_NOT_SUPPORTED"
    );
}

This check can easily be avoided by transferring 1 wei of the base token to the contract before supplying the initial liquidity. This would create an exchange with a fee-on-transfer base token, which is not desirable and new users might interact with these exchanges, not knowing that the exchange logic will not work correctly and thus should not be interacted with.

Proof of Concept

See referenced code here.

Tools Used

Inspection.

Recommended Mitigation Steps

Instead of checking if balanceOf is 0, the contract should check that the internalBalance is 0. So I recommend changing line 123/124 to be:

bool isExchangeEmpty = internalBalances.baseTokenReserveQty == 0;

This way, there is no way of supplying initial liquidity an exchange that has fee-on-transfer tokens as the base token.

[0xean]

Fee on Transfer Tokens are NOT supported in our current implementation is stated in the readme of the contest.

Yes, the check could be gamed, but we are not trying to avoid people gaming the system, we are trying to simply add a check to help people making an honest mistake.

Would suggest a low severity on this given FOT are not supported and its called out as such in the README.

[GalloDaSballo]

I agree with both sides, the finding is valid and Low Severity is appropriate given that feeOnTransfer tokens are out of scope and explicitly not supported by the protocol