Med: Incompatability with fee-on-transfer tokens

Summary

In case ERC20 token (order.asset) is fee-on-transfer, users will be unable to use reclaimContract() function

Vulnerability Detail

https://github.com/sherlock-audit/2022-11-bullvbear/blob/main/bvb-protocol/src/BvbProtocol.sol#L330-L359

        \\ function matchContract()

        if (order.isBull) {
            bull = order.maker;
            bear = msg.sender;

            makerPrice = order.collateral + bullFees;
            takerPrice = order.premium + bearFees;
        } else {
            bull = msg.sender;
            bear = order.maker;

            makerPrice = order.premium + bearFees; 
            takerPrice = order.collateral + bullFees; 
         } 

         bulls[contractId] = bull; 
         bears[contractId] = bear; 

         // Retrieve Taker payment 
         if (msg.value > 0) { 
             require(msg.value == takerPrice, "INVALID_ETH_VALUE"); 
             require(order.asset == weth, "INCOMPATIBLE_ASSET_ETH_VALUE"); 

             WETH(weth).deposit{value: msg.value}(); 
         } else if(takerPrice > 0) { 
             IERC20(order.asset).safeTransferFrom(msg.sender, address(this), takerPrice); 
         } 
         // Retrieve Maker payment 
         if (makerPrice > 0) { 
             IERC20(order.asset).safeTransferFrom(order.maker, address(this), makerPrice);

In matchContract() function, In case the ERC20 token used as order asset is fee-on-transfer, the actual amount that the contract received may be less than the amount is recorded in order.premium and order.collateral (i.e. different from original intended value)

https://github.com/sherlock-audit/2022-11-bullvbear/blob/main/bvb-protocol/src/BvbProtocol.sol#L434-L438

        \\function reclaimContract()

        uint bullAssetAmount = order.premium + order.collateral; 
         if (bullAssetAmount > 0) { 
             // Transfer payment tokens to the Bull 
             IERC20(order.asset).safeTransfer(bull, bullAssetAmount); 
         }

The result is when a user interacts with reclaimContract() function, then bullAssetAmount is calculated as same as above, but the contract recieved less tokens thatn intended, that may make unable for users to withdraw their funds.

Consider Below:

Token X is fee-on-transfer and it took 10% for each transfer. Maker Price is 1000 token X and Taker price is 2000 token X. Now, the amount stored for token X after matchContract() function call for variables order.collateral is 1000 and order.premium is 2000. But since token X has 10% fee, Contract only receives 2700 token X i.e. bullAssetAmount is 2700, the contract logic thinks it to be original i.e. 3000. Now, As the contract will have some lack of token X to fufill the transfer, the transfer may fail making contract stale.

Impact

In case ERC20 token (order.asset) is fee-on-transfer, users will be unable claim asset amount making contract stale for users and halting withdrawals.

Code Snippet

https://github.com/sherlock-audit/2022-11-bullvbear/blob/main/bvb-protocol/src/BvbProtocol.sol#L330-L359 https://github.com/sherlock-audit/2022-11-bullvbear/blob/main/bvb-protocol/src/BvbProtocol.sol#L434-L438

Tool used

Manual Review

Recommendation

Compare pre-/after token balances to compute the actual transferred amount. Things will be more complex with rebasing tokens.

Duplicate of #130

sherlock-audit / 2022-11-bullvbear-judging

0v3rf10w - Med: Incompatability with fee-on-transfer tokens #123