0xyPhilic - Incorrect math causes loss for the user

[sherlock-admin]

0xyPhilic

high

Incorrect math causes loss for the user

Summary

high

Swapping from USDT to USD1 allows the user to either set an exact amountIn or exact amountOut of tokens to receive. The protocol calculates based on the current USD1/USDT price how much the amountIn or amountOut would be while also incorporating any buyFee or sellFee. The fee is transferred as addition to the necessary amount of USDT and is later minted in the form of USD1 - basically the user pays for the minting of X + Y USD1, where X is his desired/output amount and Y is the fee that is generated on that amount.

Vulnerability Detail

The vulnerability appears when a user tries to mint USD1 with USDT while specifying that he/she wants an exactAmountOut, i.e. exact amount of USD1 to be received. The initial call is made to the swap function, which then internally calls _getSwapResult, which makes a call to _calculateSwapResult within the SwapFunctions.sol to obtain the amountIn, amountOut and fee.

Within the _calculateSwapResult the call is forwarded to _calculateSwapResultByAmountOut() function after the following check:

function _calculateSwapResult(SwapRequest memory request)
        internal
        view
        virtual
        returns (uint256 amountIn, uint256 amountOut, uint256 fee)
    {
        _validateFeeFraction(request.feeNumerator, request.feeBase);

        if (request.amountType == AmountType.In) {
            return _calculateSwapResultByAmountIn(request);
        } else {
            return _calculateSwapResultByAmountOut(request);
        }
    }

Since the request.tokenIn is not the feeToken as it is USDT, the _calculateSwapResultByAmountOut goes into the following block of code:

else {
            // When tokenOut is feeToken, adds the fee before converting the amount
            fee = _getFeeByAmountWithoutFee(amountOut, request.feeNumerator, request.feeBase);
            amountIn = _convert(
                request.tokenOut,
                request.tokenIn,
                amountOut + fee,
                MathUpgradeable.Rounding.Up,
                request.price,
                request.priceBase,
                request.quoteToken
            );
        }

The _convert function enters into the following if else statement:

else if (fromToken == quoteToken) {
            return _convertByToPrice(fromToken, toToken, fromAmount, rounding, price, priceBase);
        }

And _convertByToPrice executes calculations based on the price and amount passed to produce the expected amountIn which includes the fee:

function _convertByToPrice(
        address fromToken,
        address toToken,
        uint256 fromAmount,
        MathUpgradeable.Rounding rounding,
        uint256 price,
        uint256 priceBase
    ) internal view virtual returns (uint256) {
        uint256 fromBase = 10 ** IERC20Metadata(fromToken).decimals();
        uint256 toBase = 10 ** IERC20Metadata(toToken).decimals();
        return fromAmount.mulDiv(priceBase * toBase, price * fromBase,  rounding);
    }

The problem is that the amountIn value returned is incorrect and does not respond to the expected amountIn The following test can be run to showcase the issue:

pragma solidity ^0.8.19;

import "./Unitas.t.sol";

contract IncorrectFee is UnitasTest {

    using MathUpgradeable for uint256;

    address user = vm.addr(0x10);

    function test_incorrectFeeMath() external {
        deal(address(_usdt), address(user), 1000e6);
        deal(address(_usdt), address(_guardian), 1000e6);
        deal(address(_usd1), address(_guardian), 1000e18);
        vm.startPrank(_guardian);
        _usdt.approve(address(_insurancePool), type(uint256).max);
        _insurancePool.depositCollateral(address(_usdt), 1000e6);
        vm.stopPrank();
        vm.startPrank(_timelock);
        ITokenManager.PairConfig[] memory pairs = new ITokenManager.PairConfig[](1);
        pairs[0] = ITokenManager.PairConfig({
            baseToken: address(_usd1),
            quoteToken: address(_usdt),
            buyFee: 10000, // 1% fee
            buyReserveRatioThreshold: 1.3e18,
            sellFee: 10000, // 1% fee
            sellReserveRatioThreshold: 1.3e18
        });
        _tokenManager.updatePairs(pairs);
        vm.stopPrank();
        vm.startPrank(user);
        uint256 initialUSDTBalanceUser = _usdt.balanceOf(user);
        uint256 initialTotalSupplyUSD1 = _usd1.totalSupply();
        uint256 valueOut = 200e18;
        _usdt.approve(address(_unitas), type(uint256).max);
        _unitas.swap(address(_usdt), address(_usd1), ISwapFunctions.AmountType.Out, valueOut);
        vm.stopPrank();
        console.log("Initial USDT Balance of User: ", initialUSDTBalanceUser);
        console.log("Current USDT Balance of User: ", _usdt.balanceOf(user));
        console.log("Initial USD1 Total Supply: ", initialTotalSupplyUSD1);
        console.log("Current USD1 Total Supply: ", _usd1.totalSupply());
        assertEq(_usd1.totalSupply(), valueOut + ((valueOut * 10000) / 10 ** 6));
        assertEq(_usdt.balanceOf(user), 1000e6 - (valueOut / 10 ** 12));
    }

As we can see the buyFee and sellFee is set to 1% for easier calculations. The price of USD1/USDT is also 1:1. Inputting an expected value of 200 USD1 tokens should yield a requirement to pass 202 USDT as follows:

• 200 USDT to be used to mint 200 USD1 (at a 1:1 rate)
• 2 USDT to be paid as fee (used to mint 2 USD1 which are sent to the feeRecipient specified) - 1% of 200 USD1 = 2 USD1 = 2 USDT at 1:1 rate

However the output amounts are greater so the user is practically overcharged in terms of fee.

Impact

Loss of funds for the user as he pays more fee than he is supposed to.

Code Snippet

https://github.com/sherlock-audit/2023-04-unitasprotocol/blob/main/Unitas-Protocol/src/SwapFunctions.sol#L227-L239

Tool used

Manual Review / Foundry

Recommendation

Most likely the issue comes from the decimal difference (USD1 is 18 and USDT is 6). This issue would persist if other tokens with less than 18 decimals are added as collaterals (ex. USDC).

Review and update the math which calculates amountIn based on an expected amountOut to factor in difference of decimals or create wrappers for tokens with less than 18 decimals - i.e. wrap any USDT, USDC to a uUSDT or uUSDC token that is 1:1 and has 18 decimals.

[0xyPhilic]

Escalate for 10 USDC

[sherlock-admin]

Escalate for 10 USDC

You've created a valid escalation for 10 USDC!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[0xruhum]

Given your example: amountOut = 200 fee = 2 price = 1e18 (1:1 conversion) in the _convertByToPrice() function:

• fromToken = USD1
• toToken = USDT

Then you get: 202e18 * 1e18 * 1e6 / (1e18 * 1e18) = 202e6. Meaning, to get 200 USD1 the user has to pay 202 USDT because they have to cover the fees as well.

Looks correct to me.

[0xyPhilic]

@0xruhum I agree that the user has to supply 202 USDT in order to cover the fee, however running the PoC you can see that the final balance of the user in terms of USDT is not 800 (starting from 1000), but a bit less so essentially instead of paying 2 USDT in fee the user is overcharged

[0xruhum]

I didn't run the test so I don't know what the output is. But it shouldn't be 800 anyways since you have to subtract the fee. So 798 would be the correct balance after the swap

[0xyPhilic]

@0xruhum Apologies I wanted to write 798, however the actual balance at the end of the tx is lower than that

[jacksanford1]

Even if this is true, the impact is likely very small. I think there would be two requirements for this to constitute a Medium: 1) Show the POC again using assumptions that cause a large loss to a user at the end, and actually show the amounts lost as well as a terminal output or screenshot (so @0xruhum can see it before running the POC). 2) Would need to then verify that the POC is correct and the assumptions are reasonable.

@0xyPhilic Let me know if you plan to do this.

[0xyPhilic]

@jacksanford1 The loss essentially is not large, however it is consistent and could potentially grow for all users over time. Below I am attaching a screenshot of the above PoC ran showing the difference in terms of what should have been taken from the user and what is actually taken.

The actual amount as mentioned in the previous comments which the user was supposed to pay is 202 USDT essentially leaving the user with 798 USDT (from 1000), however the user balance after the mint is actually: 797979797 (797.979797 USDT) or a loss of 0,02 USDT. However if we assume that 1 million transactions are done this would be a collective loss of all users amounting to 20,000 USDT. Also consider that a single user loses those 0,02 USDT on each tx he does.

[jacksanford1]

Thanks for this @0xyPhilic, really helpful. Based on that loss amount ($20,000) over 1 million transactions, I think it's safe to say this vulnerability isn't material, even if it is true.

@0xruhum @Adityaxrex Is this type of loss acceptable to you? Do you agree that the amount returned would be 797.97 instead of 798?

[lucas-xrex]

The fee calculation is rounded up.

If you want to receive 200e18 USD1, you cannot just pay 202e6 USDT. The amountOut does not include the fee. If you only pay 202e6 USDT, you will only receive 199.98e18 USD1.

202e6 USDT = 202e18 USD1 202e18 - 2.02e18 (1% fee) = 199.98e18 USD1

amount out with fee: (200e18 * 1e6 - 1) / (1e6 - 0.01e6) + 1 = 202.020202020202020203e18 USD1

Convert to amount in with round up: 202.020202020202020203e18 USD1 = 202.020203e6 USDT

Since the decimal places of USDT are smaller than USD1, this is the minimum value required to swap for 200 USD1.

If using 202.020202e6 USDT (202.020203e6 - 0.000001e6) to swap for USD1, you can only get 199.99999998e18 USD1.

202.020202e6 = 202.020202e18 USD1 202.020202e18 - 2.02020202e18 (1% fee) = 199.99999998e18 USD1

[Adityaxrex]

Thank you @lucas-xrex

[0xyPhilic]

@lucas-xrex Thanks for the explanation! Based on the provided information I believe this escalation can be closed.

[hrishibhat]

Result: Low Unique

[sherlock-admin]

Escalations have been resolved successfully!

Escalation status:

• 0xyPhilic: rejected