pool is incompatible with fee-on-transfer tokens

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0x583b3a6802966b63ea69ff06445baf91abc0d509afef998d359eb6198c369eca Severity: medium

Description: Description\ Fee on transfer tokens are not supported in both 2 and 3 token pools.

Attack Scenario\ There are numerous places where FOT tokens will cause discrepancies and can harm the protocol and users:

1. in addLiquidity, balances mapping is increased with the gross amount, without getting the net balance before/after the transfer:

function add_liquidity(uint256[N_COINS] memory amounts, uint256 min_mint_amount) external payable nonReentrant {
        //Amounts is amounts of c-tokens
        require(!is_killed, "Killed");
        if (!support_ROSE) {
            require(msg.value == 0, "Inconsistent quantity"); // Avoid sending ROSE by mistake.
        }
        uint256[N_COINS] memory fees;
        uint256 _fee = (fee * N_COINS) / (4 * (N_COINS - 1));
        uint256 _admin_fee = admin_fee;
        uint256 amp = get_A();

        uint256 token_supply = token.totalSupply();
        //Initial invariant
        uint256 D0;
        uint256[N_COINS] memory old_balances = balances;
        if (token_supply > 0) {
            D0 = get_D_mem(old_balances, amp);//2
        }
        uint256[N_COINS] memory new_balances = [old_balances[0], old_balances[1]];

        for (uint256 i = 0; i < N_COINS; i++) {
            if (token_supply == 0) {
                require(amounts[i] > 0, "Initial deposit requires all coins");
            }
            // balances store amounts of c-tokens
            new_balances[i] = old_balances[i] + amounts[i];//@audit FOT won't work
        }

1. In exchange, balances will be also wrongly changed, since the whole dx is taken in the calculations without accounting for any fees from the tokenIn.

function exchange(
        uint256 i,
        uint256 j,
        uint256 dx,
        uint256 min_dy
    ) external payable nonReentrant {
        require(!is_killed, "Killed");
        if (!support_ROSE) {
            require(msg.value == 0, "Inconsistent quantity"); // Avoid sending ROSE by mistake.
        }

        uint256[N_COINS] memory old_balances = balances;
        uint256[N_COINS] memory xp = _xp_mem(old_balances);

        uint256 x = xp[i] + (dx * RATES[i]) / PRECISION;//@audit (1) From now on all the calculations will be wrong
        uint256 y = get_y(i, j, x, xp);

        uint256 dy = xp[j] - y - 1; //  -1 just in case there were some rounding errors
        uint256 dy_fee = (dy * fee) / FEE_DENOMINATOR;

        // Convert all to real units
        dy = ((dy - dy_fee) * PRECISION) / RATES[j];
        require(dy >= min_dy, "Exchange resulted in fewer coins than expected");

        uint256 dy_admin_fee = (dy_fee * admin_fee) / FEE_DENOMINATOR;
        dy_admin_fee = (dy_admin_fee * PRECISION) / RATES[j];

        // Change balances exactly in same way as we change actual ERC20 coin amounts
        balances[i] = old_balances[i] + dx;
        // When rounding errors happen, we undercharge admin fee in favor of LP
        balances[j] = old_balances[j] - dy - dy_admin_fee;

        address iAddress = coins[i];
        if (iAddress == ROSE_ADDRESS) {
            require(dx == msg.value, "Inconsistent quantity");
        } else {
            IERC20(iAddress).safeTransferFrom(msg.sender, address(this), dx);
        }
        address jAddress = coins[j];
        transfer_out(jAddress, dy);
        emit TokenExchange(msg.sender, i, dx, j, dy);
    }

After that:

1. admin will earn fewer fees, since they are dependent on the difference between balanceOf and balances
2. LPs will unfairly receive more fees since the actual token amount entering the pair contract will be less than the one added to the storage variables

Attachments

1. Proof of Concept (PoC) File
2. Revised Code File (Optional)

in TransferHandler, consider getting the balance of the pair contract before and after the transfers

[omega-audits]

ERC20 is a very loose standard and there is, as far as I know, know claim that Thorn protocol will support all variants.

[blckhv]

Hey, thank you,

Nowhere it's mentioned that only a set of tokens will be supported. Thorn is designed to be a foundational DEX on the Sapphire and it should be able to handle all the different ERC20 tokens, similar to how the Uniswap is. Also, these contracts are forked from Curve and it has support for FOT tokens.

[omega-audits]

Nowhere it's mentioned that only a set of tokens will be supported.

The first words in the blurb on the web site explicitly mention that it is a protocol for stablecoins

[blckhv]

Some of the tokens in the provided configuration script are not stablecoins: link

[dganhnhnh]

Hey, can you provide PoC for this issue? @blckhv