PRAISE - _getSwapResult() won't use inverted price when buying usd1

[sherlock-admin]

PRAISE

medium

_getSwapResult() won't use inverted price when buying usd1

Summary

_getSwapResult() won't use inverted price when buying usd1

Vulnerability Detail

This is basically caused by the code ordering, because _calculateSwapResult() is done before the price is inverted

 (amountIn, amountOut, fee) = _calculateSwapResult(request); //@audit-info this is done without inverting the price when buying USD1

        _require(amountIn > 0 && amountOut > 0, Errors.SWAP_RESULT_INVALID);

        if (tokenIn == priceQuoteToken) {//@audit-info this is done after the swap result has been calculated.
            // The base currency of oracle price is USD1, inverts the price when buying USD1
            price = request.priceBase * request.priceBase / price;

Impact

the inverted price won't be used when buying USD1

Code Snippet

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

Tool used

Manual Review

Recommendation

re-order the code and have the price inversion

   if (tokenIn == priceQuoteToken) {//@audit-info this should be done before swap result is  calculated.
            // The base currency of oracle price is USD1, inverts the price when buying USD1
            price = request.priceBase * request.priceBase / price;

done before _calculateSwapResult(request); is done.

[Love4codes]

Escalate for 10 USDC

1. i think this should be valid since inverted price is supposed to be used when buying usd1

2. also i think it should be HIGH severity because this will affect the amount of usd1 bought since the inverted price isn't used during swap()

[sherlock-admin]

Escalate for 10 USDC

1. i think this should be valid since inverted price is supposed to be used when buying usd1

2. also i think it should be HIGH severity because this will affect the amount of usd1 bought since the inverted price isn't used during swap()

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.

[ctf-sec]

this is a duplicate of #79

and sponsor has feedback for 79, which apply to this issue as well:

[x] If the price is inverted, the _convertByToPrice function will be called during the swap to divide by the price.

[jacksanford1]

@Love4codes What is your response to ctf-sec's message?

[Love4codes]

@ctf-sec yeah bro it is true that this issue is a dup of #79 but technically it is not.

#79 is wrongly judged as a dup of #48 and YUKI failed to escalate to have judges correct it

sorry i haven't responded to your comment since, i have been offline

[Love4codes]

@jacksanford1 boss after reviewing the codebase again i think what this IF statement does

if (tokenIn == priceQuoteToken) {
            // The base currency of oracle price is USD1, inverts the price when buying USD1
            price = request.priceBase * request.priceBase / price;
        }

is actually different from what the _convertByToPrice does.

 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);
    }

i think the _convertByToPrice relies on the IF statement inverting the price for USD1

The _convertByToPrice just basically divides the fromAmount by the supposed inverted price that's supposed to be done by the above IF statement.

But since the _calculateSwapResult(request) is called before the price is inverted when buying USD1, the non-inverted price (wrong price for usd1) will be used by the _convertByToPrice to divide fromAmount this is due to the code ordering

so i think that is an issue

Considering sponsors feedback: [x] If the price is inverted, the _convertByToPrice function will be called during the swap to divide by the price.

The price won't be inverted at all even tho it is USD1 so _convertByToPrice function will divide fromAmount by non-inverted price for USD1 which is an issue

Also i think since this will affect fromAmount when swapping USD1 i think it can be considered HIGH severity

[jacksanford1]

Thanks @Love4codes.

@ctf-sec What do you think about the difference in those two calcs? Is there a difference? And if so, could it create a High severity issue?

@0xffff11 If you can verify that the calcs are the same/different, would love to hear from you as well.

[SunXrex]

When calling _calculateSwapResult, we determine whether to invoke _convertByFromPrice or _convertByToPrice based on the quoteToken. https://github.com/sherlock-audit/2023-04-unitasprotocol/blob/main/Unitas-Protocol/src/Unitas.sol#L463 The price is inverted only at the end for the purpose of the return value. The Swapped event will record the price.

Example: USD1/USD91 price: 82e18

When swapping from 3.333333333333333333e18 USD91 to USD1, _convertByToPrice is called.

Result 1: 3.333333333333333333e18 * 1e18 / 82e18 = 0.04065040650406504e18 USD1 If we reverse the price first and then call _calculateSwapResult, we don’t need the _convertByToPrice function (division by the price). Instead, we should call _convertByFromPrice (multiplication by the price). USD91/USD1 price: 1e18 * 1e18 / 82e18 = 0.012195121951219512e18

Result 2: 3.333333333333333333e18 * 0.012195121951219512e18 / 1e18 = 0.040650406504065039e18 USD1 Diff: 0.04065040650406504e18 - 0.040650406504065039e18 = 0.000000000000000001e18

By reversing the price before multiplying by the amount, the user may receive slightly less USD1.

[Love4codes]

@SunXrex Hello boss, you're actually giving conflicting feedbacks.

i thought you guys said this: [x] If the price is inverted, the _convertByToPrice function will be called during the swap to divide by the price.

??

[Love4codes]

@SunXrex boss the issue here is that this IF statement in _getSwapResult() function

 if (tokenIn == priceQuoteToken) {
            // The base currency of oracle price is USD1, inverts the price when buying USD1
            price = request.priceBase * request.priceBase / price;

According to your first feedback [x] If the price is inverted, the _convertByToPrice function will be called during the swap to divide by the price.

The Above IF statement is supposed to invert the price before _convertByToPrice is called when buying USD1.

TRUE or FALSE?

[Love4codes]

@SunXrex boss you also said

Result 1: 3.333333333333333333e18 * 1e18 / 82e18 = 0.04065040650406504e18 USD1 _If we reverse the price first and then call _calculateSwapResult, we don’t need the _convertByToPrice function (division by the price). Instead, we should call convertByFromPrice (multiplication by the price). USD91/USD1 price: 1e18 * 1e18 / 82e18 = 0.012195121951219512e18

Boss whether _convertByToPrice function or _convertByFromPrice function is the one to be called with the inverted price that's not the problem.

The price won't be inverted at all, that's the issue

[jacksanford1]

Would like verification from @ctf-sec or @0xffff11 as to whether the inversion math is consistently correct or not.

[ctf-sec]

ok. let me verify, btw https://github.com/sherlock-audit/2023-04-unitasprotocol-judging/issues/79 and https://github.com/sherlock-audit/2023-04-unitasprotocol-judging/issues/102 is the duplicate of this one

[ctf-sec]

I am also inspecting the duplicate

[lucas-xrex]

@Love4codes

Hello.

When calling _convertByFromPrice, the amount is multiplied by the price. When calling _convertByToPrice, the amount is divided by the price. This equation effectively inverses the price.

The price is inversed at the end of _getSwapResult, but it is only used for event logging and statistical purposes. However, it is worth considering removing it because the price can be derived from the ratio of amountIn to amountOut.

Let's explain this with a simpler example: USD1/USD91 oracle price: 82 Quote token: USD91 Fee: 0

When swapping 1 USD1 for USD91, since the toToken is the quote token (USD91), _convertByFromPrice is called, resulting in 1 * 82 = 82 USD91. When swapping 82 USD91 for USD1, since the fromToken is the quote token (USD91), _convertByToPrice is called, resulting in 82 / 82 = 1 USD1.

Under what circumstances would we need to invert the price and then divide by it? Could you provide an example where the calculation of swapping would be incorrect?

Thanks.

[Adityaxrex]

@jacksanford1 can you please help to conclude this one? Thank you

[jacksanford1]

Thanks everyone. Ok, tagging @Love4codes to see the latest response from the protocol team.

[Love4codes]

@jacksanford1 Seen boss, i think i misunderstood the code. But i'm cleared now. Thanks everyone, i really appreciate you guys taking some time out of your tight schedule to go through this.

[hrishibhat]

Result: Low Unique

[sherlock-admin]

Escalations have been resolved successfully!

Escalation status:

• Love4codes: rejected