search

sherlock-audit / 2023-04-unitasprotocol-judging

4 stars 3 forks source link

Ruhum - USD1 is priced as $1 instead of being pegged to USDT #48

Open sherlock-admin opened 1 year ago

sherlock-admin commented 1 year ago

Ruhum

high

USD1 is priced as $1 instead of being pegged to USDT

Summary

The system treats 1 USD1 = $1 instead of 1 USD1 = 1 USDT which allows arbitrage opportunities.

Vulnerability Detail

To swap from one token to another Unitas first get's the price of the quote token and then calculates the swap result. Given that we want to swap 1 USD1 for USDT, we have USDT as the quote token:

        address priceQuoteToken = _getPriceQuoteToken(tokenIn, tokenOut);
        price = oracle.getLatestPrice(priceQuoteToken);
        _checkPrice(priceQuoteToken, price);

        feeNumerator = isBuy ? pair.buyFee : pair.sellFee;
        feeToken = IERC20Token(priceQuoteToken == tokenIn ? tokenOut : tokenIn);

        SwapRequest memory request;
        request.tokenIn = tokenIn;
        request.tokenOut = tokenOut;
        request.amountType = amountType;
        request.amount = amount;
        request.feeNumerator = feeNumerator;
        request.feeBase = tokenManager.SWAP_FEE_BASE();
        request.feeToken = address(feeToken);
        request.price = price;
        request.priceBase = 10 ** oracle.decimals();
        request.quoteToken = priceQuoteToken;

        (amountIn, amountOut, fee) = _calculateSwapResult(request);

Since amountType == AmountType.In, it executes _calculateSwapResultByAmountIn():

            // When tokenOut is feeToken, subtracts the fee after converting the amount
            amountOut = _convert(
                request.tokenIn,
                request.tokenOut,
                amountIn,
                MathUpgradeable.Rounding.Down,
                request.price,
                request.priceBase,
                request.quoteToken
            );
            fee = _getFeeByAmountWithFee(amountOut, request.feeNumerator, request.feeBase);
            amountOut -= fee;

Given that the price is 0.99e18, i.e. 1 USDT is worth $0.99, it calculates the amount of USDT we should receive as:

    function _convertByFromPrice(
        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(price * toBase, priceBase * fromBase, rounding);
    }

Given that:

So by redeeming 1 USD1 I only get back 0.99 USDT. The other way around, trading USDT for USD1, would get you 1.01 USD1 for 1 USDT: $1e6 1e18 1e18 / (0.99e18 * 1e6) = 1.01e18$

The contract values USD1 at exactly $1 while USDT's price is variable. But, in reality, USD1 is not pegged to $1. It's pegged to USDT the only underlying asset.

That allows us to do the following:

With USDT back to $1 we get: $1.003009e+23 1e18 1e6 / (1e18 * 1e18) = 100300.9e6$

That's a profit of 300 USDT. The profit is taken from other users of the protocol who deposited USDT to get access to the other stablecoins.

Impact

An attacker can abuse the price variation of USDT to buy USD1 for cheap.

Code Snippet

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

Tool used

Manual Review

Recommendation

1 USDT should always be 1 USD1. You treat 1 USD1 as $1 but that's not the case.

Adityaxrex commented 1 year ago

Aditya: USD1 is pegged to USDT instead of 1 USD. This means the liability and asset accounting is done in USDT value.

SunXrex commented 1 year ago

This should be invalid. because we design converting rate always USD1:USDT to 1:1.

0xruhum commented 1 year ago

Escalate for 10 USDC

You don't treat 1 USDT as 1 USD1. That's exactly what I'm describing here. If 1 USDT = 1 USD1 you wouldn't have to query the price of USDT to calculate the swap. The example calculation in the original issue shows how USDT slightly de-pegging opens up arbitrage opportunities. By USDT losing value you get more USD1. If USD1 were pegged to USDT you'd always get 1 USD1 for 1 USDT no matter the price of USDT.

Also, #79, #102, and #147 are not duplicates of this issue. Only #141 is a valid duplicate.

sherlock-admin commented 1 year ago

Escalate for 10 USDC

You don't treat 1 USDT as 1 USD1. That's exactly what I'm describing here. If 1 USDT = 1 USD1 you wouldn't have to query the price of USDT to calculate the swap. The example calculation in the original issue shows how USDT slightly de-pegging opens up arbitrage opportunities. By USDT losing value you get more USD1. If USD1 were pegged to USDT you'd always get 1 USD1 for 1 USDT no matter the price of USDT.

Also, #79, #102, and #147 are not duplicates of this issue. Only #141 is a valid duplicate.

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 commented 1 year ago

I actually agree with the escalation, https://github.com/sherlock-audit/2023-04-unitasprotocol-judging/issues/79, https://github.com/sherlock-audit/2023-04-unitasprotocol-judging/issues/102, and https://github.com/sherlock-audit/2023-04-unitasprotocol-judging/issues/147 are not duplicates of this issue and this issue itself can be a valid medium

Adityaxrex commented 1 year ago

Liability and asset are both calculated in USDT. Case: 1 USDT = 0.5 USD. Mint 100 USD1 from 100 USDT. Protocol liability = 100 USDT. In nowhere in our calculation we consider dollar value of USDT. If someone is able to make profit by getting cheap USDT, it is not at the cost of the protocol but USDT. It still remains not a valid issue.

jacksanford1 commented 1 year ago

@Adityaxrex Why is oracle.getLatestPrice(priceQuoteToken) being used in the swap request shown in @0xruhum's example above if it isn't being taken into account to swap between USD1 and USDT?

@0xffff11 What is your understanding of this situation?

Adityaxrex commented 1 year ago

@jacksanford1 in this version our protocol's main backing asset is USDT. In all practical purpose we fix 1USDT=1USD1. Even if USDT depegs, we keep 1USDT=1USD1. It does not change accounting whether we check the price or not. This helps us keep the design simple. However, we eventually move towards diversifying to other assets.

0xruhum commented 1 year ago

As shown in the code snippets and the example calculation, the price does have an effect on the swap. While the protocol team's intention might have been to treat 1 USD1 as 1 USDT, the actual implementation doesn't do that.

jacksanford1 commented 1 year ago

Ok, it seems we have a disagreement about how the actual implementation works.

@ctf-sec @0xffff11 Can you please verify if the swap function above is swapping USD1 to USDT (or vice versa) using a fixed 1:1 ratio or if the swap is being done based on the price of USDT returned by Chainlink?

0xffff11 commented 1 year ago

@jacksanford1 I did a deep research on the issue. It seems like it is not redeeming paired 1:1 usd1 for usdt or vice versa. The price of the quote token, in this case usdt, is used for the calculation of the amountOut instead of a 1:1 ratio. So the depeg of usdt, in my opinion, would affect the exchange rate. I think it is a valid issue and as a comment above, most dups are invalid

SunWeb3Sec commented 1 year ago

We keep the swap logic consistent, so instead of hard-coding a 1:1 ratio in the code, we maintain it through our Oracle price updates.

Adityaxrex commented 1 year ago

As mentioned by @SunWeb3Sec we keep the logic consistent and check the price for any pair periodically. However, our feeder logic keeps updating the price for USD1 and USDT price just like any other pair. As mentioned earlier we plan to keep USDT:USD1 = 1:1 for now through our feeder logic. This can be changed in future to reflect the true market price. Thank you

0xruhum commented 1 year ago

That wouldn't work. You price USDT as 1$ in that case without it being worth 1$ if it depegs.

The oracle price is used to calculate the protocol's collateral value: https://github.com/sherlock-audit/2023-04-unitasprotocol/blob/main/Unitas-Protocol/src/Unitas.sol#L512

If you don't update the price to keep the 1:1 ratio with USD1, totalReserves won't reflect the actual value of your collateral. That breaks the reserve ratio calculation.

Adityaxrex commented 1 year ago

@jacksanford1 @SunWeb3Sec the above argument is similar to issue #145. Upto you to decide whether to accept this as duplicate or the other one. Thank you

0xruhum commented 1 year ago

The argument for not keeping the price of USDT hardcoded as $1 is the same yeah. But the two reported issues are different. One describes the issue of assets being frozen because the swap functionality breaks in case the price outside the min/max values. The other shows how price movement in USDT causes the system to mint either more or less USD1 than it should. It's not a duplicate of #145 IMO.

The next time, these kinds of assumptions regarding off-chain mechanics, like the oracle values in this case, should be mentioned in the contest's README. Having them come up during the judging period is annoying for all parties.

Adityaxrex commented 1 year ago

Please see that when user tries to redeem USDT, we also use 1:1. This way we avoid any loss to the protocol. If the user is able to obtain cheaper USDT compared to 1 USD it is at the cost of the USDT seller not the protocol. Thank you

Adityaxrex commented 1 year ago

@0xruhum essentially the minter is able to make profit but the loss is coming from anyone selling USDT for lesser than 1 USD. In terms of USD1:USDT, mint and burn are both at 1:1 so there is no need to worry from the protocol point of view. It can create a scenario that protocol owns a lot of USDT which is worth less because of USDT depeg but to the protocol that is not an issue because we only redeem USDT not USD

0xruhum commented 1 year ago

The value of your tokens depends on the value of your underlying collateral. If the value of USDT falls below $1, your derivative tokens become unreserved. At that point, they will depeg as well. You can't just hardcode the price of USDT as $1. That's what I'm trying to explain here.

Here's a small example: Given that you have a reserve ratio threshold of 100%, e.g. you need 1$ of collateral to mint $1 of your tokens.

Alice mints 100 USD1 using 100 USDT. She then swaps 100 USD1 for 8200 INR (the current exchange rate is 1:82). At this point you have 100 USDT covering 8200 INR. The INR token only has that value because I can use the Unitas protocol to get back the underlying USDT. Otherwise, INR is worth nothing because there's no asset backing it. Now let's say USDT depegs and is only worth $0.9. At this point, you have $90 worth of USDT backing 8200 INR tokens. That doesn't work, right? Because USDT the underlying asset lost 10% of its value, your INR token will also lose value. Its value will now move to 8200 * 0.9 = 7380 INR while the token amount stays the same. Since your whole protocol depends on these derivative tokens mimicking the price of the asset they try to represent, e.g. INR, everything breaks. The actual USD/INR ratio hasn't changed at all. But because your underlying asset depegged, your derivative token also depegs.

Keeping USDT:USD1 at 1:1 won't protect your derivative tokens from depegging. What you do with that is to keep the reserve ratio artificially at a healthy ratio while the real reserve ratio keeps going down. So all the reserve ratio shenanigans of what tokens are supposed to be mintable and burnable at different stages won't really matter.

To sum up, if we assume that your oracle just says that USDT is worth $1 at any time, you've got the problem I've described above. If your oracle returns the correct value for USDT, whether it depegs or not, your swap calculation causes you to mint the wrong amount of USD1 as seen in the original issue.

So no matter what you do, the current design is flawed.

jacksanford1 commented 1 year ago

Agree that this is not a duplicate of #145, it describes a different part of the code as @0xruhum explains above.

Based on @0xffff11's research, it seems like it was not obvious that a 1:1 ratio would be kept here. It was reasonable to assume the oracle would be using the latest trading price of the asset. So that makes this a Medium issue imo because the functioning of this oracle doesn't seem to have been clearly stated as being hardcoded 1:1. Open to correction on this. Here's a statement from the docs which makes it sound like the conversion would be value-based instead of 1:1:

USD91 minters need to provide USDT, the protocol will mint equivalent in value USD91

I'm unclear as to whether the 1:1 hardcoding will cause additional issues in the protocol, but the team is acknowledging that the protocol should function in a way that takes into account the ramifications of hardcoding the price.

0xruhum commented 1 year ago

Yeah, the 1:1 conversion wasn't clearly stated. But, why does that change the severity of the issue?

If we go with the original assumption that the oracle will use the correct trading price of USDT, the swap calculation is broken. Because while using the real trading price for USDT, you continue to treat USD1 as worth $1. As described in the original issue, when the price of USDT drops you're able to mint more USD1 for any given amount of USDT. At the same time you're able to swap USD1 for any of the other derivative tokens at the actual conversion rate of those two assets, e.g. USD/INR. That's a HIGH. The core mechanics of the protocol are broken.

If we assume that the price is hardcoded to 1:1, we have a totally different situation. The reserve calculation breaks which causes the protocol to believe it's fully collateralized while it isn't. Even if USDT drops to $0.9, the protocol continues to treat it as $1 which keeps the reserve ratio above the threshold. But, that doesn't stop the derivative tokens from depegging. Since the value of each derivative token depends on the value of the underlying asset. Again, a HIGH severity issue since another core mechanic of the protocol (distribution of derivative tokens that represent currencies like INR and TRY) is broken.

jacksanford1 commented 1 year ago

@Adityaxrex @lucas-xrex @SunXrex Any response to the second paragraph about the downside of hardcoding the oracle at a 1:1 ratio?

@0xruhum If USD1 is hardcoded 1:1 to USDT, it wouldn't mean that USD91 depegs right? Because the conversion ratio between USD91 and USDT would take into account the fact that USDT is now trading at $0.90?

0xruhum commented 1 year ago

There are no swaps from USDT to any of the other derivative tokens. There's only USDT/USD1. USD1 is then used to trade to the other derivative tokens. The conversion between USD1 and the other derivative tokens will represent the current exchange rate between USD and those other currencies not the actual conversion rate between the tokens themselves.

USDT depegging will cause all the derivative tokens to depeg. The only reason these tokens have any value is because they are backed by an asset with value. USDT is worth $1 because you can exchange it for $1 through Tether no matter what happens (at least that's the idea). That's the reason USDT is pegged at $1.

USD1 is only worth something because it's backed by USDT. I know my USD1 is worth something because I can trade it for USDT which the market agrees is worth X US Dollars. If the price of USDT drops, the price of USD1 will also drop.

USD91 is only worth something because it's backed by USD1. If USD1 loses value, e.g. through USDT depegging, USD91 will also lose value and depeg. Even if I trade my USD91 at the same rate I initially bought it for back to USD1, the asset I get is worth less than at the time of my trade. It doesn't matter that I get the same amount of USD1 back. It's simply not worth the same amount as back then. The same logic applies to all the other derivative tokens as well.

The system here is just one long chain: $1 -> USDT -> USD1 -> USD91. If any of the links break, the whole system will blow up. Meaning, as soon as USDT depegs and doesn't trade for $1, all of your tokens will depeg as well. USDT is the only thing that gave them value.

Adityaxrex commented 1 year ago

We have discussed this internally and concluded that USDT depeg is not an issue for us. Thank you

On Sat, Jul 1, 2023 at 5:23 PM ruhum @.***> wrote:

There are no swaps from USDT to any of the other derivative tokens. There's only USDT/USD1. USD1 is then used to trade to the other derivative tokens. The conversion between USD1 and the other derivative tokens will represent the current exchange rate between USD and those other currencies not the actual conversion rate between the tokens themselves.

USDT depegging will cause all the derivative tokens to depeg. The only reason these tokens have any value is because they are backed by an asset with value. USDT is worth $1 because you can exchange it for $1 through Tether no matter what happens (at least that's the idea). That's the reason USDT is pegged at $1.

USD1 is only worth something because it's backed by USDT. I know my USD1 is worth something because I can trade it for USDT which the market agrees is worth X US Dollars. If the price of USDT drops, the price of USD1 will also drop.

USD91 is only worth something because it's backed by USD1. If USD1 loses value, e.g. through USDT depegging, USD91 will also lose value and depeg. Even if I trade my USD91 at the same rate I initially bought it for back to USD1, the asset I get is worth less than at the time of my trade. It doesn't matter that I get the same amount of USD1 back. It's simply not worth the same amount as back then. The same logic applies to all the other derivative tokens as well.

The system here is just one long chain: $1 -> USDT -> USD1 -> USD91. If any of the links break, the whole system will blow up. Meaning, as soon as USDT depegs and doesn't trade for $1, all of your tokens will depeg as well. USDT is the only thing that gave them value.

— Reply to this email directly, view it on GitHub https://github.com/sherlock-audit/2023-04-unitasprotocol-judging/issues/48#issuecomment-1615773614, or unsubscribe https://github.com/notifications/unsubscribe-auth/AXN5N655U56CPSLAAC4NNRLXN7UAPANCNFSM6AAAAAAZDTL374 . You are receiving this because you were mentioned.Message ID: @.*** .com>

jacksanford1 commented 1 year ago

@0xruhum @Adityaxrex @SunXrex Last question from me:

When USD1 is being converted into USD91 and vice versa, how is the price determined? Is it using the exchange rate between USD and rupees? Or the exchange rate between USDT and rupees?

Adityaxrex commented 1 year ago

It is with with exchange rate using USDT and INR. Thank you

On Tue, Jul 4, 2023 at 10:50 AM Jack Sanford @.***> wrote:

@0xruhum https://github.com/0xruhum @Adityaxrex https://github.com/Adityaxrex @SunXrex https://github.com/SunXrex Last question from me:

When USD1 is being converted into USD91 and vice versa, how is the price determined? Is it using the exchange rate between USD and rupees? Or the exchange rate between USDT and rupees?

— Reply to this email directly, view it on GitHub https://github.com/sherlock-audit/2023-04-unitasprotocol-judging/issues/48#issuecomment-1619388479, or unsubscribe https://github.com/notifications/unsubscribe-auth/AXN5N65W5EKTA3L2WCBBTPLXOOAG7ANCNFSM6AAAAAAZDTL374 . You are receiving this because you were mentioned.Message ID: @.*** .com>

jacksanford1 commented 1 year ago

Ok, in that case I think @0xruhum's statement below would not be true:

The conversion between USD1 and the other derivative tokens will represent the current exchange rate between USD and those other currencies not the actual conversion rate between the tokens themselves.

I think the problem is that @0xruhum assumes everything is based around the value of USD ($1) when in reality it's based around USDT (1 USDT). And so if USDT depegs, then it's correct that everything else (USD1, USD91) depegs from $1 but the system doesn't care because it's all based in USDT. So there's no problem.

This chain from @0xruhum would not be correct:

The system here is just one long chain: $1 -> USDT -> USD1 -> USD91. If any of the links break, the whole system will blow up.

And instead the chain is simply USDT -> USD1 -> USD91.

However, because it was not clear in the code or docs that the Chainlink oracle for USD/USDT would not be used in this specific case, the issue should stay a valid Medium imo. With #141 as a duplicate.

hrishibhat commented 1 year ago

Result: Medium Has duplicates

sherlock-admin commented 1 year ago

Escalations have been resolved successfully!

Escalation status:

jacksanford1 commented 1 year ago

Acknowledged by protocol team (won't fix).