Any whitelisted account can break the protocol by blocking setAssetAllowed()

[hats-bug-reporter]

Github username: @8ahoz
Submission hash (on-chain): 0x8711e507759f48a25f42dc1d10858eddeb4d5018c95796f88c4ca1fa197f6293
Severity: high severity

Description:

Description:

Whitelisted addresses that are allowed to create permissionless tranches can create tranches and call claimTrancheId() in LendingPoolConfigurator.sol which increases totalTranches number. The same number later used to iterate through all tranches on the protocol in validateAssetAllowed() which is used in setAssetAllowed() in AssetsMapping.sol

setAssetAllowed() is an important function used to enable assets on whole protocol by onlyGlobalAdmin.
If the totalTranches number is sufficiently high, the loop on AssetsMapping:L68 will revert with OOG.

https://github.com/VMEX-finance/vmex/blob/b0dc00c5dd6bdcac05827128d14dcdc730f19e1c/packages/contracts/contracts/protocol/lendingpool/AssetMappings.sol#L68

Attack scenario:

A whitelisted account that is allowed to create tranches will make a high number of calls to the claimTrancheId() and increase the totalTranches number to a sufficiently high number.
After that all calls to the setAssetAllowed() from the global admin will be blocked because of OOG errors.

[8ahoz]

Small correction: Not all calls to the setAssetAllowed() but calls with isAllowed=false will be blocked. Which means the global admin will lose the rights to disable assets.

[ksyao2002]

Quick worst-case calculation:

for (uint64 tranche = 0; tranche < totalTranches; tranche++) {
            DataTypes.ReserveData memory reserve = lendingPool.getReserveData(asset, tranche);
            //no outstanding borrows allowed
            if (reserve.variableDebtTokenAddress != address(0)) {
                // if the reserve exists in the tranche
                require(
                    IERC20Detailed(reserve.variableDebtTokenAddress).totalSupply() == 0,
                    Errors.AM_UNABLE_TO_DISALLOW_ASSET
                );
            }
            //no outstanding deposits allowed, or else they are unable to withdraw
            if (reserve.aTokenAddress != address(0)) {
                // if the reserve exists in the tranche
                require(
                    IERC20Detailed(reserve.aTokenAddress).totalSupply() == 0,
                    Errors.AM_UNABLE_TO_DISALLOW_ASSET
                );
            }
        }

This is the loop in question. 6 storage slots are loaded with the DataTypes.ReserveData memory reserve. 1 storage slot is loaded for the variable debt token total supply and 1 for the a token total supply. This means 8 storage slots per iteration, which costs 2100 gas each, so each iteration uses 16,800 gas. With a 30 million gas limit, this effectively means you would need 1785 tranches for it to go OOG.

The issue also does not impact the health of the protocol. Even if setAssetAllowed is blocked due to OOG, we don't actually envision using setAssetAllowed very often (especially since we enforce very strict checks on when it can be used, which is only when there is no collateral or borrowing across all tranches). Our strategy to disallow an asset is to slowly decrease the LTV until it's 0, and that would force that asset to essentially be disabled. The isAllowed parameter is moreso used to check if an asset is not allowed, cause an uninitialized mapping would have it be set as false.

Based on how unlikely it is to occur (as we won't be enabling permissionless tranches for the forseeable future, and the amount of tranches that need to be created is quite large), combined with the lack of protocol impact, this issue will not be fixed.

Please let me know your thoughts.

[8ahoz]

Hey @ksyao2002 , thanks for the detailed answer. I mostly agree with your reasoning but here are some points that worth to mention with some proofs.

I agree that setAssetAllowed is not existential as I initially thought. This is mainly because the admin can still enable new assets but it is blocked to disable assets. This definitely downgrades the issue severity as the possible impact is not critical.

My tests show the loop will consume around 40k gas per tranche. This means ~670 tranches will be enough to block calls to setAssetAllowed. I can not know how fast you will reach to that number but I believe it is highly possible if the protocol gets traction.
In case of an attack scenario, the attacker will need to make calls for the remaining numbers. Lets say there are 300 legit tranches, the attacker can attack by sending only < 370 txs which is cheap to achieve in layer 2s.

PoC

Here is an ss that shows how costly it is for 99 tranches(4_445_332 units):

Just add a loop in contracts-deployments.ts

 for (let i = 3; i < 700; i++) {
      await claimTrancheId(`Vmex tranche ${i}`, user1);
      const treasuryAddress = user1.address;

      const [
        assets1,
        reserveFactors1,
        forceDisabledBorrow1,
        forceDisabledCollateral1,
      ] = getTranche1MockedData(allReservesAddresses);

      await initReservesByHelper(
        assets1,
        reserveFactors1,
        forceDisabledBorrow1,
        forceDisabledCollateral1,
        user1,
        treasuryAddress,
        i
      );
    }

and then run Deletes weth from approved tokens test in asset-mappings.spec

---

As discussed before, the issue will block you from going permissionless using setPermissionlessTranches because that means unlimited number of tranches can be created by anybody. Not doing that will still expose the protocol to the same issue because it can happen by natural growth or one of the whitelisted tranche admins can do a griefing attack since they are not trusted as the global admin.

we don't actually envision using setAssetAllowed very often

I don't think the issue is related to the frequency since once the number is high enough you will lose one important feature of the protocol -the ability to disable assets to be used in tranches. It is not a frequency issue, it is more a discussion to either have it or not.

By the hats finances docs:

Medium severity vulnerability description: Attacks that make essential functionality of the contracts temporarily unusable or inaccessible

In my opinion, the issue described above fits to this description as it will block using two functions of the protocol permanently.
admins,

• can not disable assets to be used in tranches
• can not enable permissionless tranches because of the fear of exposing to this vulnerability

Recommendation

I think we have three options here.

• Change your disallow mechanism to only disable deposit & barrow related functions for that asset and users can still withdraw and repay. So that it does not need to go through all tranches to ensure there is no debt or deposits.

• If you are absolutely sure 700 is a really high number for your protocol and total tranche number will never go that high from natural growth, you can cap total number around 600 to block malicious users from inflating numbers, and implement a function to remove malicious tranches from the registry. Not recommended as it increases centralization and you can never know how big it will get.

• Just remove the option to disable assets and revoke the admin's asset disabling rights over third party tranches, if you think it is not essential. In this case I think it is fair to classify this as a Low.

[ksyao2002]

Thanks for your detailed comment and suggestions, they are really good points. I think at this stage, we don't want to make huge changes to the protocol as you suggest in your first recommendation. I think at this stage we are willing to take the risk of 700 as the upper bound, and will use the LTV as our main tool for disabling assets, which essentially accomplishes the same goal as what you state in your first recommendation. But we do appreciate all the work you put into this issue, so I think it's fair to assign this a low rating.