Accounting for fee-on-transfer tokens like USDT, can mess up the internal balances

Summary

If USDT is upgraded to have a fee-on-transfer feature, it could potentially create significant issues in internal accounting. Currently, the swap() function assumes a 0% fee on transfers between USDT and USD1, where USD1 is pegged to USDT.

Vulnerability Detail

Let's consider the scenario of swapping USDT for USD1. The swap() function involves two steps:

_swapIn(), which transfers USDT to Unitas
_swapOut(), which transfers USD1 back to the user.

The problem lies in the _swapIn() function, specifically in the following code snippet:
```
    if (tokenType == ITokenManager.TokenType.Asset) {
        _setBalance(token, _getBalance(token) + amount);//@audit fee on tranfer fails
        IERC20(token).safeTransferFrom(spender, address(this), amount);
    } 
```
The issue is that _setBalance() is called with the amount parameter, without considering how much USDT has actually been received.

For example:

USDT applies a 1% fee on transfers.
A user wants to swap 1000 USDT.
_swapIn() will execute _setBalance(token, _getBalance(token) + 1000 USDT). However, the contract will only receive 990 USDT due to the 1% fee.

Impact

Internal accounting is messed up

Code Snippet

https://github.com/sherlock-audit/2023-04-unitasprotocol/blob/main/Unitas-Protocol/src/Unitas.sol#L360-L371 https://github.com/sherlock-audit/2023-04-unitasprotocol/blob/main/Unitas-Protocol/src/Unitas.sol#L379-L400

Tool used

Manual Review

Recommendation

In order to address this issue, it is recommended to modify the _swapIn() function as follows:

        if (tokenType == ITokenManager.TokenType.Asset) {
             uint balanceBefore = getReserve(token)
             IERC20(token).safeTransferFrom(spender, address(this), amount);
             uint balanceAfter = getReserve(token);
            _setBalance(token, _getBalance(token) + balanceAfter-balanceBefore);
        }

Also you will need to account how many tokens are minted with _swapOut()

sherlock-audit / 2023-04-unitasprotocol-judging

PokemonAuditSimulator - Accounting for fee-on-transfer tokens like USDT, can mess up the internal balances #125