We audited the Ion-Protocol/ion-protocol repository at commit 98e2825.
In scope were the following files:
src
├── admin
│ ├── IonPausableUpgradeable.sol
│ ├── ProxyAdmin.sol
│ └── TransparentUpgradeableProxy.sol
├── flash
│ └── handlers
│ ├── base
│ │ ├── BalancerFlashloanDirectMintHandler.sol
│ │ ├── IonHandlerBase.sol
│ │ ├── UniswapFlashloanBalancerSwapHandler.sol
│ │ └── UniswapFlashswapHandler.sol
│ ├── EthXHandler.sol
│ ├── SwEthHandler.sol
│ └── WstEthHandler.sol
├── interfaces
│ ├── IChainlink.sol
│ ├── IInterestRate.sol
│ ├── IReserveFeed.sol
│ ├── IWETH9.sol
│ ├── IYieldOracle.sol
│ └── ProviderInterfaces.sol
├── join
│ └── GemJoin.sol
├── libraries
│ ├── LidoLibrary.sol
│ ├── StaderLibrary.sol
│ ├── SwellLibrary.sol
│ └── math
| └── WadRayMath.sol
├── oracles
│ ├── reserve
│ │ ├── EthXReserveOracle.sol
│ │ ├── ReserveFeed.sol
│ │ ├── ReserveOracle.sol
│ │ ├── SwEthReserveOracle.sol
│ │ └── WstEthReserveOracle.sol
│ └── spot
│ ├── EthXSpotOracle.sol
│ ├── SpotOracle.sol
│ ├── SwEthSpotOracle.sol
│ └── WstEthSpotOracle.sol
├── periphery
│ ├── IonRegistry.sol
│ └── IonZapper.sol
├── reward
│ └── RewardModule.sol
├── InterestRate.sol
├── IonPool.sol
├── Liquidation.sol
├── Whitelist.sol
└── YieldOracle.sol
The contracts in the src/admin directory are forked versions of the contracts from v5.0.0 of the OpenZeppelin contracts library and only the changes made to these contracts were reviewed.
Several external dependencies were reviewed throughout the audit in the context of how they are used within the system. These external dependencies were not thoroughly reviewed and may contain security issues that are unknown to the auditors but could impact the system. These dependencies should be considered within the risk model of the system. Furthermore, the testing suite was not directly reviewed but was used to gain some insight into how some components of the system are intended to be used.
Ion Protocol implements a borrowing and lending platform for liquid staking tokens (LSTs). LSTs are ERC-20 tokens that represent an amount of tokens staked with a blockchain validator and allow stakers to freely use these tokens while still earning rewards.
Since LSTs are yield-bearing, simply holding these tokens is sufficient to earn staking rewards. Ion Protocol leverages the liquidity of these tokens and allows users to earn additional yield on their LSTs by supplying these tokens as collateral to the Ion lending platform. Users can then borrow WETH against their deposited collateral allowing leverage on their original LST balance.
Depositing WETH into the protocol allows a user to earn interest from outstanding debt positions. To borrow WETH from the protocol, users must deposit one of the LSTs as collateral and borrow against that collateral which does not earn interest from outstanding debt positions. The deposited WETH is not used as collateral when determining the health of a debt position nor is it used when liquidating a user. The protocol is currently designed to support three LSTs: wstETH from Lido, ETHx from Stader, and swETH from Swell.
The IonPool contract is the main contract of the system with all other contracts either being inherited by the IonPool contract, or calling the IonPool contract for specific operations (e.g., liquidations, adding LST collateral, etc.). The main integrations include:
GemJoin contract which should be deployed for each collateral type and is where users deposit and withdraw their LSTs into/from the system.Liquidation contract provides functionality for liquidating vaults. Anyone is able to interact with this contract to liquidate unhealthy vaults and this is the only way to liquidate vaults.YieldOracle and InterestRate contracts are used to determine the interest rates for debt positions.IonZapper contract can be used to convert Ether into WETH and deposit or repay debt in a single transaction. It also contains functionality for atomically wrapping the rebasing stETH token into wstETH and depositing it as collateral.Whitelist contract defines a whitelist of allowed borrowers and lenders using a Merkle tree and is used by the IonPool contract to validate users who interact with the system.A whitelist is included in the system that restricts which accounts can interact with the pool. This whitelist restricts borrowing, depositing collateral into a vault, and depositing WETH into the system. Restrictions are not placed on withdrawing WETH, repaying debt, withdrawing collateral from a vault, transferring collateral, or performing liquidations. Thus, while the whitelist prevents unauthorized users from entering the system, it does not prevent unauthorized users from performing some actions once they have collateral in the protocol. A whitelisted user is able to deposit collateral and open positions on behalf of unauthorized accounts.
If an account is removed from the whitelist, they are still able to repay debt and close out their position by withdrawing collateral. The whitelist owner address should publish the whitelist and proofs in an easy-to-find location. Alternatively, they should host a frontend that automatically fetches the needed proof when interacting with the protocol.
The protocol includes a mechanism for pausing IonPool actions with two independent pausing sets:
The GemJoin contract also implements a simpler pausing model, whereby all actions are either "paused" or "unpaused". The only user-accessible actions in GemJoin are join and exit, which allow LSTs to either enter or leave the system. Giving GemJoin its own pausability allows control of the token flow between the system and the greater ecosystem, while not affecting vault operations. Pausing GemJoin requires a separate call from that needed to pause IonPool actions. Each GemJoin instance maintains its own pausability, so these calls will be needed for each GemJoin.
Update: The structure of pausing has been changed in pull request #36. Now, there is only one "pause" type, which pauses all sensitive actions.
LSTs are held in independent contracts called GemJoin. These contracts are relatively simple and hook into IonPool to affect users' vault positions. However, they can be paused independently to safely handle smart contract bugs and protect protocol liquidity.
Multiple oracles are used in the system to protect against price manipulation. The "reserve oracle" is determined by the LSTs on-chain and is the exchange rate between the LST and Ether. When modifying a debt position, account health is determined based on the spot price of the LSTs. These spot prices are retrieved from a variety of sources:
Internally, the collateralization value of these collaterals is taken as the minimum of these oracle prices and the reserve prices. This ensures that no one can open a debt position that is immediately liquidatable. This also reduces the overall attack surface for opening positions as moving the spot price down would devalue collateral and decrease the amount of debt a user can take on, while increasing the spot prices would not allow more borrowing beyond the reserve oracle rate. Spot market price manipulation also cannot result in a user getting liquidated as liquidations are determined only by reserve oracle rates.
The Liquidation contract allows anyone to liquidate an underwater vault. Liquidations are based on the reserve oracle price and are not affected by the spot price. When a vault is liquidated, it is brought back to a target health ratio by using some of the vault's collateral to pay off debt. The liquidator pays off the excess debt and receives collateral from the vault at a discount.
If there is insufficient collateral held by the underwater vault, a "protocol" liquidation will occur where all collateral from the vault is moved to the protocol account, while the debt is taken on as "bad" debt and is not repaid. This scenario should generally never occur under normal operating conditions as vaults should be liquidated prior to reaching this state.
To facilitate a more user-friendly approach to building leverage positions in the deposited LSTs, the protocol implements flash swap functionality whereby a leveraged position can be created within a single transaction by taking flash loans and swapping for LSTs. The result of creating flash leverage is analogous to opening a position by depositing LSTs, repeatedly borrowing WETH, swapping it to the LST and then re-depositing.
A flash leverage contract shortcuts these steps by flash-borrowing a large amount of either WETH or collateral, and using combinations of swaps and depositing/borrowing from Ion Protocol in order to repay the flash loan. Similar steps can be used to unwind leveraged positions. The BalancerFlashloanDirectMintHandler, UniswapFlashloanBalancerSwapHandler, and UniswapFlashswapHandler contracts offer various flash-leveraging and flash-deleveraging strategies for the different collateral types offered.
Depending on the collateral used, Balancer flash loans, Balancer swaps, Uniswap flash loans, and Uniswap flash swaps are utilized. Users should be aware that their flash-leverage actions are subject to prices on external exchanges which could be manipulated. While Ion protocol implements some checks to prevent sandwich attacks, this may skew prices unfavorably for users.
The system contains numerous integrations with external systems for obtaining price data, swapping assets, and using flash loans. These external systems are assumed to operate as documented and Ion Protocol team should stay vigilant towards any changes made to these systems that could negatively impact the operation of their system. In particular:
Given the large number of external integrations and the dependence of the protocol on these integrations, it is important to plan how to respond to both known and unknown scenarios that could affect the operation of the protocol. In particular, we recommend the team to take the following non-exhaustive list of actions:
dust limit. Monitor for the crossing of these boundaries. Monitor accounts which are near the dust limit in case it is planned to increase the dust limit.Given that the IonPool contract is intended to be deployed as the implementation contract under the transparent proxy pattern, we recommend carefully reviewing any upgrades to the contract in order to ensure that no unexpected issues arise. A few key areas where unexpected issues could arise have been identified:
ilkIndex should never be reused because the old data for the removed collateral type would remain in storage for the contract.Pre-upgrade tests against a forked version of the deployed contract from the Ethereum mainnet can be used to reduce the potential for issues when upgrading. Ensure that storage layouts match before and after the upgrades, and that all previously used storage slots are declared in future contract versions (even if some are no longer used). Moreover, all processes surrounding upgrades should be properly and extensively documented for developers.
Prior to performing a planned upgrade, let users know about it in advance through official communication channels, with plenty of time to act and remove funds should they choose to.
Throughout the codebase, there are privileged roles that can perform upgrades and make changes to the various parameters within the system.
In the IonPool contract and the inherited RewardModule contract, a role-based access control system is used to restrict access to certain functionality. The ION role is the main admin role and is able to perform the following actions:
RewardModule contract.initializeIlk function.updateIlkSpot function.updateIlkDebtCeiling function.updateIlkDust function.updateSupplyCap function.updateInterestRateModule function.updateWhitelist function.pauseUnsafeActions, unpauseUnsafeActions, pauseSafeActions, and unpauseSafeActions functions.LIQUIDATOR_ROLE and GEM_JOIN_ROLE roles.The contract also defines the LIQUIDATOR_ROLE and GEM_JOIN_ROLE roles. These roles are only intended to be granted to instances of the Liquidation and GemJoin contracts so that they can perform their intended actions. These roles should never be granted to EOAs or to contracts not intended to perform the intended actions.
The IonPool contract is intended to be deployed as the implementation contract under the transparent proxy pattern. Thus, the proxy admin is able to deploy a new implementation contract in place of the current IonPool contract.
The GemJoin contract has an owner who is able to pause and unpause the contract, thereby preventing joining and exiting for the specific LST the instance corresponds to.
The IonRegistry contract has an owner who is able to modify and add GemJoin instances to the registry.
The whitelist requires a Merkle proof for whitelisted accounts to be able to interact with this system. This Merkle proof must be obtained off-chain and the owner of the Whitelist contract may update the root at any point in time.
Given the overall complexity and size of the codebase, development should be performed with a security-first mindset to minimize the risk of unintentionally introducing vulnerabilities. In addition, extensive documentation should be written covering the functionality of the protocol as well as the various related systems. We highly recommend that an incident response process be developed as well such that unexpected scenarios can be quickly addressed.
In particular, a few key areas that should be documented, or where a security-focused development process could reduce risks within the protocol, were identified:
IonPool and GemJoin contracts should be coordinated and a documented plan on how the pausing is expected to be done should be written. Scripts should be developed, tested, and made ready to be used in the event that pausing needs to be done.IonPool contract contains several independent steps. If these steps are done in an incorrect order, some checks may be missed. If these steps are done across multiple transactions, frontrunning attacks may take advantage of partially initialized collaterals. Ideally, new collaterals should be initialized atomically.Whitelist and Liquidation contracts). This increases the potential for errors arising in developing and deploying new versions of these contracts.Considering that small errors or deviations from best practices build on each other, ensuring the use of best practices will make code safer, easier to work with, and easier to build upon in the future. It should be assumed that small quirks of the system will be forgotten about at some point in the future. As such, the system should behave intuitively and all strange behavior should be documented extensively. Checks should exist to prevent future developers from making dangerous changes. We believe that the codebase may benefit from implementing Natspec documentation, following the "fail early and loudly" principle, following the checks-effects-interactions pattern, and utilizing single sources of truth where appropriate (e.g., having a global pause, or referencing the IonPool for all ilk configuration related calls).
ReserveFeed ContractThe ReserveFeed contract tracks exchange rates for each collateral type. The setExchangeRate function does not contain any access control or input validation. This would allow anyone to set any price for any collateral type.
Consider implementing access control on the setExchangeRate function to ensure that only trusted users can update the tracked rates.
Update: Resolved in pull request #20.
IonZapper Will Lock WETH in ContractThe zapRepay function in the IonZapper contract is intended to allow a user to repay debt in their vault in the IonPool contract by transferring ETH into the IonZapper contract. The zapRepay function wraps the ETH sent into WETH and then is supposed to use this to repay debt on behalf of the msg.sender.
The payer argument to the repay function in the IonPool contract specifies what address the WETH is transferred from for repaying debt. This is set to the original caller's address within the zapRepay function. As a result, if msg.sender has approved the IonPool contract to spend WETH on their behalf, and they hold a sufficient WETH balance, the IonPool will transfer WETH from the user's account into the pool contract and not use the WETH that is held in the IonZapper contract. Thus, the user will spend twice as much funds as expected while the ETH, now held as WETH, sent to the IonZapper contract will be unrecoverable.
Consider setting the payer argument for the repay function to the IonZapper contract address within the zapRepay function. This will ensure that the WETH held by the IonZapper contract is transferred into the IonPool contract.
Update: Resolved in pull request #21.
repayAndWithdraw Will Frequently Fail for Full RepaymentsWithin the repayWithdraw function of the IonHandlerBase contract, the amount to repay is specified in units of WETH. As a result of inaccuracies in converting this to normalizedDebt units, when attempting to repay an account's full borrow, this function is likely to fail. Inaccuracies may arise from rounding incorrectly, or from the transaction being mined at a different time than the user anticipates and interest accrual being unexpected.
If a user attempts to pay off their entire borrow and the computed normalizedDebtToRepay is too high or too low, the call may revert. In case normalizedDebtToRepay is higher than a vault's normalizedDebt, the addition of the negated normalizedDebtToRepay will revert since the value should be less than 0. In case that normalizedDebtToRepay is too low, the vault is likely to be dusty, causing a revert.
Consider refactoring the repayAndWithdraw function to include a "max repayment" logical branch. Consider using the value repayAmount == uint.max to indicate a full repayment, for example. Note that this will require changes to the transfer of WETH since the amount of WETH will need to be computed on-the-fly, and will require an accurate rate to do so. Alternatively, consider making a separate function for full repayment, and informing users, at multiple places, of the risk of failure when using the regular repayAndWithdraw function for full or near-full repayments.
Update: Resolved in pull request #22.
IonZapper Contract From Functioning as IntendedThe zapDepositWstEth function in the IonZapper contract allows users to add collateral to the IonPool contract using stETH tokens from their account. The function is intended to first transfer the specified amountStEth amount of tokens from the user's account into the IonZapper contract, then wrap these tokens into the wstETH token, and finally call the GemJoin contract to deposit these wstETH tokens on behalf of the original caller.
However, there are two places within this function where the IonZapper contract needs to approve external contracts to spend ERC-20 tokens where the approvals do not exist:
IonZapper contract is transferred into the wstETH contract when the wrap function is called. In order for the wstETH contract to make this transfer, the IonZapper contract must approve the wstETH contract to spend stETH tokens on its behalf.join function on the GemJoin contract, the wstETH held by the IonZapper contract is transferred into the GemJoin contract. In order for the GemJoin contract to make this transfer, the IonZapper contract must approve the GemJoin contract to spend wstETH tokens on its behalf.Consider setting the appropriate token approvals for the appropriate amounts within the zapDepositWstEth function to allow it to operate as intended.
Update: Resolved in pull request #21.
In the ReserveOracle contract, a MAX_CHANGE percentage bound is enforced that determines the maximum amount the currentExchangeRate can change when the updateExchangeRate function is called. This restriction only applies each time the function is called and there is no restriction on how much the exchange rate can change within a single block or transaction. The MAX_CHANGE bound can be easily bypassed by calling updateExchangeRate multiple times in a single transaction.
The reserve oracle is used within the Liquidation contract to determine the value of collateral when determining if a vault is liquidatable. Since the reserve oracle computes the exchange rate as the minimum of the protocol exchange rate and an averaged price from external price feeds, if an attacker were to have sufficient control over any of the price feeds, or was able to manipulate the protocol exchange rate, they would be able to move the price enough within a single transaction to liquidate users by undervauling their collateral.
Consider either using a time or block-based maximum change percentage to limit large price changes to longer periods of time and prevent single transaction attack scenarios.
Update: Resolved in pull request #32.
The SwEthSpotOracle contract uses a Uniswap v3 TWAP oracle to get the price of the swETH token. The intended pool (ETH/swETH 0.05% fee pool) does not currently have a large enough observation cardinality to support the TWAP window (2 hours) intended to be used. The observation cardinality of the pool should be at least as large as the number of blocks in the TWAP window (600 blocks) otherwise the TWAP calculation may fail as the Uniswap pool would not contain any data from the beginning of the TWAP window.
This would prevent users from modifying their positions. As a result, vaults could be liquidated if there was a sufficient change in the exchange rate and users would be unable to repay their debt, or deposit collateral into their vault to prevent liquidation.
Consider increasing the observation cardinality for the pool by calling the increaseObservationCardinalityNext function on the pool contract with an input amount that is at least as large as the number of blocks within the TWAP window. Further, the TWAP should not be used until the Uniswap pool observation cardinality has increased to the target amount. The constructor for the SwEthSpotOracle contract should validate that the observation cardinality is sufficiently large when deploying the contract.
Update: Acknowledged, will resolve. Ion Protocol team stated:
This is a known issue. The team is aware that the observation cardinality of the swETH Uniswap AMM needs to be increased for the TWAP to function properly. We will not be launching with Swell's stETH until the observation cardinality is increased.
rate ValuesThe liquidate function of the Liquidation contract does not accrue interest prior to liquidating a vault. Instead, interest is accrued within the call to confiscateVault function which determines the amount sent to the liquidator. Therefore, the rate at which the amount of collateral and debt to repay for a vault is calculated will be lower than what is sent to the liquidator. This would result in the health ratio of a vault being less than the TARGET_HEALTH value after liquidation when it is expected to be no less than this value. Furthermore, the liquidator will receive the collateral at a lower discount than if interest had been accrued prior to liquidation.
Moreover, the liquidate function could determine that a vault will be dusty after being liquidated and pay off all debt when in fact the vault would not have dust after liquidation if using an accurate value for the rate. As a result, all of the debt within a vault would be paid off when not required.
Consider accruing interest within the liquidate function to ensure that all computations are done with an accurate rate.
Update: Resolved in pull request #30.
FlashSwapThere is no option to set a deadline for the flashswapLeverage, flashswapDeleverage, flashLeverageWethAndSwap and flashDeleverageWethAndSwap functions. The transaction can still be stuck in the mempool and be executed a long time after the transaction is initially called. During this time, the price in the Uniswap pool can change. In this case, the slippage parameters can become outdated and the swap will become vulnerable to sandwich attacks.
Consider adding a deadline check to the flashswapLeverage, flashswapDeleverage, flashLeverageWethAndSwap and flashDeleverageWethAndSwap functions.
Update: Resolved in pull request #25.
msg.sender Rather Than UserThe onlyWhitelistedLender and onlyWhitelistedBorrowers modifiers check if msg.sender is a whitelisted lender or borrower. However, for many functions, the user input parameter specifies the account that has their position adjusted rather than msg.sender. As a result, a whitelisted lender or borrower can create vaults for accounts which are not whitelisted.
In addition, this means that all contracts that call supply, borrow and depositCollateral, such as IonZapper.sol and BalancerFlashloanDirectMintHandler.sol, need to implement another whitelist modifier as the msg.sender in IonPool.sol would be the calling contract and not the user that called the contract.
Consider checking the user rather than msg.sender for the whitelist. Additionally, consider removing the custom modifiers existing in IonZapper.sol and IonHandlerBase.sol in favor of relying on whitelist checks within IonPool.sol.
Update: Resolved in pull request #23. Note that the protocol whitelist is only intended to contain Ion Protocol contracts and thus should not create positions for unauthorized accounts.
The getPrice function of the EthXSpotOracle contract pulls the ETHx to USD price from a Redstone oracle using the Chainlink interface using the deprecated latestAnswer function.
Consider using the non-deprecated latestRoundData function from the Chainlink interface.
Update: Resolved in pull request #32.
Within the IonPool contract, the rate stored for a collateral type can be overwritten using the initializeIlk function. If 256 or more collateral types have been initialized, subsequent calls to initializeIlk will overwrite the rates within the first 256 indices in the ilks array.
Overwriting the rate for one of the collateral types would immediately decrease the outstanding debt for all debt positions against the collateral type as the rate tracks the accumulated interest on debt. As a result, the contract would be in an inconsistent state where the outstanding debt for the collateral type would be less than what lenders are owed.
Consider explicitly validating that the length of the ilks array is 256 or less after pushing the new ilk data within the initializeIlk function.
Update: Resolved in pull request #26.
The pauseSafeActions function of the IonPool contract is intended to accrue interest when the contract is paused. Within this function, the internal _pause function is called first followed by the _accrueInterest function. As a result, interest will not be accrued since the _accrueInterest function does not accrue interest when the contract is in a safe paused state.
Consider switching the order of the internal function calls within pauseSafeActions.
Update: Resolved in pull request #27.
The getPrice function of the EthXSpotOracle contract computes the price of ETHx in ETH. Two oracles are used to determine the price: a Redstone oracle returns the price of ETHx in USD, and a Chainlink oracle returns the price of ETH in USD. These prices are combined to return the price of ETHx in ETH.
The corresponding timestamps for the prices for the oracles are never validated to ensure they are from similar times. If there is a significant discrepancy between the timestamps for the price feeds, this could result in an inaccurate ETHx in ETH price. Specifically, the two prices might have significant misalignment during volatile periods and may be large enough to cause a significant impact on the health of users' vaults. This situation could lead to the following:
Consider validating that the two price feeds are returning prices from similar timestamps.
Update: Acknowledged, not resolved. Ion Protocol team stated:
The concern around the delayed oracle misrepresenting user health vaults is not an issue that will lead to a loss of user-locked funds as no liquidation can be triggered by the spot oracle price. The spot oracle can malfunction by either 'under' valuing or 'over' valuing the market price. This misrepresented spot oracle in the core protocol is used to value the collateral when issuing debt, such that the user debt-to-collateral ratio will adhere to the LTV. It is important to note that the value reported by the spot oracle is not used for liquidations.
If the spot oracle ‘under’ reports a value, this will temporarily reduce the amount of debt that the borrowers can take out from the Ion Pool. However, it will neither lead to a liquidation nor an increase in borrow rate payments and therefore will not result in a loss for the borrower. It will only restrict the amount of debt possible to be issued. There is also no loss for the lenders aside from the borrowers creating fewer loans than what would theoretically have been possible.
If the spot oracle ‘over’ represents the value, this will allow the user to borrow more funds compared to using the ‘fair’ value. However, the maximum this value will reach is the exchange rate reported by the reserve oracle as the spot oracle is maximum-bounded by the exchange rate. This means that even in times of overvalued spot prices, the maximum divergence it can have is capped by the exchange rate. This means that the lenders will never be issuing loans at a valuation any higher than the native redemption value of the staked collateral which is the exchange rate.
The spot oracle is upper-bounded by the exchange rate value reported by the reserve oracle. Thus, the maximum price a spot oracle can report is the exchange rate. If the 'true' value of the market price of the asset falls in a drastic de-peg event, and the spot oracle is manipulated to report a value much larger than this 'true' value, then the risk exists that the attacker can borrow more than they theoretically should have at the true value. In the case of this oracle manipulation, the protocol would redeem the collateral into the beacon chain instead of selling it to make lenders whole. This is possible since the manipulated spot price will again be capped by the exchange rate.
Throughout the codebase, there are several functions that use on-chain oracles to retrieve price data. These oracles consistently lack validation of the returned data. In particular:
WstEthSpotOracle contract uses a Chainlink oracle to get the price of stETH in ETH within the getPrice function.EthXSpotOracle contract uses a Redstone oracle to get the price of ETHx in USD and a Chainlink oracle to get the price of ETH in USD within the getPrice function.Consider adding checks which validate the returned data to ensure a non-zero price is returned and that the price is recent.
Update: Acknowledged, not resolved. Ion Protocol team stated:
Please refer to the M-07 response stating that underreported and overreported spot oracle values do not cause user loss of funds.
The suggested validation above is to check the returned value against a zero value. However, even if the oracle returns a zero value, the zero value will simply not allow any issuance of debt and therefore is not a risk for the protocol. Therefore validating the zero value or not validating the zero value does not introduce a meaningful difference to user safety.
As stated in M-07, similar malfunctioning of oracle behavior will be monitored offchain to trigger pauses when necessary.
The initializeIlk function of the IonPool contract allows for adding a new collateral type. This function only includes features to partially support the addition of a new collateral type and calls to additional functions are expected to occur before the new collateral type is fully supported. Specifically:
updateIlkSpot function.updateIlkDebtCeiling function.updateIlkDust function.updateInterestRateModule function.updateWhitelist function. The current implementation of the Whitelist contract would default to allowing anyone to borrow against the new collateral type.To minimize the attack surface and ensure adding new collateral types is less error-prone, consider consolidating all of the initialization logic into a single function, or making the initialization procedure otherwise atomic.
Update: Acknowledged, will resolve. Ion Protocol team stated:
Initialization procedure for adding new collateral types will be done atomically through Gnosis Multisend.
The IonPool contract contains two internal functions for accruing interest: the _accrueInterestForIlk function for accruing interest for a single collateral type, and the _accrueInterest function for accruing interest for all collateral types. Both of these functions update the supplyFactor which is used for tracking interest accrued on the underlying token.
The supplyFactor is used by the balanceOf and totalSupply functions in the RewardModule contract for converting from normalized balance to the true balances. Both the _accrueInterestForIlk and the _accrueInterest functions pass the current totalSupply to the _calculateRewardAndDebtDistribution function to determine the respective changes to the supplyFactor, the total debt, and the new interest rate for debt positions.
_accrueInterestForIlk only accumulates interest for a single collateral type. As such, the changes to the supplyFactor, the total debt, and the new interest rate will take into consideration only the accrued interest, for that collateral type, since the last time interest was accrued. When interest is accrued for another collateral type in the future, either with _accrueInterestForIlk or _accrueInterest, the supplyFactor, and thus the totalSupply will reflect changes since the last time the first collateral type was updated, but will be applied since the last time the second collateral type was updated.
As a result, accruing interest for a single collateral type, followed by accruing interest for another collateral type in the future is not equivalent to accruing interest for all collateral types each time interest is accrued. This will result in an inaccurate interest rate being applied to users' debt positions.
Consider accruing interest for all collateral types each time there is an action made on any of the collateral types that should result in interest accrual.
Update: Resolved in pull request #27.
Throughout the codebase, functions lack validation of input arguments:
ReserveOracle contract:
_feeds.length is exactly MAX_FEED_COUNT. If _feeds.length < MAX_FEED_COUNT, the constructor will revert when assigning the FEED0, FEED1, and FEED2 variables._maxChange argument is non-zero and less than 1e27 (1 RAY)._bound function does not validate that the min value is strictly less than the max value and may lead to inaccurate output if this does not hold.SwEthSpotOracle contract:
_secondsAgo argument is non-zero.YieldOracle contract:
_historicalExchangeRates values are non-zero. A zero value would result in a division by zero on line 127._getExchangeRate function does not validate that the ilkIndex value is within the expected range and will return 0 for an invalid input.InterestRate contract:
minimumKinkRate values are at least as large as the minimumBaseRate values. If this condition does not hold, the calculateInterestRate function will revert.ilkDataList argument has a maximum length of 8. If an array with a length greater than 8 is passed, it is possible that the distributionFactorSum will be 1e4 when the data stored within the contract will not sum to 1e4.optimalUtilizationRate values are non-zero. A zero optimalUtilizationRate will cause the calculateInterestRate function to revert.reserveFactor values are less than 1 RAY (1e27). A reserveFactor value larger than 1e27 would cause the _calculateRewardAndDebtDistribution function in the IonPool contract to revert.IonPool contract:
mintAndBurnGem, collateral, normalizedDebt, vault, and gem functions do not validate that their ilkIndex arguments are within the supported range.initialize function does not validate that the value of _underlying token is the WETH token which is assumed loan contracts, specifically the IonHandlerBase contract will assume it has a "WETH" interface.Liquidation contract:
_maxDiscount array is 3._liquidationThresholds values should be non-zero. This is instead checked within the liquidate function. Passing in a zero value for the _liquidationThresholds would result in a contract that will deploy but will not be able to perform liquidations as the liquidate function will always revert._targetHealth is at least 1 RAY (1e27). A value less than 1e27 would cause the liquidate function to always revert due to an underflow in subtraction in the _getRepayAmt function._maxDiscount values are less than 1 RAY (1e27). If a value greater than 1e27 is passed, the contract will deploy correctly but liquidations will always revert due to an underflow within the liquidate function.i), _targetHealth >= _liquidationThresholds[i] / (RAY - _maxDiscount[i]). This invariant must hold otherwise all liquidations will revert when discount == configs.maxDiscount within the _getRepayAmt function._getConfigs function does not validate that the ilkIndex argument is within the expected range.UniswapFlashswapHandler contract:
_poolFee argument is not validated to match the fee of the input _pool. This argument can be removed and the fee can be read directly from the UniswapV3Pool contract._wethIsToken0 argument should be validated explicitly against the pool or assigned directly from the _pool.flashswapLeverage function does not validate that resultingAdditionalCollateral >= initialDeposit.flashswapLeverage function does not validate that sqrtPriceLimitX96 is either 0 or between the current price and MIN_SQRT_RATIO or MAX_SQRT_RATIO (depending on token ordering in the pool).flashswapDeleverage function does not validate that sqrtPriceLimitX96 is either 0 or between the current price and MIN_SQRT_RATIO or MAX_SQRT_RATIO (depending on token ordering in the pool).Consider validating the input to all constructors to ensure the contracts cannot be deployed with invalid arguments. A misconfigured contract may cause problems when later attempting to interact with the contract (e.g., liquidate a vault) and could delay performing critical actions. Additionally, ensure input arguments to all functions are properly validated with a preference to fail early and loudly.
Update: Partially resolved in pull request #33. Note that the _getConfigs function of the Liquidation contract still does not validate that the ilkIndex is within the expected range. Ion Protocol team stated:
1. Will not fix
IonPoolinput validation.2. Will not fix: “The
flashswapLeveragefunction does not validate thatresultingAdditionalCollateral >= initialDeposit”. This is Implicitly checked by Solidity’s overflow protection and does not need to be fixed.
When the liquidate function is called during a safe pause, the function reverts during the call to the repayBadDebt function in the IonPool contract. However, during a protocol liquidation, this call does not happen as the function returns early and therefore liquidations will be successful. Allowing liquidations to occur when the IonPool contract is in a safe pause state may result in unfair liquidations for users as they are unable to alter their position to make their loans healthy.
Consider disallowing protocol liquidations when the IonPool is in a safe pause state to prevent unfairly liquidating vaults.
Update: Resolved in pull request #36. A single pause has been implemented which now pauses all liquidations.
When there is no debt for a collateral type, the _calculateRewardAndDebtDistribution function in the IonPool contract will return 0 for the timestampIncrease value and the lastRateUpdate timestamp for the ilk will be unchanged. As a result, the first user to open a debt position for a collateral type when there is currently no debt against the collateral type will immediately accrue interest starting from the lastRateUpdate timestamp for the ilk. Since the timestamp was not incremented when accruing interest before their debt position is recorded internally, the user will have interest applied on their debt from before their position was opened the next time interest is accrued.
Consider always incrementing the lastRateUpdate timestamp for each ilk when interest is accrued regardless of whether there is debt against the collateral type except when the protocol is in a safe pause state.
Update: Resolved in pull request #21.
swEth at an Outdated PriceThe getPrice function of SwEthSpotOracle uses a TWAP oracle which means that a sudden change in price would not immediately affect the return value. This value is used in the getSpot function which calculates the spot price as the minimum of the result of getPrice (the TWAP price) and the exchange rate from the ReserveOracle. Using the minimum is a safety mechanism which ensures that the user can only borrow the more pessimistic of the two oracle values.
However, both the reserve oracle and the TWAP oracle have the property that there is a time delay before the price is updated. The reserve oracle is only updated when the updateExchangeRate function is called. If the price of the swEth token decreases as a step function, due to a smart contract failure or large slashing event, both the TWAP oracle and reserve oracle will not update immediately.
Thus, a user could borrow against the swEth token in the IonPool contract using an out-of-date exchange rate. If the price discrepancy is large enough, it would be profitable for an attacker to deposit a large amount of swEth and borrow the maximum allowed amount of WETH which would exceed the value of the collateral after the price has been updated. Since this deposit and borrow can be executed in a single transaction, the protocol is vulnerable to being drained during a depegging event even if the TWAP window is relatively short.
Consider using a price oracle that better reflects price jumps for the swEth token. Alternatively, consider monitoring for sharp price drops and quickly pausing the ability to borrow in this case.
Update: Acknowledged, not resolved. Ion Protocol team stated:
The TWAP range will be chosen carefully to avoid potentially feeding outdated prices while being resilient against short term price manipulations. We will be monitoring the oracle closely for any necessary pauses.
Throughout the codebase, there are several contracts that include functions that can receive ETH but with no corresponding function for withdrawing ETH. As a result, any ETH sent to these contracts could be permanently lost. In particular:
TransparentUpgradeableProxy contractflashLeverageWeth function in the BalancerFlashloanDirectMintHandler contractzapDepositWstEth function in the IonZapper contractConsider removing the payable attribute or adding a withdrawal feature.
Update: Resolved in pull request #23.
The liquidate function of the Liquidation contract performs a "protocol" liquidation when there is insufficient collateral to bring the health ratio of a vault up to the TARGET_HEALTH ratio. In this type of liquidation, the PROTOCOL address receives the collateral from the underwater vault and is responsible for repaying the bad debt. During a liquidation in which the vault does have enough collateral to bring its health ratio back to TARGET_HEALTH, the caller pays off the debt of the vault by transferring WETH to the contract, and the keeper (specified by the caller) receives the vault's collateral at a discount such that liquidating the vault is profitable to the caller. Since a protocol liquidation does not transfer discounted collateral to the caller, there is no incentive for anyone to perform a protocol liquidation.
Consider providing an incentive to users to perform protocol liquidations to ensure they are done in a timely manner.
Update: Acknowledged, not resolved. Ion Protocol team stated:
This is known and by design. We would not consider this a vulnerability. The option for protocol liquidation exists as a fallback path where the liquidator pays the gas fees for transferring the vault's bad debt to the protocol balance sheet. Vaults that are already in bad debt do not need to be liquidated by searchers and therefore doesn't require incentives.
In more detail:
Protocol liquidations are liquidations where there is not enough collateral to be sold at the discounted price to reach the target health ratio. If there’s bad debt in the position (debt that can’t be paid off with collateral), there is also no profitable path for the liquidator, and we do not expect a searcher to call an unprofitable trade.
The protocol liquidation logical path only exists such that if for some reason a searcher tries to liquidate a position that is in bad debt, it will simply automatically transfer this bad debt to the badDebt variable in IonPool such that the unprofitable searcher pays for the gas cost of transferring badDebt instead of governance having to manually call the transaction.
The alternative option is to simply not do anything about this when bad debt forms, but we decided to add this fallback to automatically transfer bad debt to the balance sheet without paying for gas.
We see this as extra utility for the governance and therefore not a vulnerability.
The updateTreasury function of the RewardModule contract changes the address of the treasury that receives a portion of accrued interest from the IonPool. This function lacks input validation that prevents setting the treasury to the zero address. If the treasury were to be set to the zero address, a majority of the functionality of the IonPool contract (accrueInterest, withdraw, supply, borrow, repay, and confiscateVault) would revert as the _mintNormalized function in the RewardModule contract would revert when attempting to mint to the treasury in the _accrueInterest and _accrueInterestForIlk functions in the IonPool contract.
Consider validating that the treasury address is not the zero address in both the _initialize and updateTreasury functions in the RewardModule contract.
Update: Resolved in pull request #29.
Within the IonPool contract, the depositCollateral and withdrawCollateral functions allow users to deposit or withdraw collateral from a vault. These functions do not accrue interest prior to modifying the collateral balance within the vault. This would allow a user to withdraw collateral from a vault that would have been unhealthy had interest been accrued prior to the withdrawal. Additionally, it would prevent a user from depositing collateral into their vault if their debt position is dusty but would no longer be dusty after accruing interest.
Consider accruing interest prior to all operations that affect the collateral and debt in vaults.
Update: Resolved in pull request #27.
Pragma directives should be fixed to clearly identify the Solidity version with which the contracts will be compiled.
Throughout the codebase, there are multiple floating pragma directives:
ProxyAdmin.sol has the solidity ^0.8.20 floating pragma directive.TransparentUpgradeableProxy.sol has the solidity ^0.8.20 floating pragma directive.WadRayMath.sol has the solidity ^0.8.0 floating pragma directive.Consider using a fixed Solidity version throughout the codebase.
Update: Acknowledged, not resolved. Ion Protocol team stated:
This is intended. Will not fix.
The liquidate function of the Liquidation contract does not update the reserve oracle exchange rate prior to liquidating a vault. This could result in a vault being falsely liquidated that should not be liquidatable based on the current exchange rate from the reserve oracle had the rate been updated. Furthermore, it could lead to a keeper attempting to liquidate a vault and the liquidation failing due to the vault not being liquidatable based on the out-of-date exchange rate.
Consider updating the reserve oracle exchange rate within the liquidate function prior to liquidation calculations.
Update: Acknowledged, not resolved. Ion Protocol team stated:
This is intended. We want a delay between the exchange rate used by the protocol and the true exchange rate. The delay allows the protocol to bound the reported exchange rate and respond with emergency measures in the case of serious malfunction from the liquid staking provider's exchange rate oracle.
Throughout the codebase, many of the contracts have missing or incomplete docstrings. There are also several docstrings that either do not document all arguments, do not document any arguments, or do not document the return values.
IonZapper contract, the IonRegistry contract, or the ReserveFeed contract.RewardModule contract is missing @notice and @return docstrings for most functionsInterestRate contract is missing @notice and @return docstrings for the function calculateInterestRateIonPool contract is missing @notice docstrings for many public interface functions.YieldOracle contract is missing @notice docstrings for the updateAll function.IonHandlerBase contract is missing @notice docstrings for the repayAndWithdraw function.UniswapFlashloanBalancerSwapHandler contract is missing @notice docstrings for the flashDeleverageWethAndSwap function.LidoLibrary, StaderLibrary, and SwellLibrary contracts, as well as accompanying EthXHandler, SwEthHandler and WstEthHandler contracts, while containing internal functions, have almost no documentation whatsoever.Consider thoroughly documenting all functions (and their parameters) that are part of any contract's public API. Functions implementing sensitive functionality, even if not public, should be clearly documented as well. When writing docstrings, consider following the Ethereum Natural Specification Format (NatSpec).
Update: Acknowledged, will resolve. Ion Protocol team stated:
The documentation will be reviewed and edited post-audit to include all up-to-date information.
The _accrueInterest function of the IonPool contract loops over all collateral types and updates the data associated with them. If there are 256 or more collateral types, the for-loop on lines 384-405 will never terminate preventing interest accrual. This would additionally prevent the contract from being safe paused with the pauseSafeActions function, and would prevent users from calling withdraw and supply to withdraw and supply the underlying token.
Consider either explicitly preventing the contract from having 256 or more collateral types in the initializeIlk function, or using a larger type for the loop index variable.
Update: Resolved in pull request #26.
Within the IonPool contract, users are unable to deposit collateral into their vault if their vault is dusty. This would prevent users from adding collateral to their vault which could allow them to increase their borrow amount above the dusty threshold.
Since adding collateral is a "safe" action, consider allowing users to deposit collateral into their vault regardless of whether their vault is dusty.
Update: Acknowledged, not resolved. Ion Protocol team stated:
If the protocol raises the dust value over a position's current borrow amount, then the user can close their position first and reopen the position again. Not fixed as it is an unlikely scenario and no users are at risk.
Several view functions in the IonPool and RewardModule contracts return inaccurate values as they do not accrue interest prior to being called.
balanceOf functiontotalSupply functionsupplyFactor functionrate functiondebt functiongetCurrentBorrowRate functionnormalizedTotalSupply functionThis can lead to difficulties for users who are interacting with the protocol. In particular, if a user wants to withdraw their entire underlying balance, they would expect that calling withdraw(msg.sender, balanceOf(msg.sender)) would withdraw their entire balance but it would not.
In addition, if a user wants to borrow a fixed amount of WETH (amountOfWETH), they would expect that calling borrow(ilkIndex, msg.sender, msg.sender, amountOfWETH / rate(), proof) would result in borrowing exactly amountOfWETH WETH but due to an out-of-date rate value they would receive less WETH than expected.
Moreover, if a user wants to repay their debt and only wants to approve the exact amount of WETH to be transferred from their wallet, the approved amount would be less than what would be required after accruing interest.
Consider implementing functions that will return the correct up-to-date state of the contract for the aforementioned view functions such that their resulting output will be accurate. These functions may be called something like getAccrued... in order to allow the previous functions to remain useful for internal integrations without breaking functionality. We also suggest carefully checking all calls to each of these functions to determine if they should be swapped out for an up-to-date version.
Update: Resolved in pull request #30.
YieldOracle Contract Returns Stale Data After Price DecreaseThe _updateAll function of the YieldOracle contract assumes that the exchange rates are monotonically increasing. Thus, if there is ever a decrease in the exchange rate of one of the tokens (e.g., due to slashing), the YieldOracle will continue to return the exchangeRate and apys values which were there before the exchange rate decrease happened, and will keep doing so until the exchange rate has moved above the last value that was recorded prior to the price decrease. Given the magnitude of expected price decreases due to events such as slashing, and the expected annualized yield of LSTs, it could take a significant amount of time for the YieldOracle to begin returning new data.
Consider implementing a mechanism to return appropriate exchange rates and apys immediately after a price decrease and enable the YieldOracle to begin returning accurate data once the 1-week look-back window has passed.
Update: Resolved in pull request #31.
The join function of the GemJoin contract allows a user to deposit the GEM ERC-20 token into the contract and increases their gem balance in the IonPool contract. The function currently updates the internal totalGem balance, and mints gem to the user account in the IonPool contract prior to transferring the GEM token from msg.sender to the contract. If the GEM token contains a _beforeTokenTransfer hook or some other callback, there could potentially be an opening for reentrancy allowing a user to manipulate their gem balance and steal funds from the protocol.
Though this problem does not exist for the currently planned collateral types, consider moving the token transfer before updating any internal accounting to better implement defensive coding practices and prevent exploits in the future when more collaterals are added, or current collaterals are upgraded.
Update: Acknowledged, not resolved. Ion Protocol team stated:
The protocol should not be supporting hook tokens. Will not fix.
BalancerFlashloanDirectMintHandlerIn the receiveFlashLoan function of the BalancerFlashLoanDirectMindHandler contract, there is a check comparing the LST amount minted to the maximum WETH-denominated debt.
This check is incorrect as it compares two different tokens' amounts. It happens to work for the current collaterals as they have 18 decimals and are more valuable than WETH, so a certain value of LST will generally be less than the corresponding value of WETH. However, in the case that the LST loses value, or a new LST is onboarded with more LST corresponding to a certain amount of WETH, this check may fail, causing the flashLeverageWeth function to be uncallable.
In addition, note that the intention of this check is redundant with the check on line 129, where both variables being compared are denominated in WETH, and prevents the maxResultingDebt from being exceeded.
Consider removing the identified check to avoid failure in the described cases and to improve the readability and maintainability of the code.
Update: Resolved in pull request #24.
The Whitelist contract validates borrowers against a whitelist for each collateral type using the isWhitelistedBorrower function. This function never validates that the input ilkIndex is within the expected range of indices and will default to returning true for an invalid index. If a new collateral type were added to the IonPool contract before updating the whitelist to support it, this would allow anyone to borrow against the new collateral type, bypassing the whitelist.
Consider validating that the input ilkIndex to the isWhitelistedBorrower function is within the expected range for the deployed instance of the Whitelist contract.
Update: Acknowledged, not resolved. Ion Protocol team stated:
Not fixed as the code works as intended. A new collateral type by default should not have a whitelist. A whitelist should only apply to a new collateral type if explicitly set by the protocol. The fix would alter this behavior to apply whitelist to all new collateral types which is not the intention.
privateThroughout the codebase, there are internal functions and state variables that could use private visibility as they either use complex storage patterns, or are only ever called from the base contract they are implemented in. In particular:
ILKCONFIG variables in the InterestRate contract as the data stored in these variables should only ever be read using the _unpackCollateralConfig function._packCollateralConfig function in the InterestRate contract as this function is only ever called by the constructor and writes to immutable variables.Consider reducing the visibility of these functions and variables to ensure they are only used in the correct context.
Update: Resolved in pull request #35.
Within the _modifyPosition function in the IonPool contract, the check for an uninitialized ilk will never cause execution to revert since passing an invalid ilkIndex will revert when accessing the rate.
Consider removing this unreachable code.
Update: Acknowledged, not resolved. Ion Protocol team stated:
Acknowledged, but not fixed in this audit.
Throughout the codebase, magic numbers are used. In particular:
IonPool.sol.UniswapFlashloanBalancerSwapHandler.sol.BalancerFlashloanDirectMintHandler.sol.Following Solidity's style guide, consider defining constant variables to represent these numbers to help developers and auditors understand the intention of the code. These variables will also make future modifications to the code easier by localizing needed changes to a single spot.
Update: Partially resolved in pull request #34. Only the first three instances from the second bullet point have been fixed.
Throughout the codebase, there are several events that would benefit from including additional arguments. In particular:
IonPool contract should include the ilkIndex as an argument as the events are emitted when only a specific ilk is updated.Liquidate event in the Liquidation contract should include the msg.sender.Consider adding these additional arguments to these events to make it clear under what context the events are emitted.
Update: Partially resolved in pull request #35. Ion Protocol team stated:
This was partially fixed, only Liquidate event was altered. Fixes for
IonPoolevents will come after the audit since they are housed in pull request #17.
The invalidWhitelist argument for the InvalidWhitelist error is unnecessary as it will always be the zero-address.
Consider removing this redundant argument.
Update: Resolved in pull request #35.
UPPER_CASE FormatThroughout the codebase, there are constants not using UPPER_CASE format. For instance:
IonPausableStorageLocation constant in the IonPausableUpgradeable contractIonPoolStorageLocation constant in the IonPool contractRewardModuleStorageLocation constant in the RewardModule contractAccording to the Solidity Style Guide, constants should be named with all capital letters with underscores separating words. For better readability, consider following this convention.
Update: Acknowledged, not resolved. Ion Protocol team stated:
Will not fix. This is intended.
Throughout the codebase, there are multiple imports that are unused and could be removed. For instance:
SafeCast import in ReserveOracle.solWadRayMath import in EthXReserveOracle.solWadRayMath import in StaderLibrary.solConsider removing unused imports to improve the overall clarity and readability of the codebase.
Update: Resolved in pull request #35.
During development, having well described TODO/Fixme comments will make the process of tracking and solving them easier. Without this information these comments might age and important information for the security of the system might be forgotten by the time it is released to production. These comments should be tracked in the project's issue backlog and resolved before the system deploy.
The following instance of TODO/Fixme comments were found:
TODO comment on line 122 in UniswapFlashswapHandler.sol.Consider removing all instances of TODO/Fixme comments and instead tracking them in the issues backlog. Alternatively, consider linking each inline TODO/Fixme to the corresponding issues backlog entry.
Update: Resolved in pull request #33.
Since Solidity 0.8.18, developers can utilize named parameters in mappings. This means mappings can take the form of mapping(KeyType KeyName? => ValueType ValueName?). This updated syntax provides a more transparent representation of the mapping's purpose.
Throughout the codebase, there are multiple mappings without named parameters. For instance:
protocolWhitelist mapping in the Whitelist contract.borrowersRoot mapping in the Whitelist contract.unbackedDebt mapping in the IonPool contract.isOperator mapping in the IonPool contract._normalizedBalances mapping in the RewardModule contract.Consider adding named parameters to the mappings to improve readability and maintainability of the code.
Update: Resolved in pull request #35.
Throughout the codebase, there are unused errors. For instance:
TotalDebtsLength error in InterestRate.solSpotUpdaterNotAuthorized error in IonPool.solExternalUniswapFlashloanNotAllowed error in UniswapFlashloanBalancerSwapHandler.solExternalFlashswapNotAllowed error in UniswapFlashswapHandler.solInsufficientBalance error in UniswapFlashswapHandler.solInvalidConstructorArguments error in Whitelist.solInvalidWhitelistMerkleProof error in Whitelist.solTo improve the overall clarity, intentionality, and readability of the codebase, consider either using or removing any currently unused errors.
Update: Resolved in pull request #35.
Throughout the codebase, instances of functions that are updating the state without an event emission were found. For instance:
constructor function in the Whitelist contract.updateBorrowersRoot function in the Whitelist contract.updateLendersRoot function in the Whitelist contract.approveProtocolWhitelist function in the Whitelist contract.revokeProtocolWhitelist function in the Whitelist contract._initializeExchangeRate function in the ReserveOracle contract.updateIonPool function in the YieldOracle contract._setSupplyFactor function in the RewardModule contract.flashLeverageCollateral function in the BalancerFlashloanDirectMintHandler contract.flashLeverageWeth function in the BalancerFlashloanDirectMintHandler contract.join function in the GemJoin contract.exit function in the GemJoin contract.setGemJoin function in the IonRegistry contract.setDepositContract function in the IonRegistry contract.setExchangeRate function in the ReserveFeed contract.Consider emitting events whenever there are state changes to make the codebase less error prone and improve its readability.
Update: Partially resolved in pull request #35. Ion Protocol team stated:
Partially fixed, the event was only added in the
_initializeExchangeRatefunction of theReserveOraclecontract).
There are multiple contracts throughout the codebase that have inconsistent usage of named returns in their functions. For instance:
Consider consistently using either named or unnamed returns for all functions in each contract.
Update: Acknowledged, not resolved. Ion Protocol team stated:
Acknowledged, but not changed in this audit.
Throughout the codebase, there are multiple functions that have variables that are unnecessarily cast. For instance:
FEED0 variable in the _aggregate function in the ReserveOracle contract.FEED0 variable in the _aggregate function in the ReserveOracle contract.FEED1 variable in the _aggregate function in the ReserveOracle contract.FEED0 variable in the _aggregate function in the ReserveOracle contract.FEED1 variable in the _aggregate function in the ReserveOracle contract.FEED2 variable in the _aggregate function in the ReserveOracle contract.RESERVE_ORACLE variable in the getSpot function in the SpotOracle contract.assetIn variable in the _simulateGivenOutBalancerSwap function in the UniswapFlashloanBalancerSwapHandler contract.assetOut variable in the _simulateGivenOutBalancerSwap function in the UniswapFlashloanBalancerSwapHandler contract.ethPerStEth variable in the getPrice function in the WstEthSpotOracle contract.wstEth variable in the getLstAmountOutForEthAmountIn function in the LidoLibrary library.ReserveOracle(configs.reserveOracle).currentExchangeRate() in the liquidate function in the Liquidation contract.To improve the overall clarity, intent, and readability of the codebase, consider removing unnecessary casts.
Update: Resolved in pull request #35.
Throughout the codebase, there are multiple unused variables. For instance:
FACTORY state variable in the UniswapFlashswapHandler contract.POOL_FEE state variable in the UniswapFlashswapHandler contract.ethXDecimals free variable in EthXReserveOracle.sol.PROVIDER_PRECISION free variable in YieldOracle.sol.reserveOracle variable in the _getConfigs function in the Liquidation contract.To improve the overall clarity, intentionality, and readability of the codebase, consider removing any unused variables.
Update: Partially resolved in pull request #35. All identified instances have been removed except for PROVIDER_PRECISION.
Throughout the codebase, there are state variables that lack an explicitly declared visibility. For instance:
EthXHandler.solEthXSpotOracle.solEthXSpotOracle.solIonHandlerBase.solIonHandlerBase.solIonHandlerBase.solIonHandlerBase.solIonHandlerBase.solIonHandlerBase.solSwEthSpotOracle.solSwEthSpotOracle.solUniswapFlashloanBalancerSwapHandler.solUniswapFlashswapHandler.solUniswapFlashswapHandler.solUniswapFlashswapHandler.solUniswapFlashswapHandler.solWstEthSpotOracle.solWstEthSpotOracle.solFor clarity, consider always explicitly declaring the visibility of variables, even when the default visibility matches the intended visibility.
Update: Resolved in pull request #35.
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 proves 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 lack of knowledge on how to do so. Additionally, if the contract incorporates third-party libraries and a bug surfaces in these, it becomes easier for the maintainers of those libraries to make contact with the appropriate person about the problem and provide mitigation instructions.
Security contact information is omitted throughout the codebase.
Consider adding a NatSpec comment containing a security contact on top of the contracts definition. Using the @custom:security-contact convention is recommended as it has been adopted by the OpenZeppelin Wizard and the ethereum-lists.
Update: Acknowledged, will resolve. Ion Protocol team stated:
Acknowledged, security contract will be added post-audit.
Named return variables are a way to declare variables that are meant to be used within a function body for the purpose of being returned as the function's output. They are an alternative to explicit in-line return statements.
Throughout the codebase, there are multiple instances of unused named return variables. For instance:
newTotalDebt return variable in the accrueInterest function in IonPool.sol.supplyFactorIncrease return variable in the calculateRewardAndDebtDistribution function in IonPool.sol.treasuryMintAmount return variable in the calculateRewardAndDebtDistribution function in IonPool.sol.newRateIncrease return variable in the calculateRewardAndDebtDistribution function in IonPool.sol.newDebtIncrease return variable in the calculateRewardAndDebtDistribution function in IonPool.sol.newTimestampIncrease return variable in the calculateRewardAndDebtDistribution function in IonPool.sol.priceX96 return variable in the _getPriceX96FromSqrtPriceX96 function in SwEthSpotOracle.sol.Consider either using or removing any unused named return variables.
Update: Partially resolved in pull request #35. The identified instances in IonPool.sol have not been fixed.
The IonPausableUpgradeable contract defines two separate pause states and is used by the IonPool contract. The states are:
The IonPool contract currently allows for any combination of these pause states. If the contract were to be in a state where safe actions were paused, and unsafe actions were not paused, users would be able to take on additional debt, and transfer collateral out of their vault, both of which would put their vault into a less healthy position but there would be no way for users to make their vault more healthy.
Consider only allowing safe actions to be paused when unsafe actions are also paused.
Update: Acknowledged, will resolve. Ion Protocol team stated:
Unsafe and Safe pauses were combined to form a single protocol-wide pause.
EIP-7201 defines a namespaced storage layout to prevent storage collisions for modular contracts. The EIP defines the @custom:storage-location NatSpec annotation that should annotate the storage struct in a contract. Several contracts throughout the codebase that implement EIP-7201 lack this annotation. In particular:
IonPausableStorage struct in the IonPausableUpgradeable contractRewardModuleStorage struct in the RewardModule contractIonPoolStorage struct in the IonPool contractConsider annotating the storage structs in these contracts with @custom:storage-location.
Update: Resolved in pull request #35.
There are several files throughout the codebase that deviate from the recommended Solidity Style Guide. In particular:
ReserveOracle contract there are functions defined before the constructor.Liquidation contract the Configs and the LiquidationArgs structs are defined between functions.UniswapFlashswapHandler contract, the FlashswapData struct is defined after the constructor.UniswapFlashloanBalancerSwapHandler contract, the FlashCallbackData struct is defined between functions.To favor readability, consider always following a consistent style throughout the codebase.
Update: Partially resolved in pull request #35. Ion Protocol team stated:
Partially fixed, functions were moved below constructor but structs in between functions will not be moved.
The reserve oracle contracts track the latest exchange rate between a LST and ETH and each instance inherits from the abstract ReserveOracle contract. The updateExchangeRate function is used to update the currentExchangeRate state variable to reflect the latest exchange rate however there is no variable that tracks the last timestamp that the exchange rate was updated at.
Consider tracking the last timestamp that exchange rate was last updated at to allow users to easily query whether the exchange rate is up-to-date.
Update: Resolved in pull request #32.
The currentExchangeRate state variable in the abstract ReserveOracle tracks the most recent exchangeRate between an LST and ETH. Since this variable uses public visibility within an abstract contract, a child contract would be able to directly modify this value.
Consider using private visibility for all non-constant and non-immutable state variables in abstract contracts. If child contracts should be able to update values for the state variables, consider creating internal functions for updating these variables which emit appropriate events and verify that desirable conditions are met.
Update: Acknowledged, not resolved. Ion Protocol team stated:
Acknowledged but not fixed in this audit.
public Functions That Should Have external VisibilityThe following public functions should be external:
updateExchangeRate function in the ReserveOracle contract.ilkCount function in the IonPool contract.getIlkIndex function in the IonPool contract.getIlkAddress function in the IonPool contract.addressContains function in the IonPool contract.addressesLength function in the IonPool contract.getCurrentBorrowRate function in the IonPool contract.Consider changing the visibility of these functions to external in order to clarify that these functions will only be called by external contracts.
Update: Resolved in pull request #35.
immutableThroughout the codebase, there are several variables that could be immutable. For instance:
protocolFeed variable in the EthXReserveOracle contract.protocolFeed variable in the SwEthReserveOracle contract.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: Partially resolved in pull request #35. Only the instance in SwEthReserveOracle has been fixed.
Throughout the codebase, there are several functions and variables that could be renamed to better reflect their purpose, in particular:
_getPriceX96FromSqrtPriceX96 function in the SwEthSpotOracle contract with named return priceX96 implies that it returns an "X96" (base 2**96) price when it returns a "WAD" price (base 10**18).ADDRESS0, ADDRESS1, and ADDRESS2 public state variables in the YieldOracle contract.from and to arguments of the AddOperator and RemoveOperator events are unclear which argument corresponds to the user and which is the operator.sender argument for the InsufficientBalance error is the account with an insufficient balance and is not necessarily the account making the call.amountIn argument of the _simulateGivenOutBalancerSwap function in the UniswapFlashloanBalancerSwapHandler contract should be named amountOut, since it represents an amount "out" from the Balancer swap.Consider addressing these naming issues to improve the readability of the codebase.
Update: Partially resolved in pull request #35. All instances except those in YieldOracle have been fixed.
Throughout the codebase, contract types are used for external contracts that could be upgraded or changed. If the contracts will be verified on a block explorer, and any of these external contracts are later updated, the verified code would be misleading.
In particular:
Whitelist contract within the IonPool contractInterestRate contract within the IonPool contractSpotOracle contract within the IonPool contractConsider defining implementation-agnostic interfaces that define the expected functionality these contracts will implement and using these for the types. This will prevent old contract code from being treated as current on block explorers after these contracts have been upgraded.
Update: Acknowledged, not resolved. Ion Protocol team stated:
Acknowledged, but was not fixed in this audit as we do not foresee these contract interfaces changing in the future.
The ilkCount and addressesLength functions in the IonPool contract are redundant as they will always return the same value. Consider removing one of these redundant functions.
Update: Resolved in pull request #35. Note that the addressesLength function has been removed.
_depositAndBorrowWithin the clause for if(amountToBorrow != 0), there is only the borrow call. Since the computations for normalized borrow amount will not be used if amountToBorrow == 0, consider moving them inside this clause.
Update: Resolved in pull request #35.
flashLeverageCollateralIn the flashLeverageCollateral function, the case where amounts[0] == 0 signifies that there is no leverage needed.
Since amounts[0] == amountToLeverage, consider moving this check up to right after amountToLeverage is determined.
Update: Resolved in pull request #35.
Consider correcting the following typographical errors to improve the readability of the codebase:
IonPausableUpgradeable.sol, "Depnding" should be "Depending".SwEthReserveOracle.sol, "Swel" should be "Swell".UniswapFlashswapHandler.sol, "sandwhich" should be "sandwich".IonPool.sol, "Total unbacked Dai" should be "Total unbacked WETH".Update: Partially resolved in pull request #35. All instances except the last bullet have been fixed.
Throughout the codebase, misleading and inaccurate documentation was identified. In particular:
IonPausableUpgradeable contract states that there are three pause states when the contract only contains two._aggregate function in the ReserveOracle contract states that it calculates the minimum of the exchange rates returned from the reserve feeds when the function returns the average._aggregate function in the ReserveOracle contract states that it will revert if the quorum isn't met when the function will only revert if calling getExchangeRate on one of the reserve feeds reverts.getPrice function in the EthXSpotOracle contract states that the Redstone oracle returns 6 decimal places when it returns 8.EthXSpotOracle.sol state that these are for the price of stETH in ETH when it is the price of ETH in USD.updateExchangeRate and getExchangeRate functions in the IReserveFeed interface state that the functions update and get the total reserve when they update and get the exchange rate of the asset against ETH.IlkData struct in InterestRate.sol document how the values are packed into the ILKCONFIG variables but does not align with how the values are packed in memory within the struct. Specifically, the reserveFactor value would be part of the first word for the packed struct.kpr argument to the liquidate function in the Liquidation contract states that it is the payer of the debt when the payer of debt is msg.sender.Liquidation.sol states that the exchange rate is returned as a uint72 type when it is a uint256 type.UniswapFlashswapHandler.sol state that the token is "swEth" when it is used for both swEth and wstEth.Consider clarifying all in-line documentation to ensure developers and auditors understand the intended purpose of the code.
Update: Acknowledged, will resolve. Ion Protocol team stated:
The documentation will be reviewed and edited post-audit to include all up-to-date information.
Throughout the codebase, there are instances of more than one interface with the same name. For instance:
Consider renaming the interfaces to avoid unexpected behavior and improve the overall clarity and readability of the codebase.
Update: Resolved in pull request #35.
Within the getEthAmountInForLstAmountOut function in the StaderLibrary library, the exchangeRate is queried using the getExchangeRate function. The exchangeRate is a public variable within the StaderOracle contract and be queried directly to save gas.
Consider directly querying the exchangeRate variable from the StaderOracle instead of calling the getExchangeRate function.
Update: Acknowledged, not resolved. Ion Protocol team stated:
The EthxReserveOracle contract's public exchange rate variable stores the exchange rate that is delayed and bounded for oracle security. The library queries real time exchange rate through the getter as it should operate on the up to date exchange rate and not the bounded exchange rate. Will not fix.
UniswapFlashswapHandlerWithin the UniswapFlashswapHandler contract, the two functions flashswapLeverage and flashswapDeleverage are designed to change a user's leverage by a specified amount.
Within both of these functions, there is a check which reverts only if the amount received is not the expected output and the sqrtPriceLimitX96 is unspecified (equal to 0). Since it is intended that amountOutReceived be exactly equal to amountOut in all cases, this check can remove the conditional regarding sqrtPriceLimitX96.
Consider removing the stipulation that sqrtPriceLimitX96 is 0 in order to catch all instances in which the amount received does not match the exact desired amounts. Note that the sqrtPriceLimitX96 check is applied within Uniswap. Also consider moving the check before the _depositAndBorrow or _repayAndWithdraw function calls, to follow the fail-early-and-loudly principle and help users understand when their transactions fail. Though these calls should fail if the desired amount out is not obtained (due to failures within _depositAndBorrow or _repayAndWithdraw), they may not fail if there are already tokens in the UniswapFlashswapHandler contract.
Update: Resolved in pull request #28.
When the IonPool contract is in a safe pause state, interest is not intended to be accrued for vaults as the functionality for repaying debt is disabled. The _accrueInterest function prevents interest accrual during a safe pause by returning early. However, the _accrueInterestForIlk function does not implement the same logic and will allow for interest accrual during a safe pause state. As a result, when a user borrows against their collateral, interest will be accrued for all vaults for the corresponding collateral type.
Consider disallowing interest accrual within the _accrueInterestForIlk function during a safe pause as is done in the _accrueInterest function.
Update: Resolved in pull request #27.
Throughout the codebase, there are missing return value checks for ERC-20 transfers:
BalancerFlashloanDirectMintHandler.solIonHandlerBase.solUniswapFlashloanBalancerSwapHandler.solUniswapFlashloanBalancerSwapHandler.sol.Consider validating the return value from these ERC-20 transfers or using the SafeERC20 library to ensure that transfers have been successful.
Update: Resolved in pull request #22.
The UniswapFlashloanBalancerSwapHandler, BalancerFlashloanDirectMintHandler and UniswapFlashswapHandler contracts do not contain the onlyWhitelistedBorrowers and onlyWhitelistedLenders modifiers. As a result, even users who are not whitelisted can create positions in the vault.
Consider adding the onlyWhitelistedBorrowers modifier to the functions in the flash leverage contracts. Changing the whitelisting modifiers in the IonPool contract to check user rather than msg.sender would also prevent non-whitelisted users from opening positions.
Update: Resolved in pull request #25.
Given the complexity of the system, we recommend building a comprehensive test suite covering the entire system using realistic parameter values. An extensive and well-designed test suite is critical for minimizing the risk of unintentionally introducing issues during future development and may aid in identifying unintended features of the existing codebase.
We recommend:
IonPool contract with realistic parameter valuesIonPool contract unit tests to ensure that the invariants holdA comprehensive review of the test suite was not performed and the above list only includes some specific areas noted during the audit.
Several key areas where continuous monitoring would benefit the health of the protocol were identified:
BalancerFlashloanDirectMintHandler assumes balancer flash loans are feeless.The above monitoring recommendations are a non-comprehensive list. It is recommended that Ion Protocol team build an extensive monitoring suite to ensure the proper functionality of the system.
A number of critical and high severity issues were identified and many suggestions have been made to improve the overall robustness of the codebase and minimize the risk of unintentionally introducing issues into the codebase during future development.
To minimize the potential for critical issues being exploited, we recommend establishing additional security controls such as:
In addition, due to the complexity of the system and the total number of critical, high and medium severity issues discovered, we believe that the codebase would benefit from an additional external review after addressing the issues presented in this report. Any future changes to the codebase should be externally reviewed before public releases.
Overall, Ion team was quite responsive throughout the audit period and engaged in useful discussions that greatly aided our understanding of the system.