We audited the radiant-capital/riz repository originally at the v0.0.1-alpha tag, and later, after changes were introduced, at the 65ca7bb commit.
In scope were the following files:
src/
├── interfaces/
│ ├── Riz/
│ │ ├── IRizAToken.sol
│ │ ├── IRizLendingPool.sol
│ │ └── IRizLendingPoolAddressesProvider.sol
│ ├── IAggregatorV2V3.sol
│ ├── IBaseStrategy.sol
│ ├── IOracleRouter.sol
│ ├── IPyth.sol
│ ├── IPythEvents.sol
│ ├── ITokenizedStrategy.sol
│ ├── IYVaultFactory.sol
│ ├── IYVaultV3.sol
│ └── PythStructs.sol
├── leverager/
│ └── RizLeverager.sol
├── libraries/
│ ├── swap-strategies/
│ │ └── DexSwapStrategy.sol
│ ├── EmergencyWithdraw.sol
│ └── Errors.sol
├── riz-lending/
│ ├── RizLendingPool.sol
│ ├── RizLendingPoolAddressesProvider.sol
│ ├── RizLendingPoolConfigurator.sol
│ └── RizLendingPoolStorage.sol
├── safety-module/
│ └── RizSafetyModule.sol
├── vaults/
│ ├── reference/
│ │ ├── BaseStrategy.sol
│ │ └── TokenizedStrategy.sol
│ └── YRizStrategy.sol
├── tokenization/
│ └── RizAToken.sol
├── BadDebtManager.sol
├── OracleRouter.sol
├── RevenueManagement.sol
├── RizLockZap.sol
└── RizRegistry.sol
The "Riz" system is a part of Radiant known as the "Radiant Innovation Zone". Its general purpose is to allow the creation of new lending markets that are isolated from each other. This allows Radiant to create markets which include assets which are riskier, either due to price movement, liquidity or smart contract bugs or behavior. These lending markets are based on Aave V2 and utilize the overcollateralized lending pattern, along with liquidations which can be performed when users' borrows are too high.
Lending pools can be deployed via a contract factory pattern, so there are also contracts which manage these deployments and track all relevant information such as which lending pools are still valid and active.
Since Riz markets are intended to take on riskier assets, there is an "emergency shutdown" feature in case a market suffers from an issue. These issues are not strictly defined, but could include loss of liquidity or quick price movements making borrows unliquidateable. Emergency shutdown is triggered by the "Emergency Admin" role and immediately stops all pool operations, such as depositing and borrowing, but also including liquidations and transferring rTokens. Additionally, it takes a "snapshot" of the market's state at the time of shutdown, and this snapshot can be used to then deduce how much each user is owed after the shutdown. After the market has been "shutdown", the withdraw functionality becomes the "emergency withdrawal" one, which can be called once per user which transfers their owed amount to them. After the snapshot, each user's balance may receive the same percentage slash in order to be able to fairly compensate users after an incident and debt socialization.
Users may utilize "referral codes" when interacting with the protocol, which simply emit events for off-chain use. It should be noted that the value 0 codes for "no referral code", so when referral codes are being assigned it should be made clear that 0 is not available. Referral codes come from Aave originally, and it appears that currently the Radiant team does not have any plans for implementing referral codes.
At the moment, only Chainlink and Pyth oracles have been included to supply the assets' prices in the system. This information is used to compute health factors for accounts and the fair amount owed to users after snapshots.
A Zapper contract exists to help users easily transfer their Riz market assets into LP tokens within the Radiant system, to be eligible for Radiant rewards.
There is a leverager contract so that users can seamlessly loop their positions into a leveraged one, by atomically borrowing and redepositing collateral in a single transaction.
There is a "YRizStrategy" contract, based on Yearn.finance strategies, which allows users to deposit collateral and automatically re-invest dividends. This is controlled by the radiant team and allows users to spread their deposits of some asset across multiple lending pools.
Finally, there is a "revenue management" contract which receives system profit and swaps it through a Dex into some predetermined basket of more liquid "blue-chip" assets.
Stable borrowing, while a part of the system, is turned off by default when pools are launched. Radiant has stated an intention to never allow stable borrowing, though the stable-borrowing functionality still exists in the code and can technically be turned on. Stable borrowing is simply borrowing some asset in a lending pool at a stable, pre-fixed interest rate.
The system has many privileged Roles with various powers. In general, these roles are trusted not to abuse their power in order to profit from the system. This could be done in a few cases by setting addresses which receive funds or by setting up interactions with decentralized exchanges which can be sandwich-attacked.
Generally speaking, all privileged roles in the system are EOAs or multi-sig accounts controlled by Radiant.
One of the most powerful contracts within the system is the RizRegistry contract, which is able to initialize new pools and set their "Pool Admin" addresses. The Pool Admin role is able to set up new reserves in a lending pool, update debt token implementations, update rToken implementations, and modify which reserves are active or enabled as collateral. They also can set pool parameters such as borrow cap, supply cap, and interest rate strategies. Since the RizRegistry owner assigns the Pool Admin role, the RizRegistry owner thus has some degree of power over all of these actions as well. Generally speaking, the Pool Admin is trusted to:
Never enable stable rate borrowing. The RizRegistry owner is trusted to choose Pool Admins who can be trusted
Set correct addresses for the Leverager contract, Bad Debt Manager contract, and proxy implementations throughout the system. Most relevant contracts in the system are proxies and can be changed at will.
The RizLockZap contract is relatively isolated, and has an owner role. The owner of RizLockZap is trusted to:
RizLockZap is trusted to set swap routes which are efficient and do not result in sandwiching opportunities or other profit-making opportunities for themselves.The OracleRouter contract has an owner role which is trusted to:
heartbeat and fallback.The RevenueManagement contract has both an owner and a whitelist (composed of many addresses) with special permissions. The owner is trusted to:
RevenueManagement during emergencies. Ideally, these should be pre-defined.inputTokenOracle values to the OracleRouter contract.The "whitelisted" users within the RevenueManagement contract are trusted to:
The YRizStrategy contract has the roles management, keepers, emergencyAuthorized and authorized. The management role is trusted to:
The keeper role within the YRizStrategy contract is trusted to:
tend and report regularly.YRizStrategy arbitrarily, so this is an especially important responsibility.The emergencyAuthorized role within the YRizStrategy contract is trusted to:
The authorized role within the YRizStrategy contract is trusted to:
The owner of the BadDebtManager contract has the ability to affect post-shutdown repayment parameters. These directly affect how much a user will receive after a pool has been shut down, and they can be updated anytime by the owner. Thus, the owner is trusted to:
setParamsForEmergencyWithdrawals function in the case that the snapshot functionality fails for some reason.Generally, all privileged roles are expected to be available at any time for emergency actions, and are expected to monitor all external integrations for good behavior (for example, ensure that all swaps performed by the protocol are through the optimal swap path). We encourage the Radiant team to establish both automated monitoring to help detect emergencies, as well as track information coming from oracles for correctness and track all swaps to identify potential sandwiching or arbitrage against the protocol. We also encourage Radiant to define all known sources of emergencies which could result in shutdowns or pauses on different contracts, and to monitor for these on-chain utilizing a tool such as Forta or Defender.
We also recommend implementing Timelocks on privileged actions, so that users cannot be surprised or adversely affected by changes to different system settings.
Finally, we recommend that any upgrades to proxies are thoroughly tested and scanned for correct storage layout before upgrading. We encourage fork-testing to ensure that the upgrade process will work. And we encourage Radiant to verify all contracts on Etherscan and relay this information to their community well in advance of performing upgrades.
Note that for the purpose of this audit, it was assumed that stable rate borrowing would not be enabled on any RizLendingPool instance.
swapTokens Function Is UnusableThe _depositToCoreLendingPool function has a nonReentrant modifier. However, it is only called by the swapTokens function which also has a nonReentrant modifier. As a result, the second nonReentrant modifier will revert since it would detect that the contract has been re-entered. This means that the swapTokens function will never execute successfully.
Consider removing the nonReentrant modifier from the internal _depositToCoreLendingPool function. Additionally, consider implementing unit testing to ensure that the function executes properly.
Update: Resolved in pull request #60 at commit c35716e.
emergencyWithdrawIn the emergencyWithdraw function, the check which prevents double withdrawals is at the top of the function. However, the value representing "has withdrawn" is only set at the bottom of the function, after the assets are transferred to the user. Importantly, since RizATokens are not burned at this step, if the underlying asset has a hook on transfer, a user could potentially reenter this function and withdraw again, multiple times.
Consider setting the user's withdrawn status directly below the check at the top of the function. Alternatively, consider implementing a non-reentrant modifier onto the emergencyWithdraw function. Finally, consider vetting all tokens and ensuring that any tokens added to the Riz system do not have hooks on transfer and cannot be upgraded to have them in the future.
Update: Resolved in pull request #61 at commit bd13afe. The emergencyWithdraw function now sets the status on the holder just after checking it.
The RevenueManagement contract implements the functionality to allow whitelisted users to swap tokens based on the inputs and the predefined configuration of the contract. Its inputs consist of an array holding the trade amounts going in, the asset tokens, the minimum expected in exchange, the output token, the strategy and some bytes data. The first step performed is to calculate the total value going into the trade using the array of input assets and their amounts. Later, the DEX is called, passing all the inputs and the bytes data. However, in the DEX, only the bytes data is used to perform the swap operations.
This means that there is a disconnection between the inputs passed to the swapTokens function and what the DEX will be using for the actual swap. Moreover, the rest of the inputs passed to the DexSwapStrategy library can be independently used as part of the attack to grant allowance on some other asset stored in the contract to then proceed with the attack. Even though the entry point is access-controlled, any whitelisted user could take advantage of that power and craft the data (and the right conditions, such as creating a pair for the swap using a token controlled by them and the desired asset) to get funds from the contract.
Consider constraining the encoding of the inputs passed to the swapTokens function to prevent the possibility of crafting undesired routes and to reduce the attack surface of the functionality.
Update: Acknowledged, not resolved. The Radiant team stated:
We acknowledge this issue but believe that there is a low chance of anything malicious happening due to the restrictions already imposed. The only addresses which are granted whitelist permission are the addresses we call for the swaps. In our case, these are 1inch's
AggregationRouterand Paraswap'sAugustusSwapper. We also revoke approvals before the end of the function.
The RevenueManagement contract implements functionality which allows users to swap the tokens deposited in the contract for a specific list of output tokens with pre-defined ratios. The sum of such ratios must equal 10_000 (or 100%) during the setup.
However, when computing that sum, the setOutputTokensConfig function does not take into consideration the possibility of having repeated elements in the _outputTokens input array. As a result, two problems arise:
To prevent erroneous transactions during the swap after a faulty setup is not caught by the current validation, consider checking that there are no repeated elements in the _outputTokens input array.
Update: Resolved in pull request #63 at commit a98e99e.
Distributions May Still Exist After UpdatingWithin the YRizStrategy contract, the setFullPoolDistributions function changes the content of the _distributions mapping via the _setPoolDistribution function.
However, in the event that newDistros.length is less than the number of previous elements in the _distributions mapping, the elements which exceed the length of newDistros will never be overwritten nor deleted in the _setPoolDistribution function.
This means that if there is a reconfiguration of distributions where the number of distributions decreases, some of these old distributions may still appear valid, and functions such as getUiKeeperPoolBalances, getAllPoolDistributions, and _deployFunds may behave unintuitively and may move funds in unexpected ways.
Consider deleting Distributions which are no longer used, and setting both the pool and bps values to address(0) and 0, respectively. This will prevent old invalid pools from receiving distributions, and will help detect invalid pools early in loops.
Update: Resolved in pull request #100 at commit e15ba56.
The _checkNoLiquidity function checks the balance of the underlying asset in the aToken contract. This is paired with a check on the liquidity rate being zero. The _checkNoLiquidity function is used when setting a reserve's liquidation threshold to 0 and when setting a reserve to "inactive". The goal of this function appears to be to check whether a reserve is currently unused. However, in the case that all deposited assets are currently borrowed, the balance check can still return 0. In this case, a reserve may be deactivated or removed as a collateral while it still has borrows. If it is deactivated, those borrows will not be repayable due to the check of isActive within validateRepay.
Consider changing the logic of _checkNoLiquidity to check that there are no open borrows and that no assets are currently deposited.
Update: Acknowledged, not resolved. The Radiant team stated:
We have thoroughly reviewed this issue and acknowledge it. We have decided not to take any action because reserve deactivation will be unlikely in our case since Riz will mostly have two asset-pairs only.
The RizLeverager contract implements functionality allowing users to loop over their positions. To do this, users must specify several parameters including the number of loops, the borrow ratio for each loop, and the amount. However, by having control over these values, a user might pass a high loop count with a small amount value and a borrow ratio of 100%. As a result, due to how fees are calculated on line 209 and line 227, if the amount value is small enough, the fee will be zero for all the operations. It is worth noting that even though the fee would be zero, the gas cost associated with the process will increase. Nonetheless, this attack might be more potent in networks that have a lower gas fee which could result in a break-even net result.
Consider always rounding up the fee calculations.
Update: Resolved in pull request #88 at commit ba21d2e.
In the bytesToSwapData function of the DexSwapStrategy contract, there is only a single, insufficient check on rawData.length. This check ensures that rawData.length >= 96. However, later on in the function, it is assumed that rawData.length >= 160. In the case that rawData.length is somewhere between 96 and 160, the subtraction will fail silently.
Consider changing the initial check to check that the length is at least 160. Alternatively, consider handling inputs whose length is shorter than 160.
Update: Resolved in pull request #81 at commit 4d61055. The length validation now uses 160.
getAllPoolDistributionsWithin the YRizStrategy contract, the getAllPoolDistributions function may return data which is not useful. This data could be interpreted incorrectly by contracts that call the getAllDistributions function and lead to unexpected behavior.
The problem arises when an element of the _distributions mapping has the pool value set to 0. This should generally correspond to an invalid or uninitialized distribution. Once this is detected, the loop which is constructing the return value of getAllPoolDistributions will break, but crucially, the returned distributions array will contain the invalid configuration at its end. Note that this is only the case if the _distributions array contains less than maxRizPools number of configurations.In this case, an invalid set of Distributions will be returned, which may result in an integration misinterpreting this as correct data.
Consider first checking whether the _distributions element is initialized, and then writing to distributions, rather than doing it in the opposite order.
Update: Resolved in pull request #52. The Radiant team stated:
Added a variable which will hold all the valid distributions. We loop through those instead of the max Riz pools.
In the swapTokens function, the computeInputValue function only totals the values of swap inputs which are ERC20 tokens. It does not include the value field of data, which will be sent when performing the swap.
Since the call within DexSwapStrategy performs an arbitrary call, it can have any target and any parameters. So, it easily could be used to receive ETH and send it to a different address.
However, this is complicated by the fact that the RevenueManagement contract has no payable functions.
Consider including a receive function within RevenueManagement and including native assets within the total input value computations. Alternatively, consider entirely removing the ability to send the native asset within the DexSwapStrategy contract.
Update: Resolved in pull request #53. The Radiant team stated:
Removed the ability to send the native asset in the swap and added an
ifcheck to make sure the value!= 0.
Within the codebase, there are many places where ERC-20 tokens are transferred either into or out of the protocol. In the case that they are being transferred in, there is a chance that some behavior results in fewer tokens than expected being transferred. This could be the case if the token charges a fee on transfer.
The following places have transfers without corresponding checks on the amount transferred:
RizLendingPoolRizLockZapRizLockZapRizRegistryRizLeveragerRizLeveragerRizLeveragerRizLendingPoolTokenizedStrategyConsider implementing a balance check before and after each of the above transfers, and confirming that the desired number of tokens have been received.
Update: Acknowledged, not resolved. The Radiant team stated:
Acknowledged. We think we can accept that transactions are reverting in case there is not much liquidity to transfer.
SafeLibalso has some descriptive error messages in case a transfer fails.
In the RizLendingPool contract, within the withdraw and the borrow functions, there are no checks which confirm the existence of liquidity in the market. In the case that a user attempts to withdraw or borrow more assets than the pool has, the transaction is likely to revert upon transferring to the user. However, this may not be obvious to users. In addition, since tokens can implement arbitrary logic, it is possible that the call does not revert but the user receives significantly less than expected.
Consider implementing a check at the beginning of these functions to ensure that the desired withdrawal or borrow amount is available in the lending pool.
Update: Acknowledged, not resolved. The Radiant team stated:
We think that we have never had any checks for token balances during deposits/withdrawals. Aave v2 does not have them either. We think we can accept that transactions are still reverting in case there are not enough token balances.
_setAssetSourceWhen the owner sets the parameters for a feed using Pyth, there is a check being done over the feedId and feedAddress. However, because they are using an &&, if the feedAddress is zero, then the feedId would be zero as well and the call WILL NOT revert as there are no Pyth price fees for a feedId of 0.
Consider correcting this check to be meaningful and disallow the feedId from being 0.
Update: Resolved in pull request #80 at commit cf84c06. The Radiant team stated:
Removed the check for CL feed address when we set Pyth in the Router.
addPoolWithin the RizRegistry contract, the addPool function only checks that LENDING_POOL_ADDRESSES_PROVIDER and BAD_DEBT_MANAGER are initialized. However, this function also depends on LENDING_POOL, LENDING_POOL_CONFIGURATOR, and BAD_DEBT_MANAGER being initialized in implementations.
Since all of the aforementioned initializations are necessary, consider including them in the check at the beginning of the addPool function.
Update: Resolved in pull request #66 at commit bac3658.
Within YRizStrategy.sol, the setFullPoolDistribution function allows a new set of "distributions" to be set up for the strategy. Old distributions can be removed and new distributions can be added. Inside the _checkPoolsWithBalanceAreIncluded function, pools where the YRizStrategy contract has a balance cannot be removed. If any pool which is being removed has a balance at this point, the entire call will revert. This can be taken advantage of by a bad actor to delay or prevent changes to the distribution set. Any user may front-run calls to setFullPoolDistribution with a transfer of tokens from a pool which is being removed, and the entire pool distribution change will be stopped. Even in chains where front-running is not possible, a user may be able to simply transfer a single token every time they detect the balance of the strategy contract is 0. Note that a user may have many incentives for doing this, such as keeping interest rates low or high for certain pools (by preventing strategy deposits or withdrawals) or may desire to keep the bps for some pool at a certain level. Note that this front-running stops changes to all pools, not just the one which is associated with the transferred tokens.
Consider implementing an automated withdrawal from the removed pools, rather than a reversion when its balance differs from zero.
Update: Resolved in pull request #100 at commit e15ba56.
When a market is shutdown, the updateLendingPoolStatus function from the RizRegistry contract is called. In it, the implementation checks if the addressProvider address is a valid address in the _isValidAddressProvider mapping and whether the pool was registered in the _lendingPools mapping, and proceeds to set it to the false state. However, since the flag in the _isValidAddressProvider mapping is not set to false, a "skipping" check done in the setImplementation function will never skip a single element. This will result in modifying the implementations of proxies for shutdown pools.
Consider setting the _isValidAddressProvider mapping to false when status == false within updateLendingPoolStatus and handling that state when used.
Update: Resolved in pull request #89 at commit d2728a7.
The OracleRouter contract implements the receive function used for getting the ETH for paying the fees in Pyth. As the updateUnderlyingPrices method is not payable, the caller needs to first send a transaction with ETH through the receive function and then call the latter method. However, because the operation is not atomic, the transferred ETH can be back-run to then update another market instead, spending the original caller's ETH.
Consider making the updateUnderlyingPrices function payable or encapsulating both calls to prevent using the ETH for another price update.
Update: Resolved in pull request #79 at commit fe24699.
deployFundsIn the _deployFunds function there could be an off-by-one error due to rounding. Consider the case where two assets have 5000 as their bps allocation and the _amount is 10001. In this case, the amount for both will be 5001, which will fail upon the second deposit as the remaining funds will be 5000.
Since distributions should be descending, consider transferring the remaining balance for the final asset instead of computing the amount using bps.
Update: Resolved in pull request #94 at commit f8d4889.
_depositToCoreLendingPoolWithin the _depositToCoreLendingPool function, the call to lendingPool.deposit could potentially pass an _amount of 0, causing the validateDeposit function to fail.
Consider implementing a check to skip the entire _depositToCoreLendingPool call if actualTokens == 0.
Update: Resolved in pull request #84 at commit b25bc7a.
The RizLeverager contract implements the functionality to loop over positions and increase both the deposit and debt at the same time. To do so, it must approve the lending pool for taking the particular asset. Such approvals check if the allowance is zero in the lending pool and the treasury before raising it to the maximum.
However, depending on the assets and how they are looped over, it might be possible for them to reach an artificially high value, which would gradually reduce the allowance. This might happen with assets that do not have a full uint256 range for the allowance, since the storage slot is shared with another variable (e.g., a slot containing the balance and the allowance in the same 256 bits). The problem arising from this would be that even with a small, non-zero value as the allowance, the _approve function will not raise the allowance back to the maximum value. Thus, the transaction will fail as it will not have enough allowance to proceed. When the allowance drops below the value that will be used for the loop (which primarily depends on the number of loops and the amount), the allowance should be raised again.
Consider checking the value that will be used as a whole during the loop against the current allowance and raising it again if needed. This will help prevent transactions from failing with certain assets.
Update: Acknowledged, not resolved.
The Riz protocol is meant for riskier assets and supports a feature whereby a shutdown can be triggered to then slash the positions and redistribute the remaining funds after the bad debt has been socialized. However, even though the shutdown state cannot currently be changed after it has been triggered, most of the modules in the market are upgradeable. As such, these modules might call an upgraded implementation that would de-shutdown the state.
In order to follow the specs of being completely and irreversibly shut down, consider preventing any shutdown markets from being upgraded.
Update: Acknowledged, not resolved. The Radiant team stated:
Noted. If a shutdown happens, we will need to burn admin access to all the proxies in that specific Riz market. Making the proxy bricked might be slightly more complicated.
In the RizLendingPoolAddressesProvider contract, the _addresses mapping stores all the module addresses against their respective ids, whether they are proxy instances or contracts. Typically, to retrieve or update one of these modules, the contract uses a function that eventually calls the _updateImpl function. This function either deploys a new proxy and stores it in the mapping, or updates the proxy to point to the new implementation.
However, some methods bypass this mechanism and allow for manually setting the target address where the address provider will be redirecting the calls to. For instance, the setLendingPoolCollateralManager and the general setAddress functions directly overwrite the address for the respective id. This can lead to issues if the contract owner manually sets the address to the implementation instead of the proxy. In such cases, the market might use the implementation contract directly which may not have all the parameters set up correctly. Since there are no checks to validate whether the new address is a proxy or the underlying implementation, the present setup of links between modules is error-prone and can adversely affect the protocol.
Where applicable, consider implementing validation to ensure that the new address is always a proxy and not the underlying implementation.
Update: Acknowledged, not resolved. The Radiant team stated:
We have fixed
setAddressto always check whetheraddress(0)has been passed as the input. We acknowledge this and want to use it as it is since we can quickly fix it even if we pass something wrong through our multi-sig configuration.
setAddress Could Reset Module StorageThe _updateImpl function of the RizLendingPoolAddressesProvider contract deploys a new proxy when the _addresses mapping has a zero value for the particular id. Otherwise, it makes a call to the deployed proxy to initiate the upgrade to the new implementation.
However, if the owner calls the setAddress function with a zero value for the particular id and then upgrades the implementation of the module, the contract will read the zero address, create a new proxy, and start the storage from scratch in that module. This would reset all stored data for that module, potentially causing functionality to become stuck or users to lose funds.
As the _updateImpl function already takes into account whether a proxy has been deployed or not, consider either removing the manual setAddress functionality from the code, or restricting its parameter to any implementation besides the zero address.
Update: Resolved in pull request #82 at commit 8e7584e. The setAddress function no longer accepts the zero address as input.
In the EmergencyWithdraw library, the code gets the amount of tokens owed to the user for each reserve in the market. The mentioned operation results in:
(valueInUSD[i] / priceAsset[i]) * DEFAULT_DENOMINATOR
However, because the division operation comes first, the value in assets will be rounded down by the price and then the result will be multiplied up by DEFAULT_DENOMINATOR (1e18). This means that it will have 18 ending zeros and part of the precision of the operation will be lost.
Consider swapping the operations to improve the precision of the result.
Update: Resolved in pull request #83 at commit c92e990.
After a market has been shut down, the shutdown function from the RizLendingPool contract takes a snapshot through the BadDebtManager contract. This is done to keep a record of the prices in the particular lending pool and also calculate the ratio for slashing the remaining users who still possess some positive net worth in the market.
In case an asset becomes problematic or the prices cannot be trusted due to market dynamics, the owner of the BadDebtManager contract can still modify the snapshot data after the initial snapshot. However, there is no restriction on when and how many times this data can be modified. In particular, the owner can alter the parameters, especially the slashing ratio, after some users have already proceeded with emergency withdrawals, resulting in unfairness towards the users before and after this change. Moreover, the contract owner can front-run users' withdrawals and change the parameters before those are executed.
In favor of having fair debt socialization, consider using a short time window, perhaps a single day, in which the owner can adjust the values after the snapshot, and after that change is made, values cannot be modified. In addition, consider limiting the number of times the snapshot data can be modified within this window, ideally to a maximum of one time.
Update: Acknowledged, not resolved. The Radiant team stated:
Acknowledged. We want to leave as much room for adjustments as possible for this one, as there is a large room for errors when things go really bad. The
BadDebtManageris the last resort for users to withdraw funds, and we might even need to run snapshots and votes in order to set the proper data if that is ever needed. We will discuss this internally and check if we need to do anything.
transfer/transferFrom CallSome early implementations of ERC-20 tokens would return false instead of reverting the transaction when a transfer failed. This behavior is considered unsafe because a false return value could be ignored by the caller, leading to unexpected behavior. For instance, in the _depositToCoreLendingPool function of the RevenueManagement contract, the transfer call is unchecked.
Consider using the SafeERC20 OpenZeppelin library to perform safe token transfers in the mentioned case and in other similar situations.
Update: Resolved in pull request #71. The Radiant team stated:
We are now using
safeTransferfordepositToCoreLendingPool.
The following instance of unused constant was identified and can be removed:
BIPS_DIVISOR constant declared in RizLendingPool and in RizLendingPoolConfiguratorConsider removing any unused constant to improve the overall clarity and readability of the codebase.
Update: Resolved in pull request #49. The Radiant team stated:
We removed the unused constant.
Throughout the codebase, there are instances of unsafe casts.
uint256 cast in the OracleRouter contractuint256 cast in the OracleRouter contractuint8 cast in the RizLendingPoolConfigurator contractuint8 cast in the RizLendingPoolConfigurator contractuint8 cast in the RizLendingPoolConfigurator contractTo avoid unexpected behavior, consider ensuring the values being casted are in the expected type range before being casted and use informative error messages in case they are not within acceptable ranges.
Update: Resolved in pull request #72. The Radiant team stated:
Added checks before casting.
RizLeverager Could Fail During loopIn the loop function of RizLeverager, the division by the RATIO_DIVISOR can return 0 in some cases. Due to the requirement that the amount cannot be 0 for validating deposits and borrows, this could cause a revert.
Consider exiting the loop early if the computed amount is 0. This will have the same effect but without reverting and will help save gas as well.
Update: Resolved in pull request #73. The Radiant team stated:
We now break from the loop early if
amount == 0.
Within the codebase, some files have been forked from other projects, namely Aave and Yearn. However, the docstrings in these files still reference the previous projects. As a result, these docstrings have also become outdated.
RizLendingPool contract has outdated @title, outdated @dev, and outdated @author tags.RizLendingPoolAddressesProvider contract has outdated @title, outdated @dev and outdated @author tags.RizLendingPoolConfigurator contract has outdated @title, outdated @author, and outdated @dev tags.Consider updating all docstrings to reflect the changes that have been made since forking the code.
Update: Resolved in pull request #78. The Radiant team stated:
Updated the docstrings in the recommended contracts.
Within OracleRouter, a subtraction of chainlink's decimals is performed inside _getChainlinkPrice. If the decimals from the chainlink aggregator exceed 18, then this operation will fail non-descriptively.
Consider implementing an informative error message which is returned when the decimals exceed 18. Alternatively, consider adding logic to format prices with greater than 18 decimals.
Update: Acknowledged, not resolved. The Radiant team stated:
Acknowledged. We find this scenario to be very unlikely.
Throughout the codebase, there are several instances of incomplete docstrings. For example:
RevenueManagement.solbatchInitReserve function within RizLendingPoolConfigurator.solRizRegistry.solNote that these are just a few examples and not an exhaustive list.
Consider thoroughly documenting all functions/events (and their parameters or return values) that are part of a contract's public API. When writing docstrings, consider following the Ethereum Natural Specification Format (NatSpec).
Update: Resolved in pull request #96. The Radiant team stated:
Added docstrings/NatSpec at various places.
Within the OracleRouter contract, a subtraction of Pyth's expo value is performed inside the _getPythPrice function. If the expo from Pyth's return exceed 18 or are less than -18, then this will fail non-descriptively.
Consider implementing an informative error message which is returned when abs(priceData.expo) > 18. Alternatively, consider adding logic to format prices with exponents outside of [-18,18].
Update: Resolved in pull request #92 at commit 3f0fdc0.
swapStrategyWithin the swapTokens function in RevenueManagement.sol, if the provided swapStrategy value is not specified (i.e., is 0), it will be considered of type AGGREGATOR. This is because the SwapStrategies enum has only one value.
To prevent invalid swapStrategy values from being successful in the future, consider adding a buffer as the first element of the enum, like NONE or UNSPECIFIED.
Update: Resolved in pull request #86. The Radiant team stated:
Added
NONEas the first option in the struct.
Within the codebase, there are instances of unused errors:
Consider removing the unused errors.
Update: Resolved in pull request #87. The Radiant team stated:
Removed unused errors.
Throughout the codebase, there are instances of missing event emissions:
RizRegistry contract, the configureReserveAsCollateral function triggers the CollateralConfigurationChanged event inside the RizLendingPoolConfigurator contract. However, it does not trigger an analogous event that also emits the RizLendingPoolAddressesProvider's address.RizLendingPoolAddressesProvider contract, the setLiquidationFeeTo function does not emit any event. Moreover, the function calling this function from the RizRegistry contract does not emit an event either.To improve code readability and inform about the sensitive actions happening in the protocol, consider emitting relevant events at the aforementioned places.
Update: Resolved in pull request #77. The Radiant team stated:
Added the events recommended by the audit team.
Throughout the codebase, there are several access-controlled actions that allow the respective roles to set/link the different modules between each other relying only (in some cases) on the fact that the passed address is a contract or not. However, even when an address might have code at it, the expected implementation might be completely different from the desired one. This could render the markets useless as expected logic will not be executed.
The functions setOracleRouter, setOracle, setReserveInterestRateStrategyAddress, setWethGateway all do not validate anything beyond whether the address being set is a contract. The function setTreasury does no validation whatsoever. All of these would benefit from checking that the interface of the input parameter contract matches what is expected.
Consider using an introspection standard to prevent scenarios in which a market is tied to an erroneous module by mistake.
Update: Partially resolved in pull request #95 at commit 13d4377. The Radiant team stated:
We have added a zero address check in the
setTreasuryfunction but have decided not to implement introspection checks in the other mentioned functions. This decision is based on our belief that the current checks are already sufficient. In addition, these functions are restricted to being called by the owner which, in our setup, is a multi-sig wallet. The multi-sig wallet requires multiple approvals for any transaction, significantly reducing the likelihood of passing incorrect data to the functions without it being noticed during the review process.
Throughout the codebase, there are instances of inconsistent coding style:
RizLendingPool contract, when the liquidationCall function is called, the returnCode output is compared against zero instead of using the constant assigned for it in the LendingPoolCollateralManager contract.RizLendingPoolConfigurator contract, the code uses the ILendingPool interface for defining the variable type. However, as it is referring to the Riz version, the poolShutdown function needs to convert it to the IRizLendingPool interface, which has the extended methods. The same applies to the ILendingPoolAddressesProvider interface and the IRizLendingPoolAddressesProvider interface with the extended methods.Consider fixing these inconsistencies throughout the codebase to improve code readability.
Update: Partially resolved in pull request #85 at commit c310807. The Radiant team stated:
Implemented the recommendation in
RizLendingPool. ForRizLendingPoolConfigurator, we decided to only change thelendingPoolAddressesProviderto the Riz interface as we would have to modify more code to be able to useIRizLendingPoolon some of the methods due to incompatibility with v2-core.
updateLendingPoolStatusThe RizRegistry contract implements the updateLendingPoolStatus function which is used whenever a market is being shutdown. Since the function is only callable by the RizLendingPoolConfigurator contract and has a constant false as the status input there, the second parameter of the function does not serve any purpose.
Consider removing the status parameter from the updateLendingPoolStatus function if there is no possibility of using a true status.
Update: Acknowledged, not resolved. The Radiant team stated:
Acknowledged. We have decided to keep it as it is.
In the RizRegistry contract, the initialize function calls the __Ownable_init hook but not the __UUPSUpgradeable_init one. Even though the __UUPSUpgradeable_init hook is currently empty, it is recommended to call it nonetheless. This would help reduce the error-proneness of the initialization of future implementations that do implement some logic in this hook.
In the initialize function of the RizRegistry contract, consider calling the __UUPSUpgradeable_init hook as well.
Update: Resolved in pull request #91 at commit 3457609.
The RizLendingPoolAddressesProvider contract allows the owner to define the marketId parameter when adding a new pool manually. In addition to this, the owner can also generally set the marketId at will. This action can cause problems if done after the market has become operational. In particular:
marketId might cause linking problems in other parts of the codebase.marketId can be accidentally changed to that of another market.To prevent possible breakdown of a market or the protocol, consider disabling such functionality. Alternatively, consider documenting the reasons for having this functionality while also implementing checks to detect and disallow identical marketIds.
Update: Resolved in pull request #98. The Radiant team stated:
Implemented a validation check to ensure that
marketIdis not empty.
The RizRegistry contract implements the functionality to perform an emergency transfer of tokens on behalf of the contract owner. However, the contract being used is the WETHGateway contract and the owner can specify any token when performing such a transfer, not limited to WETH or ETH.
This can cause confusion and make the protocol error-prone, as unexpected actions can occur (such as having assets stuck in a gateway or having a different asset than WETH/ETH in a gateway meant for WETH/ETH).
Consider adapting the WETHGateway contract to reflect a more general kind of gateway, splitting the functionality into a different contract, or restricting it in the aforementioned contract.
Update: Resolved in pull request #99. The Radiant team stated:
Updated the docstring to clearly detail the function's purpose. Please note that this function is only used if an asset is stuck inside
wethGateway, which is unlikely.
setAddressAsProxyIn the RizLendingPoolAddressesProvider contract, the setAddressAsProxy function allows for updating the implementation of a certain id. However, even though it is known that passing an id for an implementation that does have a dedicated setter could result in a bad scenario, there is no validation or restriction on these particular ids.
As ids that might be susceptible to such problems are known, consider enforcing respective validations to prevent touching such modules with this functionality.
Update: Resolved in pull request #97. The Radiant team stated:
Implemented validation checks.
OracleRouter Not Utilizing Pyth Confidence IntervalThe OracleRouter reads data from Pyth without incorporating the confidence interval returned by Pyth.
In some cases, this confidence interval may be useful, to set bounds on the value of assets in the system for purposes of collateralization or liquidation, or when markets are subject to high fluctuation of prices.
Consider utilizing the Pyth returned confidence interval, or alternatively explaining why it is not needed within the documentation.
Update: Acknowledged, not resolved. The Radiant team stated:
Chainlink has no such functionality and we treat each oracle provider equally (i.e., the oracle can provide the latest prices). Also, we have doubts if we need to utilize this function in the
OracleRouter.
uint256 Casted to EnumIn the RizLendingPool contract, the repay function accepts, among other parameters, the rateMode input, whose type is uint256. However, this input is an element of the InterestRateMode enum from the DataTypes library. Thus, it is converted back to the InterestRateMode type to be used later during the execution. In Solidity, enums are stored as uint8, meaning that an enum can only hold 256 values at most. Using an input which exceeds this size might result in problems such as overflows and unexpected reversions.
Consider passing the rateMode input argument as a InterestRateMode enum type directly instead of passing it as a numerical value of type uint256.
Update: Resolved in pull request #76. The Radiant team stated:
Switched from
uint256toDataTypes.InterestRateModeenum.
The RevenueManagement contract inherits the properties of the ReentrancyGuard contract to reduce the attack surface and mitigate certain attacks. However, it is importing the regular dependency instead of the upgradeable one, as it is being done for other dependencies.
Consider using the upgradeable version of the ReentrancyGuard contract.
Update: Resolved in pull request #75. The Radiant team stated:
We are now using
ReentrancyGuardUpgradeable.
Due to the complex nature of the system, we believe this audit would have benefited from more complete testing coverage.
While insufficient testing is not a vulnerability per se, it nonetheless implies a high probability of hidden vulnerabilities and bugs. Given the complexity of this codebase, the integration with previous versions, and the numerous interrelated risk factors, this probability is further increased. Testing provides a full, implicit specification along with the expected behaviors of the codebase, which is especially important when adding novel functionalities. A lack of testing increases the chances that correctness issues will be missed. It also results in more effort to establish basic correctness and reduces the amount of effort spent exploring edge cases, thereby increasing the chances of missing complex issues.
We recommend implementing a comprehensive multi-level test suite consisting of contract-level tests with >90% coverage and per-layer deployment, integration tests that test the deployment scripts as well as the system as a whole, and tests for planned upgrades. Crucially, the test suite should be documented in a way so that a reviewer can set up and run all these test layers independently of the development team. Furthermore, applying advanced testing over the protocol, such as invariant property testing, might have the extra benefit of finding more edge cases or patterns that might not be easy to think of while coding cases, even when using fuzzing scenarios. Also, as the codebase relies on previous versions, it would benefit both protocols to create common properties that could be shared and imported into the respective test suites. Implementing such a test suite should be a very high priority to ensure the system's robustness and reduce the risk of vulnerabilities and bugs.
Update: Acknowledged, not resolved. The Radiant team stated:
Acknowledged.
Within the codebase there are a few places where names of variables or functions result in confusion. For example:
RizRegistry.sol, the variable reserve actually refers to an underlying asset. Consider renaming this variable to something like asset.RizRegistry.sol, the function is titled _deployCopyFromFromImpl, with a repetition of the word "from". Consider renaming or removing this function, as it appears unused.RizRegistry.sol, the variable amountInETH will actually contain a value in USD. A similar pattern is noted on line 521, but on line 748 it is not noted. Consider changing the naming or noting with comments that it will not be an ETH-denominated value.EmergencyWithdraw.sol, the user performing the withdrawal is called owner. Throughout the codebase, owner is used to refer to the "owner" of a proxy contract. Consider renaming this parameter to "user" or something similar.Consider following the above-outlined recommendation for increased code quality and clarity. Remember that any future development work may rely on these names, so having correct and descriptive naming will speed up development work and result in a lower probability of introducing bugs.
Update: Resolved in pull request #74. The Radiant team stated:
Implemented the recommendations by the audit team.
In the RizLeverager contract it is impossible to withdraw ETH and as a result, funds sent to it would be lost.
This stems from the integration of WETH (Wrapped ETH) as a potential "common asset". A payable receive function has been added to handle the WETH contract transferring ETH to the RizLeverager contract. However there would be no way to actually utilize this, since the WETH contract can only transfer ETH to the caller of the withdraw function, and there is no call to withdraw within the RizLeverager contract.
There appears to be no need for this contract to store WETH.
Consider removing this storage variable, updating the receive function to always revert, and removing code associated with WETH, such as the import of the IWETH interface, and its use within the initialize function.
Update: Resolved in pull request #51.
Since Solidity version 0.8.4, custom errors provide a cleaner and more cost-efficient way to explain to users why an operation failed.
Throughout the codebase, instances of revert and/or require statements were found. Specifically, within the TokenizedStrategy.sol file, most error handling uses literal strings instead of custom errors. In addition, within RizLendingPool.sol, RizLendingPoolConfigurator.sol, and RizAToken.sol, two different contracts called Errors.sol are imported, with one being renamed to RizErrors to resolve namespacing concerns. Having intermixed errors from both of these files makes understanding reverts during runtime more difficult for both users and developers.
For conciseness and gas savings, consider replacing the require and revert statements with custom errors wherever they are used. Moreover, for the sake of code clarity, consider combining both of the Errors.sol files into a single file that is local to the riz github repository. This will make understanding errors and situations when they arise much easier to understand, and will prevent integration issues if changes are made to the v2-core repository.
Update: Resolved in pull request #69. The Radiant team stated:
Merged the error files into one and changed all
requirestatements toifstatements with custom errors.
Providing a specific security contact (such as an email or ENS name) within a smart contract significantly simplifies the process for individuals to communicate if they identify a vulnerability in the code. This practice is quite beneficial as it permits the code owners to dictate the communication channel for vulnerability disclosure, eliminating the risk of miscommunication or failure to report due to a lack of knowledge on how to do so. In addition, if the contract incorporates third-party libraries and a bug surfaces in those, it becomes easier for their maintainers to contact the appropriate person about the problem and provide mitigation instructions.
Throughout the audited codebase, there are no contracts that have a security contact.
Consider adding a NatSpec comment containing a security contact above each contract definition. Using the @custom:security-contact convention is recommended as it has been adopted by the OpenZeppelin Wizard and the ethereum-lists.
Update: Resolved in pull request #70. The Radiant team stated:
Added a security contact in each Riz contract.
In the EVM, it is more gas-efficient to read values from the stack than from memory or state. In a for loop, where a value is read repeatedly, it is more efficient to write the length of an array to stack and read it from there.
Throughout the codebase, there are instances where the aforementioned optimization could be applied:
BadDebtManager.solBadDebtManager.solBadDebtManager.solDexSwapStrategy.solRizRegistry.solRizRegistry.solFor gas savings, consider writing the array length to the stack (e.g., uint256 arrayLength = array.length;) and then using the arrayLength variable.
Update: Resolved in pull request #64. The Radiant team stated:
Wrote array lengths to stack before looping through them.
immutableIf a variable is only ever assigned a value from within the constructor of a contract, then it could be declared as immutable.
The maxRizPools state variable could be immutable.
To better convey the intended use of variables and to potentially save gas, consider adding the immutable keyword to variables that are only set in the constructor.
Update: Resolved in pull request #59. The Radiant team stated:
Made
maxRizPoolsimmutable.
The swap function of DexSwapStrategy.sol returns a uint256 value. However, the only place where this function is called does not utilize this return value.
Consider either removing it, utilizing the return value within the swapTokens function, or using comments to document why it is not being used.
Update: Resolved in pull request #55. The Radiant team stated:
Removed the unused return variable.
SafeMath LibraryThe OpenZeppelin SafeMath library provides arithmetic functions with overflow/underflow protection, but Solidity 0.8.0 has added built-in overflow and underflow checking, supplanting the functionality provided by the library.
Throughout the codebase, the SafeMath library is being used in contracts with a Solidity version greater than 0.8.0, resulting in the addition of redundant overflow/underflow checks.
The following contracts import the SafeMath library:
Consider removing the SafeMath import and its associated function calls from the codebase.
Update: Resolved in pull request #50. The Radiant team stated:
Removed the
SafeMathlibrary from where it is not needed.
The following typographical errors were identified in the codebase:
BaseStrategy.sol there is either a missing or an incorrect word.PythStructs.sol should say "use" rather than "how".PythStructs.sol contains a link which no longer is valid. It may have been replaced by the URL https://docs.pyth.network/price-feeds/best-practices, which is currently active.RizLendingPool.sol should say "proportional" instead of "proportionally".RizLockZap.sol only needs one of the words "amount" or "value".TokenizedStrategy.sol says "deposits" when it should say "deposit".TokenizedStrategy.sol has two periods at the end instead of one.TokenizedStrategy.sol should say "Assess" instead of "Asses".To improve readability, consider correcting typographical errors in the codebase.
Update: Resolved in pull request #54. The Radiant team stated:
Fixed all typographical errors.
++i Can Save Gas in LoopsThroughout the codebase, there are multiple opportunities where this optimization could be applied.
For instance:
Note that this is not an exhaustive list. Consider using the prefix increment operator ++i instead of the post-increment operator i++ in order to save gas. This optimization skips storing the value before the incremental operation, as the return value of the expression is ignored.
Update: Resolved in pull request #57 at commit 4076233.
In RizRegistry.sol, the BadDebtManagerSet event is unused.
To improve the overall clarity, intentionality, and readability of the codebase, consider emitting or removing any currently unused events.
Update: Resolved in pull request #58. The Radiant team stated:
Removed the unused event.
When a setter function does not check whether the value has changed, it opens up the possibility for spamming events that indicate a value change, even when no change has occurred. Spamming the same values can potentially confuse off-chain clients.
Throughout the codebase, there are multiple instances of possible event spamming:
setParamsForEmergencyWithdrawals sets the slashingRatio and emits an event without checking if the value has changed.setLeverager sets the leverager and emits an event without checking if the value has changed.setBadDebtManager sets the badDebtManager and emits an event without checking if the value has changed._setMarketId sets the _marketId and emits an event without checking if the value has changed.setFeePercent sets the feePercent and emits an event without checking if the value has changed.setTreasury sets the treasury and emits an event without checking if the value has changed.setCommonAsset sets the commonAsset and emits an event without checking if the value has changed.setUniRouter sets the uniRouter and emits an event without checking if the value has changed.setEmergencyAdmin sets the emergencyAdmin and emits an event without checking if the value has changed.setTreasury sets the treasury and emits an event without checking if the value has changed.setOracle sets the oracle and emits an event without checking if the value has changed.setDepositLimit sets the depositLimit and emits an event without checking if the value has changed.Consider adding a check which ensures that an event is only emitted if the value has changed.
Update: Resolved in pull request #65. The Radiant team stated:
Implemented checks on the listed methods and a few extra ones which were missed.
Throughout the codebase, there are multiple contracts that deviate from the Solidity Style Guide due to having inconsistent ordering of functions:
OracleRouter contract in OracleRouter.solRevenueManagement contract in RevenueManagement.solRizLendingPool contract in RizLendingPool.solRizLendingPoolAddressesProvider contract in RizLendingPoolAddressesProvider.solRizLendingPoolConfigurator contract in RizLendingPoolConfigurator.solRizLeverager contract in RizLeverager.solRizLockZap contract in RizLockZap.solYRizStrategy contract in YRizStrategy.solTo improve the project's overall legibility, consider standardizing ordering throughout the codebase as recommended by the Solidity Style Guide (Order of Functions).
Update: Resolved in pull request #67. The Radiant team stated:
Followed the Solidity Style Guide. Had to adjust tests/scripts slightly to accommodate these changes.
Throughout the codebase, there are instances of unused imports:
import {RizLendingPoolAddressesProvider, IRizLendingPoolAddressesProvider} from "./RizLendingPoolAddressesProvider.sol"; imports unused alias RizLendingPoolAddressesProvider in RizLendingPool.sol.import { RizAToken } from "../tokenization/RizAToken.sol"; imports unused alias RizAToken in RizLendingPool.sol.import { OracleRouter } from "../OracleRouter.sol"; imports unused alias OracleRouter in RizLendingPool.sol.import { Errors as RizErrors } from "../libraries/Errors.sol"; imports unused alias RizErrors in RizLendingPoolConfigurator.sol.Consider removing unused imports to improve the overall clarity and readability of the codebase.
Update: Resolved in pull request #62. The Radiant team stated:
Removed the unused imports.
TODOsThere is a "TODO" comment in the codebase that should be tracked in the project's issue backlog. See, for example, line 11 of IOracleRouter.sol.
During development, having well-described "TODO" comments will make the process of tracking and solving them easier. Without that information, these comments might tend to rot and important information for the security of the system might be forgotten by the time it is released to production. These TODO comments should at least have a brief description of the task pending to do, and a link to the corresponding issue in the project repository.
To improve code clarity, consider updating the TODO comments to add the above information. For completeness and traceability, a signature and a timestamp can be added as well.
Update: Resolved in pull request #68. The Radiant team stated:
Removed the TODO in
IOracleRouteras it is not needed.
Within the codebase, there are places where small changes can be made to save gas without affecting functionality:
RizRegistry.sol, the lendingPool variable can be re-used instead of making another external call.RizLendingPoolAddressesProvider.sol, the initialization of the proxy variable is redundant when proxyAddress == address(0). Consider moving this initialization into the else clause of the conditional below.BadDebtManager.sol, the check for zero length can be reduced to checking only one array, as the following check will ensure equal length of each array.RizLendingPool.sol, the reservesCount is the same as reservesList.length. Since this is the only place that the Params struct is used at, consider removing reservesCount from the struct rather than passing it and instead read the length of reservesList inside any receiving function.EmergencyWithdraw.sol, calls to badDebtManager.getAssetPrice are made. Instead of making external calls, these values can be cached within EmergencyWithdraw.sol.EmergencyWithdraw.sol, the slashingRatio will be read from badDebtManager multiple times due to the loop. Since slashingRatio is not a function, it will return the same value here each time. Consider reading this value once and caching it for use here.Consider implementing the changes outlined above to save gas. Ensure that all passing tests still pass and either use the same or lesser amount of gas after each change.
Update: Resolved in pull request #90. The Radiant team stated:
Implemented the gas optimizations recommended by the audit team. We split the
EmergencyWithdrawlibrary into separate helper functions as the stack was too deep.
The visibility of the _marketIdToAddressProvider mapping of the RizRegistry contract is public. However, the contract contains a getter function for it under a different name.
In order to improve code readability and minimize the attack surface, consider reducing the visibility of the mapping from public to internal.
Update: Resolved in pull request #56. The Radiant team stated:
Changed the visibility to
internal.
The use of non-explicit imports in the codebase can decrease code clarity and may create naming conflicts between locally defined and imported variables. This is particularly relevant when multiple contracts exist within the same Solidity file or when inheritance chains are long.
Within RizLendingPoolConfigurator.sol, the import "@openzeppelin/contracts/token/ERC20/extensions/IERC20Metadata.sol"; import is a global import.
Following the principle that clearer code is better code, consider using the named import syntax (import {A, B, C} from "X") to explicitly declare which contracts are being imported.
Update: Resolved in pull request #93. The Radiant team stated:
Used explicit imports.
_freeFundsWithin the _freeFunds function, the YRizStrategy contract loops over all the pools and attempts to withdraw as much as possible up to the specified _amount. However, in the case that the amount to withdraw is computed as 0, attempting this withdrawal will revert within validateWithdraw.
Update: Resolved in pull request #47. The implemented logic skips the withdrawal when the amount is 0.
We recommend that the Radiant team create a "checklist" for ERC20 tokens before they are listed in the Riz protocol. For example, we encourage them to check:
name and symbol.While operating Riz markets, we recommend that the Radiant team setup monitoring on their usage. For example:
address(1). Use this to keep a running list of updates to slashing ratios to be applied after shutdown, since the value of those lost tokens can be re-distributed to users.We also recommend that the Radiant team:
0.Finally, due to the nature of this early v0.0.1-alpha release of the protocol, we recommend another audit or security review after the code is finalized. We understand more changes are planned and needed before deployment. Ensure that all recommendations from this audit, as well as from the solidity style guide, are followed before finalizing the code. Consider conducting an internal audit of all code and changes versus any locations the code was forked from.
The Riz system is a complex set of contracts which enable many pieces of functionality surrounding isolated lending markets within the Radiant system. The audited changes are the base of the Riz system and allow users to engage with isolated markets, add ease of use for more complicated functionality, and enable administrative roles to control the operation and deployment of such markets.
Generally, the codebase appears to be well-thought-out in design. However, the implementation could benefit from some improvements in order to enhance its security. There are some inherited features from AAVE’s codebase, such as stable borrowing, that are not intended to be used. It is recommended that any unused functionality be removed from the codebase for the sake of simplification and to ensure that the protocol behaves as expected in the future. In addition, forking of other codebases, such as those of Yearn and Aave, may result in some pain points in integrating the two protocols. This should be approached carefully and with deep knowledge of the security risks of both protocols. During the audit, we found some sections that require some further work such as making sure the documentation inherited from Aave is up to date, ensuring that non-reentrant modifiers are properly used in all the sensitive points, and enforcing stronger input validation in the most relevant sections of the code. A stronger testing suite with wider coverage could have prevented some of the higher-severity issues and would have made the codebase, in general, more robust.
In order to increase user trust and confidence in the protocol, it is advisable that some safeguards are put in place (e.g., timelocks) for sensitive protocol changes or bricking proxies once they are stable and mature. The Radiant team has a significant degree of control over the audited contracts which carries a big responsibility. Furthermore, we strongly encourage the Radiant team to, aside from increasing the test coverage, include invariant testing and fuzz testing. If major changes are made before deployment, a new round of auditing would be advisable, apart from internal audits and strict adherence to the Solidity Style Guide. This adds intentionality to the development process and decreases the chances of some severe vulnerabilities going unnoticed.
We appreciate the Radiant team's diligent communication with the audit team during this engagement, along with their cheerfulness and willingness to share any requested information.