Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Aave V3 Roles Logic Inconsistency #767

Open
oneski opened this issue Dec 13, 2022 · 2 comments
Open

Aave V3 Roles Logic Inconsistency #767

oneski opened this issue Dec 13, 2022 · 2 comments

Comments

@oneski
Copy link

oneski commented Dec 13, 2022

Flagging a potential design flaw in the ASSET_LISTING_ADMIN Role.

Problem:

In Aave V3 we have several roles that allow an address to take certain actions, pertinent to this situation are:

  • RISK_ADMIN, which can edit risk parameters and has access to the following methods:

    setGracePeriod

    setReserveBorrowing

    configureReserveAsCollateral

    setReserveStableRateBorrowing

    setReserveFreeze

    setBorrowableInIsolation

    setReserveFactor

    setDebtCeiling

    setBorrowCap

    setSupplyCap

    setLiquidationProtocolFee

    setEModeCategory

    setAssetEModeCategory

    setUnbackedMintCap

    setReserveInterestRateStrategyAddress

    setSiloedBorrowing

  • ASSET_LISTING_ADMIN, which can list assets in a market and has access to the following methods:

    setAssetSources

    setFallbackOracle

    initReserves

The potential inconsistency is with the goal of ASSET_LISTING_ADMIN and the access it actually has. In order to fulfill its designed goal of “being able to list assets in a market” the functions it has access to are insufficient. At the end of initReserves, an asset has an LTV of 0% and is unborrowable, while “functionally listed” without additionally granting the ASSET_LISTING_ADMIN the powers given by RISK_ADMIN there is no way to “properly list” an asset simply by having the ASSET_LISTING_ADMIN permission. At the end of the initReserve call, there is nothing the listed asset can do, it can’t be used as collateral, it can’t be borrowed, there is no supply or borrow caps, no isolation or silo mode setting. This is a particularly bad and potentially dangerous situation. There is a reason ALL asset listings initialize risk parameters in addition to initializing the reserves.

The current design requires granting RISK_ADMIN permission to any potential candidate for ASSET_LISTING_ADMIN, this is a very powerful privilege that allows the address to edit the risk parameters across the whole market at any time. This is an unnecessary power to grant an ASSET_LISTING_ADMIN, who only needs the ability to initialize risk parameters during the listing (which their role would imply they can do).

Suggestion:

  • create a new function on PoolConfigurator called initReserveAndSetParams which allows a ADMIN_LISTING_ADMIN to be able to pass in the initial risk parameters in addition to the parameters needed to initialize a reserve.
  • This can be accomplished by creating another contract that has both permissions (RISK_ADMIN & ASSET_LISTING_ADMIN), and allows any ASSET_LISTING_ADMIN to call a function initReserveAndSetParams which then calls the necessary functions on PoolConfigurator.

Current Mitigation:

  • When granting roles in tandem, thoroughly check to ensure any potential ASSET_LISTING_ADMIN has no way of abusing the additional RISK_ADMIN permissions.
    • Consider giving guardians ROLE_ADMIN permission to be able to revoke permissions if abused
@oneski
Copy link
Author

oneski commented Dec 13, 2022

Strongly prefer the second suggestion of creating separate periphery contract to handle the initReserveAndSetParams. Requires no changes to core code, and be easily added to any deployment (governance grants needed role to the periphery contract).

@miguelmtzinf
Copy link
Contributor

It feels a bit off that the ASSET_LISTING_ADMIN can add new oracles to the system, list new assets but cannot make them usable (giving risk params). It would make sense to let this role configure assets as collateral/borrowable (or even risk params), but then it would partially overlap with the RISK_ADMIN role?

Since there is no clear strategy about which entities are supposed to hold the role of ASSET_LISTING_ADMIN, I suggest to keep it as is. As @oneski mentions, having a periphery contract granted as ASSET_LISTING_ADMIN and RISK_ADMIN could nicely work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants