Ion Protocol weETH Integration Audit

OpenZeppelin Security | February 27, 2024

Table of Contents

Summary

Type
DeFi
Timeline
From 2024-02-05
To 2024-02-09
Languages
Solidity
Total Issues
9 (7 resolved, 2 partially resolved)
Critical Severity Issues
0 (0 resolved)
High Severity Issues
0 (0 resolved)
Medium Severity Issues
2 (2 resolved)
Low Severity Issues
1 (0 resolved, 1 partially resolved)
Notes & Additional Information
4 (3 resolved, 1 partially resolved)
Client Reported Issues
2 (2 resolved)

Scope

We audited the Ion-Protocol/ion-protocol repository at commit 5333fe3.

The following files were in scope:

 src
├── flash
│   └── handlers
│       ├── base
│       │   └── UniswapFlashloanDirectMintHandler.sol
│       └── WeEthHandler.sol
├── interfaces
│   └──  IRedstone.sol
├── libraries
│   └── EtherFiLibrary.sol
├── oracles
│   ├── reserve
│   │   └── WeEthWstEthReserveOracle.sol
│   └── spot
│       └── WeEthWstEthSpotOracle.sol
└── Constants.sol

Additionally, the following files were reviewed for changes between commit 81d5aee and commit 5333fe3.

 src
├── flash
│   └── handlers
│       └── base
│           └── IonHandlerBase.sol
├── interfaces
│   └── ProviderInterfaces.sol
└── YieldOracle.sol

Further, the src/IonPool.sol file was reviewed for changes between commit c4d5479 and commit 5333fe3.

Update: After all fixes were reviewed, a final review was performed on pull request 59 at commit 0f2cdf6 that merged all changes into a single pull request. This pull request has been validated and confirmed that all fixes have been properly addressed and no unintentional issues have been introduced.

System Overview

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

This portion of the audit reflects changes to the system in order to add the weETH token from EtherFi as a collateral asset. In addition, changes have been made to generalize borrowing for any asset, allowing weETH to collateralize the borrows of wstETH.

The changes implement new code for borrowing wETH from Uniswap, and using it to mint weETH and borrow wstETH against it. Moreover, new oracles have been implemented, utilizing Chainlink and Redstone oracles, to price weETH within the system.

New Oracle

Multiple oracles have been 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 ETH. 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. Compared to the prior audit, the new source is for the weETH token and the liquid restaking token (LRT), and uses a Redstone oracle.

Internally, the collateralization value of these assets is taken to be 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 the reserve oracle rates.

New 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 the 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 following integrations were added:

  • The spot price for the weETH token is obtained using a Redstone oracle.
  • Internal conversion rates for ETH-to-LST are queried from the new weETH LST system and used as a maximum bound for collateralization values for this LST.
  • The weETH contract is used for minting weETH in exchange for wETH.

All external integrations are assumed to work as documented for the purposes of this audit.

Security Model and Monitoring Concerns

The system is dependent on the weETH and eETH contracts behaving as expected. In addition, "black swan" events which may affect the value of eETH and weETH tokens may have negative effects on the protocol, especially when it comes to valuing collateral or liquidations. Consider monitoring the following:

  • Monitor the weETH, eETH, eETH Liquidity Pool, and eETH Liquifier contracts for upgrades. Since all of these contracts are proxies, ensure that after an upgrade, all fork tests still pass, and compare the logic of any called functions to the previously implemented logic.
  • Monitor the rate of shares to total pooled ETH in the eETH contracts to quickly identify problems with the invariant that ETH per share should increase slowly over time.
  • Monitor all calls interacting with the eETH contracts and ensure that the values for minted tokens correspond to the wETH amounts involved.

System Design Concerns

There exists a section of code within the EtherFiLibrary contract in which computations for converting between eETH, weETH, and ETH incur some rounding error. Additions of 1 resolve some of the rounding errors and, based on testing, appear to allow the contract to function properly. Although this was examined by the auditors, there was not enough time to conclusively determine the source of the rounding error. As such, we recommend reviewing the rounding error in more detail and either documenting the source of the missing unit(s) or implementing robust functions which account for and predict rounding errors. Note that there were no explicit vulnerabilities found regarding these calculations.

 

Medium Severity

Incorrect Check in UniswapFlashswapDirectMintHandler

The check against maxResultingDebt in the UniswapFlashswapDirectMintHandler contract is incorrect and may mislead users. The intent for this handler is to help leverage positions within the wstETH / weETH market. The maxResultingDebt parameter is intended to limit the borrowed amount for the user, measured in terms of the "base asset".

Here, the base asset should be wstETH. However, the check compares this value to an amount of wETH. Since wETH units are worth slightly less than wstETH, this will have the effect of causing reverts more often for users as the threshold for "max resulting debt" will be lower than they are anticipating. This will result in a poor user experience overall, as well as potential errors in calculations from trying to convert to a reasonable wstETH value.

Consider either changing the docstrings or name of the variable to make it clear that this value represents wETH. Alternatively, consider converting the amountWethToFlashloan value to an equivalent value of wstETH before the check.

Update: Resolved in pull request #52.

UniswapFlashswapDirectMintHandler Unusable When Not Leveraging

Within the UniswapFlashswapDirectMintHandler contract, the _flashswapAndMint function contains a clause that is unreachable. This is due to the fact that the getEthAmountInForLstAmountOut function in the EtherFiLibrary contract always returns a value of 1 or greater. Intuitively, for an amount "out" of 0, the amount "in" should also be 0. However, due to the addition of 1, this is not possible. The UniswapFlashswapDirectMintHandler contract is designed to handle cases in which the amountLrt is 0, and when this is the case, we will get an amountWethToFlashloan of 1.

Not only does this mean that an unnecessary flash-borrow will occur, but the transaction execution will fail as well. During the flashswap callback, the contract will attempt to mint the collateral asset using the value 1. This will eventually revert inside the eETH liquidity pool contract since the division in _sharesForDepositAmount returns 0. This prevents the contract from functioning under normal circumstances if a user were to use flashswapAndMint for depositing collateral and borrowing. Note that this is technically not a given and is dependent on the number of shares being less than the total pooled ETH in the eETH contract.

Consider adding an early-escape check within getEthAmountInForLstAmountOut, which returns 0 when lrtAmount is 0.

Update: Resolved in pull request #53.

Low Severity

Missing Docstrings

Throughout the codebase, there are several parts that do not have docstrings:

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: Partially resolved in pull request #54. the following instances remain unresolved:

Notes & Additional Information

Unused Return Variable

Named return variables are a way to declare variables that are meant to be used within a function's body for the purpose of being returned as the function's output. They are an alternative to explicit in-line return statements.

The amountIn return value for the _initiateFlashSwap function in the UniswapFlashswapDirectMintHandler contract is being set but not used in the rest of the function or in the _flashswapAndMint function it returns to.

Consider either using or removing any unused named return variables.

Update: Resolved in pull request #52.

Typographical Errors

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

  • At line 216 of UniswapFlashswapDirectMintHandler.sol, "the a" should be "the".
  • At lines 222 and 223 of UniswapFlashswapDirectMintHandler.sol, "eth" should be "mint asset".
  • At lines 43 and 44 of WeEthWstEthReserveOracle.sol, "rate between ETH and weETH" should be "rate between wstETH and weETH".

Update: Resolved in pull request #54.

Unused Variables

Throughout the codebase, there are multiple unused variables:

To improve the overall clarity, intentionality, and readability of the codebase, consider removing any unused variables.

Update: Resolved in pull request #54.

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 is quite beneficial as it permits the code owners to dictate the communication channel for vulnerability disclosure, eliminating the risk of miscommunication or failure to report due to a lack of knowledge on how to do so. In addition, if the contract incorporates third-party libraries and a bug surfaces in these, it becomes easier for the maintainers of those libraries to make contact with the appropriate person about the problem and provide mitigation instructions.

Throughout the codebase, there are contracts that do not have a security contact:

Consider adding a NatSpec comment containing a security contact above the contract definitions. Using the @custom:security-contact convention is recommended as it has been adopted by the OpenZeppelin Wizard and the ethereum-lists.

Update: Partially resolved in pull request #54. The instances pertaining to the interfaces are not fixed.

Client Reported

Liquidation Contract Incompatible With IonPool When ilkIndex Less Than Three

Inside the constructor of the Liquidation contract, the _maxDiscounts, _liquidationThresholds, and _reserveOracles arguments are implicitly expected to have lengths of at least three. However, the lengths of these arrays are compared to the ilkCount returned from the input instance of the IonPool contract. If the instance of the IonPool contract uses an ilkCount of less than three, the constructor will revert when attempting to assign the MAX_DISCOUNT_ state variables with an out-of-bounds error for the array.

Consider refactoring the constructor to accept any non-zero number of ilk indices.

Update: Resolved in pull request #59 at commit 8c4f615.

Reserve Factor Incorrect Decimals Validation

Within the InterestRate contract, the reserveFactor uses 4 decimal places of precision. However, the constructor for the contract incorrectly validates that the input has 27 (RAY) decimals of precision.

Consider updating the validation in the constructor to check for 4 decimals of precision.

Update: Resolved in pull request #58.

 
 

Conclusion

The audit covered changes made to previously audited files along with new components to support allowing the weETH token to be used as collateral. Although the changes are minimal, the introduction of new integrations added further complexity to the overall system. Generally, the complexity was added due to the integrations with the new token, eETH/weETH, and generalizing the contracts used for managing positions to accommodate the wstETH token as a borrowable asset. Two medium-severity issues were identified and recommendations to improve the overall quality and health of the codebase have also been made.

Due to the introduction of additional integrations to the system, advancing the test suite and monitoring that the external contracts are behaving as expected would be of great benefit. Specifically, testing for edge cases, contract upgrades to the eETH/weETH contracts, and testing all logical branches of the swap handler code would provide greater certainty of their behaviors. The Ion team was responsive throughout the audit period, in both answering questions and providing supplementary information about the system.

Appendix - Issue Classification

Critical: The issue puts a large number of users’ sensitive assets at risk and is reasonably likely to have a catastrophic impact on the client’s reputation or serious financial implications for the client or users. It often involves, but is not limited to, some form of loss or locking of funds or other core system functionality failures.

High: The issue puts a large number of users’ sensitive assets at risk and is reasonably likely to have a high impact on the client’s reputation or notable financial implications for the client or users. It often involves, but is not limited to, some form of temporary loss or locking of funds or other core system functionality failures for which reasonable mitigations may be available.

Medium: The issue puts a subset of users’ sensitive assets at risk, which would be detrimental to the client’s reputation if exploited or is reasonably likely to have a moderate financial impact on the client and users. Alternatively, the issue results from an implementation that does not adhere to the specified system design but does not result in financial impact.

Low: The issue is relatively small, could not be exploited regularly, or is a risk that the client has identified as having a low impact given their business model.

Notes & Additional Information: This type of issue is not exploitable but still worth resolving to increase the security and quality of the codebase.

 

Related Posts