Incorrect pricing of tokens

Summary

The oracle price is used incorrectly in some places, leading to over/underpriced tokens.

Vulnerability Detail

The function swap() checks the oracle price, and then calculates how many tokens to be burnt/minted. This calculation however is not entirely correct in certain cases, and can lead to over/underpriced transactions.

The oracle used records the prices of the quoteTokens. Thus for a USD1/USDT Pair, the oracle needed will store the price of the USDT token.

Lets assume a USD1/USDT pair is created, with the USD1 as the base token and USDT as the quote token. In the following lines, the oracle takes out the price of USDT.

https://github.com/sherlock-audit/2023-04-unitasprotocol/blob/main/Unitas-Protocol/src/Unitas.sol#L438-L440

This is then passed into the _calculateSwapResult function as a request. Assume we are spending USDT and buying USD1. Thus tokenIn = USDT, and tokenOut = USD1, price = price_of_USDT.

Inside function _calculateSwapResult, lets assume we are swapping with AmountIn specified, thus the code calls the _calculateSwapResultByAmountIn function.

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

In this function, _convert is called. Here, tokenIn = USDT, tokenOut = USD1, price = USDTprice.

Since the fromToken is USDT, which is the quote token, the second if branch is invoked.

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

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

In this function, we see that the fromAmount which is the USDT amount, is divided by the price, which is the USDT price. Thus if the price is lower, the answer of this statement is higher, and returns more USD1.

Thus if the oracle reports a lower value of USDT, the swap will return a higher amount of USD1 for the same USDT. This is demonstrated in the POC below.

Impact

Wrong price calculations.

Code Snippet

The attack is described in the following POC. This is extracted from the test test_swap_WhenMintUSD1WithAmountOut which is already present. Here, the user swaps 1e6 USDT for 1e18 USD1. The oracle price is now artificially changed to 2e18. Thus 1e6 USDT should yield 2e18 USD1, since USDT value is doubled. But the swap function actually mints only 5e17 USD1, basically half the amount. This is because instead of multiplying the price, it divides by it.

function test_ATTACK() public {
        // fee ratio                        : 0
        // price                            : 1
        // spend                            : 1 USDT
        // obtain                           : 1 USD1
        // fee                              : 0 USD1
        uint256 amountIn = 1e6;
        uint256 amountOut = 5e17; // @audit Only 0.5e18 tokens received. SHOULD BE 2e18
        uint256 fee = 0;

        // Update price to 2
        _updatePrice(address(_usdt), 2e18);
        // Adds collateral to pass the reserve ratio checking
        _addCollateral(
            address(_usdt),
            ScalingUtils.scaleByDecimals(1000e18, 18, _usdt.decimals())
        );
        deal(address(_usdt), address(this), amountIn);

        address user = address(this);
        address unitas = address(_unitas);
        uint256 userUSDT = _usdt.balanceOf(user);
        uint256 userUSD1 = _usd1.balanceOf(user);
        uint256 unitasUSDT = _usdt.balanceOf(unitas);
        uint256 surplusPoolUSD1 = _usd1.balanceOf(_surplusPool);

        _unitas.swap(
            address(_usdt),
            address(_usd1),
            ISwapFunctions.AmountType.In,
            amountIn
        );

        emit log_named_uint("usdt price", 2e18);
        emit log_named_uint("usdt spent", userUSDT - _usdt.balanceOf(user));
        emit log_named_uint("usd1 received", _usd1.balanceOf(user) - userUSD1);

        assertEq(
            _usdt.balanceOf(user),
            userUSDT - amountIn,
            "user usdt balance after minted"
        );
        assertEq(
            _usd1.balanceOf(user),
            userUSD1 + amountOut,
            "user usd1 balance after minted"
        );
        assertEq(
            _usdt.balanceOf(unitas),
            unitasUSDT + amountIn,
            "unitas usdt balance after minted"
        );
        assertEq(
            _usd1.balanceOf(_surplusPool),
            surplusPoolUSD1 + fee,
            "surplus pool usd1 balance after minted"
        );
    }

The output of the logs show:

Running 1 test for test/Unitas.t.sol:UnitasProxyUpgradedTest
[PASS] test_ATTACK() (gas: 619512)
Logs:
  usdt price: 2000000000000000000
  usdt spent: 1000000
  usd1 received: 500000000000000000

This shows that the oracle reporting a higher price leads to lower exchange rates.

Tool used

Foundry

Recommendation

In the convert functions, swap the multiplications and divisions.

Duplicate of #48

sherlock-audit / 2023-04-unitasprotocol-judging

carrotsmuggler - Incorrect pricing of tokens #141