search

sherlock-audit / 2023-04-unitasprotocol-judging

4 stars 3 forks source link

Juntao - Users may not be able to fully redeem USD1 into USDT even when reserve ratio is above 100% #95

Open sherlock-admin opened 1 year ago

sherlock-admin commented 1 year ago

Juntao

high

Users may not be able to fully redeem USD1 into USDT even when reserve ratio is above 100%

Summary

Users may not be able to fully redeem USDT even when reserve ratio is above 100%, because of portfolio being taken into the account for calculation.

Vulnerability Detail

Reserve ratio shows how many liabilities is covered by reserves, a reserve ratio above 100% guarantees protocol has enough USDT to redeem, the way of calculating reserve ratio is Reserve Ratio = allReserves / liabilities and is implemented in Unitas#_getReserveStatus(...) function:

            reserveRatio = ScalingUtils.scaleByBases(
                allReserves * valueBase / liabilities,
                valueBase,
                tokenManager.RESERVE_RATIO_BASE()
            );

allReserves is the sum of the balance of Unitas and InsurancePool, calculated in Unitas#_getTotalReservesAndCollaterals() function:

        for (uint256 i; i < tokenCount; i++) {
            address token = tokenManager.tokenByIndex(tokenTypeValue, i);
            uint256 tokenReserve = _getBalance(token);
            uint256 tokenCollateral = IInsurancePool(insurancePool).getCollateral(token);

            if (tokenReserve > 0 || tokenCollateral > 0) {
                uint256 price = oracle.getLatestPrice(token);

                reserves += _convert(
                    token,
                    baseToken,
                    tokenReserve,
                    MathUpgradeable.Rounding.Down,
                    price,
                    priceBase,
                    token
                );

                collaterals += _convert(
                    token,
                    baseToken,
                    tokenCollateral,
                    MathUpgradeable.Rounding.Down,
                    price,
                    priceBase,
                    token
                );
            }
        }

liabilities is the total value of USD1 and USDEMC tokens, calculated in Unitas#_getTotalLiabilities() function:

        for (uint256 i; i < tokenCount; i++) {
            address token = tokenManager.tokenByIndex(tokenTypeValue, i);
            uint256 tokenSupply = IERC20Token(token).totalSupply();

            if (token == baseToken) {
                // Adds up directly when the token is USD1
                liabilities += tokenSupply;
            } else if (tokenSupply > 0) {
                uint256 price = oracle.getLatestPrice(token);

                liabilities += _convert(
                    token,
                    baseToken,
                    tokenSupply,
                    MathUpgradeable.Rounding.Down,
                    price,
                    priceBase,
                    token
                );
            }
        }

Some amount of USDT in both Unitas and InsurancePool is portfolio, which represents the current amount of assets used for strategic investments, it is worth noting that after sending portfolio, balance remains the same, which means portfolio is taken into account in the calculation of reserve ratio.

This is problematic because portfolio is not available when user redeems, and user may not be able to fully redeem for USDT even when protocols says there is sufficient reserve ratio.

Let's assume :

Unitas's balance is 10000 USD and its portfolio is 2000 USD, avaliable balance is 8000 USD InsurancePool's balance is 3000 USD and its portfolio is 600 USD, available balance is 2400 USD AllReserves value is 13000 USD Liabilities (USDEMC) value is 10000 USD Reserve Ratio is (10000 + 3000) / 10000 = 130%.

Later on, USDEMC appreciates upto 10% and we can get:

AllReserves value is still 13000 USD Liabilities (USDEMC) value is 11000 USD Reserve Ratio is (10000 + 3000) / 11000 = 118%.

The available balance in Unitas is 8000 USD so there is 3000 USD in short, it needs to be obtain from InsurancePool, however, the available balance in InsurancePool is 2400 USD, transaction will be reverted and users cannot redeem.

There would also be an extreme situation when reserve ratio is above 100% but there is no available balance in protocol because all the balance is portfolio (this is possible when InsurancePool is drained out), users cannot redeem any USDT in this case.

Impact

Users may not be able to fully redeem USD1 into USDT even when reserve ratio is above 100%, this defeats the purpose of reserve ratio and breaks the promise of the protocol, users may be mislead and lose funds.

Code Snippet

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

Tool used

Manual Review

Recommendation

Portfolio should not be taken into account for the calculation of reserve ratio.

    function _getTotalReservesAndCollaterals() internal view returns (uint256 reserves, uint256 collaterals) {
        ...
-           uint256 tokenReserve = _getBalance(token);
+           uint256 tokenReserve = _getBalance(token) - _getPortfolio(token);
-           uint256 tokenCollateral = IInsurancePool(insurancePool).getCollateral(token);
+           uint256 tokenCollateral = IInsurancePool(insurancePool).getCollateral(token) - IInsurancePool(insurancePool).getPortfolio(token);
        ...
    }
SunXrex commented 1 year ago

We think it's invalid. it's design decision.

Aditya: The purpose of insurance pool is to support additional USDT requirements in the scenario the reserve pool does not have required amount. To simplify this, we keep only a small portion of assets for yield generation. Priorities : User redemption above yield generation. Ie: Only when there is enough capital in reserve and insurance pool, we take a small portion to invest in delta nuetral defi pools.

ctf-sec commented 1 year ago

Protocol's design choice, not a issue

0xruhum commented 1 year ago

Escalate for 10 USDC This is not just a design choice. There are no specifics on what a "small portion" or "enough capital in reserve and insurance pool" means. Allowing the protocol team to remove collateral from the system to earn yield is a security risk. In the current version, USDT is the collateral for USD1. Without USDT, USD1 is worth nothing. The reserve ratio is used to keep the protocol healthy at all times. By including the funds inside the portfolio in the calculation you allow the protocol to become under-collateralized under certain circumstances.

Considering that #16 describes the risk of the portfolio incurring a loss which is deemed a valid issue, it's reckless to account for the funds inside the portfolio when calculating the reserve ratio. From a protocol perspective, these funds are not part of the system anymore. They were moved into a different protocol to earn yield. That is fine as long as you keep the reserve ratio of Unitas above 130%. For that to be the case, the funds inside the portfolio have to be excluded from the reserve ratio calculation.

To sum up, any funds inside the portfolio should not be used to calculate the reserve ratio. They can be gone at any moment due to the risks described in #16.

EDIT:

I checked the docs on whether each of these duplication issues had to be escalated separately. I didn't find any rules for that. So I decided to bring them up in a single comment. Otherwise, I'd had to create 6 additional escalations without even knowing whether this original issue here is deemed valid. If that's not the case you don't care about the wrong duplication anyways.

sherlock-admin commented 1 year ago

Escalate for 10 USDC This is not just a design choice. There are no specifics on what a "small portion" or "enough capital in reserve and insurance pool" means. Allowing the protocol team to remove collateral from the system to earn yield is a security risk. In the current version, USDT is the collateral for USD1. Without USDT, USD1 is worth nothing. The reserve ratio is used to keep the protocol healthy at all times. By including the funds inside the portfolio in the calculation you allow the protocol to become under-collateralized under certain circumstances.

Considering that #16 describes the risk of the portfolio incurring a loss which is deemed a valid issue, it's reckless to account for the funds inside the portfolio when calculating the reserve ratio. From a protocol perspective, these funds are not part of the system anymore. They were moved into a different protocol to earn yield. That is fine as long as you keep the reserve ratio of Unitas above 130%. For that to be the case, the funds inside the portfolio have to be excluded from the reserve ratio calculation.

To sum up, any funds inside the portfolio should not be used to calculate the reserve ratio. They can be gone at any moment due to the risks described in #16.

EDIT:

  • 14 is not a valid duplicate. If portfolio > collateral the subtraction would revert so there's no underflow there.

  • 43 is not a valid duplicate. Warden argues that _getPortfolio() is not called although it clearly is in the code snipped they provide. The submission makes no sense

  • 90 is not a valid duplicate. Warden argues that a swap will fail if there are not enough funds in the insurance pool. That's desired behavior. You don't want to execute the swap if there are not enough funds for it.

  • 127 is not a valid duplicate. Warden argues that the portfolio amount is accounted for multiple times. But, the insurance pool is a separate contract. The portfolio amount would be 0 for the insurance pool. Thus you don't account for it twice.

  • 129 is not a valid duplicate. The issue itself is valid. The admin is able to leave the protocol under-collateralized by sending to many funds to the portfolio. That's a duplicate of #40

  • 142 is not a valid duplicate. Tokens that are not registered in the TokenManager are not used as collateral. Even if the insurance pool holds these tokens they shouldn't be accounted for since they are not registered collateral tokens. Also, the admin has to make a mistake and send these tokens to the insurance pool since they are the only ones that are allowed to deposit.

I checked the docs on whether each of these duplication issues had to be escalated separately. I didn't find any rules for that. So I decided to bring them up in a single comment. Otherwise, I'd had to create 6 additional escalations without even knowing whether this original issue here is deemed valid. If that's not the case you don't care about the wrong duplication anyways.

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

Will bring for sponsor review, I think this can be a valid medium

thangtranth commented 1 year ago

Escalate for 10 USDC

Reading the original report, I see no mentioning of the potential path or risk that leads to the issue. However in the escalation, the arguments are built upon the insights and finding of another report after submission. If the original report stands alone, I am not sure that it is convincing enough.

This escalation also aims to clarify the rules for other Watsons who may do the same in the future.

sherlock-admin commented 1 year ago

Escalate for 10 USDC

Reading the original report, I see no mentioning of the potential path or risk that leads to the issue. However in the escalation, the arguments are built upon the insights and finding of another report after submission. If the original report stands alone, I am not sure that it is convincing enough.

This escalation also aims to clarify the rules for other Watsons who may do the same in the future.

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.

jacksanford1 commented 1 year ago

@0xruhum I see what you're getting at. But the only way this can be a valid issue is if the protocol has intended for users to always be able to redeem USD1 into USDT when the reserve ratio is 100% or greater. I don't think Unitas has made that claim anywhere? If you see a claim like this (in comments or in code), please show me.

jacksanford1 commented 1 year ago

Update @0xruhum: Based on the following sentence in the README, the broader point of this issue should be true:

The Unitas protocol guarantees unrestricted and unconditional conversion of its unitized stablecoins “back” to USD-pegged stablecoins.

The issue correct gets that "users may not be able to fully redeem USD1 into USDT even when the reserve ratio is above 100%" so this issue should be a valid Medium (maybe even High). But the portfolio aspect is not super unique, so it should be duplicated with any other issues that get the general idea of this vulnerability.

jacksanford1 commented 1 year ago

Escalation accepted

Duplicate of #13

0xruhum commented 1 year ago

But the portfolio aspect is not super unique, so it should be duplicated with any other issues that get the general idea of this vulnerability.

Yep. Just wanted to mention again that the issues I've linked in the original escalation comment aren't valid duplicates tho. That should be taken into account.

jacksanford1 commented 1 year ago

Ok @0xruhum, as far as I'm aware none of them are considered to be duplicates right now.

Jiamincoin commented 1 year ago

Ok @0xruhum, as far as I'm aware none of them are considered to be duplicates right now.

Hey I think #114 should be taken into account too. I didn't create escalation because it was considered duplicate of the main report in the first place.

jacksanford1 commented 1 year ago

Ok @Jiamincoin, I agree that #114 is a duplicate of this issue.

0x00ffDa commented 1 year ago

I do not see any discussion of duplicate #118. Please consider it before ending the escalation evaluations.

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).