OpenZeppelin
Skip to content

Ion Protocol Audit

OpenZeppelin Security | January 22, 2024

Table of Contents

Summary

Type
DeFi
Timeline
From 2023-11-14
To 2023-12-15
Languages
Solidity
Total Issues
71 (40 resolved, 10 partially resolved)
Critical Severity Issues
2 (2 resolved)
High Severity Issues
4 (3 resolved)
Medium Severity Issues
16 (10 resolved, 1 partially resolved)
Low Severity Issues
12 (6 resolved)
Notes & Additional Information
34 (16 resolved, 9 partially resolved)
Client Reported Issues
3 (3 resolved)

Scope

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.

System Overview

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:

  • The GemJoin contract which should be deployed for each collateral type and is where users deposit and withdraw their LSTs into/from the system.
  • The 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.
  • The YieldOracle and InterestRate contracts are used to determine the interest rates for debt positions.
  • The 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.
  • The 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.
  • The reserve oracle contracts determine the exchange rate of the LSTs directly from the LST contracts or related contracts from the LST providers and these prices are used for determining vault health during liquidations.
  • The spot oracle contracts use price oracles to pull spot prices for the LSTs, these are used for determining the health of a vault along with the reserve price when modifying the position in a vault.
  • Flash leverage contracts simplify the user experience for leveraging or deleveraging a vault. See the Flash Leverage section below for details.

Protocol Features

Whitelist

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.

Pausability

The protocol includes a mechanism for pausing IonPool actions with two independent pausing sets:

  • Unsafe pausing where only "unsafe" actions are paused. Unsafe actions are any actions that would decrease the health of a vault or of the system, this includes withdrawing WETH, withdrawing collateral from a vault, borrowing additional WETH, and transferring collateral to a different vault.
  • Safe pausing where only "safe" actions are paused. Safe actions are any actions that generally make a vault or the system safer. These include accruing interest, supplying WETH, repaying debt, repaying "bad" debt, and conducting liquidations. The intention is for safe actions to only be paused when unsafe actions are also paused. However, it is currently possible for safe actions to be paused while unsafe actions are not paused. A recommendation has been made here to couple the pauses together.

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.

GemJoin

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

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:

  • wstETH: Chainlink
  • swETH: Uniswap TWAP oracle
  • ETHx: A combination of Chainlink and Redstone oracles

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.

Liquidations

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.

Flash Leverage

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.

External Integrations

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:

  • The spot price for the swETH token is obtained using Uniswap TWAP oracle.
  • The spot price for the wstETH token is obtained using a Chainlink oracle.
  • The spot price for the ETHx token is obtained using a combination of a Chainlink and Redstone oracle.
  • Uniswap pools are used for swapping tokens when opening a leveraged position using the flash leverage functionality.
  • A Balancer pool is used for taking a flash loan when opening a leveraged position.
  • Internal conversion rates for ETH-to-LST are queried from the different LST systems (wstETH, swETH, and ETHx) and used as a maximum bound for collateralization values for these LSTs.

Security Model

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:

  • Ensure that there is a documented plan of how to respond to various scenarios both known and unknown. Have a chain of command for resolving black-swan events quickly.
  • Define "known unknowns" (e.g., a validator slashing event or a smart-contract bug in one of the LSTs) and what admin actions to take in each case.
  • Understand when to pause the protocol and document what procedures would be required to get the protocol operational again.
  • Write and test scripts to pause the entire protocol atomically. Similarly, write and test scripts which unpause the protocol fully and in pieces.
  • Periodically ensure that actors who implement a pause are able to fulfil their duty quickly which includes being contactable, validating that a request to pause the system is legitimate, and being able to access keys and sign messages in order to implement a pause. This may involve running "fire drills". If this is the case, ensure accurate note-taking is done to understand who is the quickest to respond and resolve any infrastructure-related issues.
  • Understand how slashing events may impact the protocol.
  • Understand how high-volatility environments may impact the protocol where block space may be very expensive. As a result, liquidations may not be processed in a timely manner, oracle prices may not be updated, etc.
  • Monitor for positions which may need shortfall liquidations and perform shortfall liquidations quickly.
  • Outline a plan or plans for dealing with bad debt in the case that the protocol has shortfall liquidations.
  • Set boundaries based on gas prices and ETH prices to determine when to raise or lower the 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.
  • Whenever upgrading contracts, perform storage layout checks in a simulated test environment. Ensure that any old state is correctly managed, especially in the case of deprecating a collateral type. In the case that state variables should no longer be used, consider declaring an unused variable in that storage slot and documenting what it used to be.

Upgradeability

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:

  • If a collateral type is ever removed from the contract, the corresponding ilkIndex should never be reused because the old data for the removed collateral type would remain in storage for the contract.
  • While the use of EIP-7201 prevents storage collisions between contracts, it does not prevent issues if variables are reordered within a contract. Thus, if any variables are ever removed from the contract, a gap filler variable must be included in place of the original variable to ensure that the old data from storage is not incorrectly accessed.

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.

Privileged Roles

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:

  • Upgrade the treasury address in the RewardModule contract.
  • Add new collateral types with the initializeIlk function.
  • Change the spot oracle for a collateral type with the updateIlkSpot function.
  • Change the debt ceiling for collateral types with the updateIlkDebtCeiling function.
  • Change the dust threshold for collateral types with the updateIlkDust function.
  • Update the supply limit with the updateSupplyCap function.
  • Update the interest rate module with the updateInterestRateModule function.
  • Update the whitelist with the updateWhitelist function.
  • Pause and unpause safe and unsafe actions with the pauseUnsafeActions, unpauseUnsafeActions, pauseSafeActions, and unpauseSafeActions functions.
  • Administer the 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.

System Design Concerns

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:

  • External functions should perform their own validation of input arguments and state. Invalid input should not be propagated down the call chain as this may cause confusion and important checks may be forgotten in future development efforts.
  • Several processes were identified that require multiple steps or potentially simultaneous execution:
    • The pausing and unpausing of the 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.
    • The process for adding a new collateral type to the 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.
  • Several components of the protocol require redeployment when updating rather than natively supporting features such as adding a new collateral type (e.g., the Whitelist and Liquidation contracts). This increases the potential for errors arising in developing and deploying new versions of these contracts.
  • Several contracts have been extensively optimized which leads to complex code that can be difficult to modify. The optimizations made to these contracts should be thoroughly documented such that developers are aware of the intention behind them. Generally, over-optimization is not recommended as it can obscure complex vulnerabilities and makes code difficult for developers to understand.

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

 

Critical Severity

No Access Control on ReserveFeed Contract

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

Repaying Debt With IonZapper Will Lock WETH in Contract

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

High Severity

repayAndWithdraw Will Frequently Fail for Full Repayments

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

Lack of ERC-20 Approvals Prevents IonZapper Contract From Functioning as Intended

The 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:

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.

Ineffective Bound of Exchange Rate Change in Reserve Oracle

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.

Observation Cardinality Insufficient for Uniswap SwETH Oracle

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.

Medium Severity

Liquidations Use Inaccurate rate Values

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

Missing Deadline Check for Functions Which Call Uniswap v3 FlashSwap

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

Whitelist Modifiers Check msg.sender Rather Than User

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

Collateral Rate Can Be Overwritten

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.

Pausing Does Not Accrue Interest as Intended

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.

Timestamp Discrepancy in Spot Oracle Price Feeds May Cause Inaccurate Price

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:

  • A user could take on less additional debt if the oracle underprices their collateral.
  • A user could take on more debt than intended when collateral is over-priced, although this overpricing will never exceed the reserve exchange rate.

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.

Oracle Data Lacks Validation

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:

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.

Inconsistencies When Adding a New Collateral Type

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:

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.

Inconsistency When Accruing Interest for a Single ILK vs. All ILKs

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.

Lack of Input Validation

Throughout the codebase, functions lack validation of input arguments:

  • Within the ReserveOracle contract:
    • The constructor does not validate that the _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.
    • The constructor does not validate that the _maxChange argument is non-zero and less than 1e27 (1 RAY).
    • The _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.
  • Within the SwEthSpotOracle contract:
    • The constructor does not validate that the _secondsAgo argument is non-zero.
  • Within the YieldOracle contract:
    • The constructor does not validate that none of the _historicalExchangeRates values are non-zero. A zero value would result in a division by zero on line 127.
    • The _getExchangeRate function does not validate that the ilkIndex value is within the expected range and will return 0 for an invalid input.
  • Within the InterestRate contract:
    • The constructor does not validate that the minimumKinkRate values are at least as large as the minimumBaseRate values. If this condition does not hold, the calculateInterestRate function will revert.
    • The constructor does not validate that the 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.
    • The constructor does not validate that the optimalUtilizationRate values are non-zero. A zero optimalUtilizationRate will cause the calculateInterestRate function to revert.
    • The constructor does not validate that the 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.
  • Within the IonPool contract:
    • The mintAndBurnGem, collateral, normalizedDebt, vault, and gem functions do not validate that their ilkIndex arguments are within the supported range.
    • The 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.
  • Within the Liquidation contract:
    • The constructor does not validate that the length of the _maxDiscount array is 3.
    • The constructor does not validate that the _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.
    • The constructor does not validate that _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.
    • The constructor does not validate that the _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.
    • The constructor does not validate that for each collateral index (i), _targetHealth >= _liquidationThresholds[i] / (RAY - _maxDiscount[i]). This invariant must hold otherwise all liquidations will revert when discount == configs.maxDiscount within the _getRepayAmt function.
    • The _getConfigs function does not validate that the ilkIndex argument is within the expected range.
  • Within the UniswapFlashswapHandler contract:

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 IonPool input validation.

2. Will not fix: “The flashswapLeverage function does not validate that resultingAdditionalCollateral >= initialDeposit”. This is Implicitly checked by Solidity’s overflow protection and does not need to be fixed.

Protocol Liquidations Are Not Pausable

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.

Incorrect Interest Accrual When No Debt

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.

Users May Be Able to Borrow swEth at an Outdated Price

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

Locked ETH in Contract

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:

Consider removing the payable attribute or adding a withdrawal feature.

Update: Resolved in pull request #23.

No Incentive to Perform Protocol Liquidation

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.

Low Severity

Lack of Validation of Treasury Address Could Halt Contract

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.

Interest Not Accrued Prior to Withdrawing or Depositing Collateral

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.

Floating Pragma

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:

Consider using a fixed Solidity version throughout the codebase.

Update: Acknowledged, not resolved. Ion Protocol team stated:

This is intended. Will not fix.

Liquidations May Use Outdated Reserve Oracle Exchange Rate

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.

Missing or Incomplete Docstrings

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.

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.

Infinite Loop Prevents Interest Accrual

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.

Users Cannot Deposit Collateral into a Dusty Vault

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.

Functions Return Inaccurate Values Without Interest Accrual

Several view functions in the IonPool and RewardModule contracts return inaccurate values as they do not accrue interest prior to being called.

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

The YieldOracle Contract Returns Stale Data After Price Decrease

The _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.

ERC-20 Transfer Should Occur Before Updating Internal State

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.

Incorrect Check in BalancerFlashloanDirectMintHandler

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

Whitelist Can Be Bypassed for New Collateral Type

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.

Notes & Additional Information

Functions and variables should be private

Throughout 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:

  • The ILKCONFIG variables in the InterestRate contract as the data stored in these variables should only ever be read using the _unpackCollateralConfig function.
  • The _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.

Unreachable Code

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.

"Magic" Values are Used

Throughout the codebase, magic numbers are used. In particular:

  • On lines 203, and 492 of IonPool.sol.
  • On lines 29, 192, 232, and 259 of UniswapFlashloanBalancerSwapHandler.sol.
  • On line 12 of 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.

Missing Arguments for Events

Throughout the codebase, there are several events that would benefit from including additional arguments. In particular:

  • Several events in the IonPool contract should include the ilkIndex as an argument as the events are emitted when only a specific ilk is updated.
  • The 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 IonPool events will come after the audit since they are housed in pull request #17.

Redundant Argument for Error

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.

Constants Not Using UPPER_CASE Format

Throughout the codebase, there are constants not using UPPER_CASE format. For instance:

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

Unused Imports

Throughout the codebase, there are multiple imports that are unused and could be removed. For instance:

Consider removing unused imports to improve the overall clarity and readability of the codebase.

Update: Resolved in pull request #35.

Todo Comments in the Code

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:

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.

Multiple Instances of Missing Named Parameters in Mappings

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:

Consider adding named parameters to the mappings to improve readability and maintainability of the code.

Update: Resolved in pull request #35.

Unused Errors

Throughout the codebase, there are unused errors. For instance:

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

Functions Are Updating the State Without Event Emissions

Throughout the codebase, instances of functions that are updating the state without an event emission were found. For instance:

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 _initializeExchangeRate function of the ReserveOracle contract).

Inconsistent Use Named Returns

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.

Unnecessary Variable Casts

Throughout the codebase, there are multiple functions that have variables that are unnecessarily cast. For instance:

To improve the overall clarity, intent, and readability of the codebase, consider removing unnecessary casts.

Update: Resolved in pull request #35.

Unused Variables

Throughout the codebase, there are multiple unused variables. For instance:

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.

State Variable Visibility Not Explicitly Declared

Throughout the codebase, there are state variables that lack an explicitly declared visibility. For instance:

For clarity, consider always explicitly declaring the visibility of variables, even when the default visibility matches the intended visibility.

Update: Resolved in pull request #35.

Lack of Security Contact

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.

Unused Named Return Variables

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:

  • The newTotalDebt return variable in the accrueInterest function in IonPool.sol.
  • The supplyFactorIncrease return variable in the calculateRewardAndDebtDistribution function in IonPool.sol.
  • The treasuryMintAmount return variable in the calculateRewardAndDebtDistribution function in IonPool.sol.
  • The newRateIncrease return variable in the calculateRewardAndDebtDistribution function in IonPool.sol.
  • The newDebtIncrease return variable in the calculateRewardAndDebtDistribution function in IonPool.sol.
  • The newTimestampIncrease return variable in the calculateRewardAndDebtDistribution function in IonPool.sol.
  • The 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.

Unsafe and Safe Pauses and Unpauses Should Occur Simultaneously

The IonPausableUpgradeable contract defines two separate pause states and is used by the IonPool contract. The states are:

  • Safe pause where safe actions are disabled including depositing collateral, moving collateral into a vault, repaying debt, and accruing interest.
  • Unsafe pause where unsafe actions are disabled including withdrawing collateral from a vault, transferring collateral between vaults, borrowing, and withdrawing collateral.

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.

Missing EIP-7201 Annotation

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:

Consider annotating the storage structs in these contracts with @custom:storage-location.

Update: Resolved in pull request #35.

Coding Style Deviates from Solidity Style Guide

There are several files throughout the codebase that deviate from the recommended Solidity Style Guide. In particular:

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.

No Method To Determine Last Update to Reserve Oracles

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.

Abstract Contracts Allow Direct Modification of State Variables

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 Visibility

The following public functions should be external:

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.

Variables could be immutable

Throughout the codebase, there are several variables that could be immutable. For instance:

  • The protocolFeed variable in the EthXReserveOracle contract.
  • The 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.

Naming Issues Hinder Code Understanding and Readability

Throughout the codebase, there are several functions and variables that could be renamed to better reflect their purpose, in particular:

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.

Interfaces should be used for type definitions

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:

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

Redundant Functions

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.

Inefficiency in _depositAndBorrow

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

Inefficiency in flashLeverageCollateral

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

Typographical Errors

Consider correcting the following typographical errors to improve the readability of the codebase:

  • On line 15 of IonPausableUpgradeable.sol, "Depnding" should be "Depending".
  • On line 25 of SwEthReserveOracle.sol, "Swel" should be "Swell".
  • On line 114 of UniswapFlashswapHandler.sol, "sandwhich" should be "sandwich".
  • On line 136 of 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.

Misleading and Inaccurate Documentation

Throughout the codebase, misleading and inaccurate documentation was identified. In particular:

  • The docstring for the IonPausableUpgradeable contract states that there are three pause states when the contract only contains two.
  • The docstring for the _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.
  • The docstring for the _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.
  • The docstring for the getPrice function in the EthXSpotOracle contract states that the Redstone oracle returns 6 decimal places when it returns 8.
  • The comments on lines 45 and 46 in EthXSpotOracle.sol state that these are for the price of stETH in ETH when it is the price of ETH in USD.
  • The docstrings for the 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.
  • The inline documentation for the 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.
  • The documentation for the 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.
  • The comment on line 167 of Liquidation.sol states that the exchange rate is returned as a uint72 type when it is a uint256 type.
  • The comments on lines 73, 74, 92, and 123 of 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.

Interfaces With the Same Name

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.

Gas Optimization By Directly Querying External Variable

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.

Over-Constrained Check in UniswapFlashswapHandler

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

Client-Reported

Borrow Actions Continue to Accrue Interest During Pause

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.

Missing Return Value Check for ERC-20 Transfer

Throughout the codebase, there are missing return value checks for ERC-20 transfers:

  • At line 189 of BalancerFlashloanDirectMintHandler.sol
  • At line 132 of IonHandlerBase.sol
  • At line 212 or UniswapFlashloanBalancerSwapHandler.sol
  • At line 242 of UniswapFlashloanBalancerSwapHandler.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.

Flash Leverage Contracts Lack Whitelist Checks

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.

 

Recommendations

Testing

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:

  • Increasing test coverage to 100%
  • Testing the IonPool contract with realistic parameter values
  • Testing the impact of various parameter changes (increasing dust, decreasing debt ceiling, etc.)
  • Testing accruing interest (incrementing the block timestamp) within the IonPool contract unit tests to ensure that the invariants hold

A comprehensive review of the test suite was not performed and the above list only includes some specific areas noted during the audit.

Monitoring

Several key areas where continuous monitoring would benefit the health of the protocol were identified:

  • Monitor the swETH pool in Uniswap for suspicious activity affecting the TWAP price, including swaps and large increases or decreases in liquidity.
  • Monitor the Chainlink wstETH oracle to ensure it is returning accurate and up-to-date information.
  • Monitor the Redstone and Chainlink oracles associated with pricing ETHx. Ensure that they are returning accurate and up-to-date information. Furthermore, ensure that combining their rates results in an accurate price of ETHx.
  • Monitor the upgrades to the LST token contracts. These upgrades may change or break functionality that Ion integrates with. Monitor for changes to the internal exchange rates within the LST contracts.
  • Ensure liquidations occur in a timely manner (to avoid requiring a protocol liquidation and creating bad debt)
  • Monitor bad debt
  • Monitor all external price feeds for anomalous data.
  • Monitor for changes to the fees charged for Balancer flash loans. Note that BalancerFlashloanDirectMintHandler assumes balancer flash loans are feeless.
  • Monitor for slashing events to relevant validators.
  • Monitor for vault operations which seem suspicious, such as frequent calls to the same functions from the same addresses, or calls which result in a user's holdings increasing in value.
  • Monitor for suspicious calls to integrated LSTs, such as large amounts of tokens being minted, burned, or moved, or mints and burns which do not match the internal exchange rate of the LST.
  • Monitor the liquidity of all flash loan and flash swap sources. Monitor for suspicious behavior like large liquidity deposits/withdrawals or large swaps.
  • Monitor for sandwiching attacks or frontrunning attacks by checking the transactions near protocol interactions for profitable swaps with protocol collaterals.
  • Monitor all protocol actions to ensure compliance with invariants. For example, ensure that no user can borrow more than they owe, and that vaults are liquidatable when their health factors are below the threshold.
  • Monitor for protocol access and vault manipulation for users who are not whitelisted in order to detect faulty enforcement of the whitelist.

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.

 

Conclusion

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:

  • Extensive monitoring across all the key areas of the system.
  • Comprehensive incident response plan with tools supporting automated response.
  • Closed whitelisted live testing of the protocol after initial deployment to allow for the monitoring of potential issues before opening up the protocol to the public.

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.

Related Posts